This in effect copies similar logic from xend: While there's no way to
check whether a device is assigned to a particular guest,
XEN_DOMCTL_test_assign_device at least allows checking whether an
IOMMU is there and whether a device has been assign to _some_
guest.
For the time being, this should be enough to cover for the missing
error checking/recovery in other parts of libxl's device assignment
paths.
There remains a (functionality-, but not security-related) race in
that the iommu should be set up earlier, but this is too risky a
change for this stage of the 4.3 release.
This is a security issue, XSA-61.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
If the DTB is loading right after the kernel, on some setup, Linux will
overwrite the DTB during the decompression step.
To be sure the DTB won't be overwritten by the decompression stage, load
the DTB near the end of the first memory bank and below 4Gib (if memory range is
greater).
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
When GICD_ICENABLERn is read, all the 1s bit represent enabled IRQs.
Currently gic_shutdown_irq:
- read GICD_ICENABLER
- set the corresping bit to 1
- write back the new value
That means, Xen will disable more IRQs than necessary.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
vgic_rank contains data which inform the guest if an IRQ is
enabled/actived/pending...
The structure must be zeroed otherwise the guest can retrieve wrong GIC state.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
When doing virtual VM entry and virtual VM exit, we need to
sychronize the PAE PDPTR related VMCS registers. With this fix,
we can boot 32bit PAE L2 guest (Win7 & RHEL6.4) on "Xen on Xen"
environment.
Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
Tested-by: Yongjie Ren <yongjie.ren@intel.com>
Acked-by: Keir Fraser <keir@xen.org>
Acked-by: "Dong, Eddie" <eddie.dong@intel.com>
XSA-36 changed the default vector map mode from global to per-device. This is
because a global vector map does not prevent one PCI device from impersonating
another and launching a DoS on the system.
However, the per-device vector map logic is broken for devices with multiple
MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
of a guests interrupt remapping tables. The core problem is not trivial to
fix.
In an effort to get AMD systems back to a non-regressed state, introduce a new
type of vector map called per-device-global. This uses per-device vector maps
in the IOMMU, but uses a single used_vector map for the core IRQ logic.
This patch is intended to be removed as soon as the per-device logic is fixed
correctly.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
This grub.cfg from a default fedora 19 Beta install
caused pygrub failures.The previous pygrub commit
fixed taht. So this example file added for reference.
Signed-off-by: Marcel Mol <marcel@mesa.nl>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Booting a fedora 19 domU failed because a it could not properly
parse the grub.cfg file. This was cased by
set default="${next_entry}"
This statement actually is within an 'if' statement, so maybe it would
be better to skip code within if/fi blocks...
But this patch seems to work fine.
Signed-off-by: Marcel Mol <marcel@mesa.nl>
Acked-by: Ian Campbell <ian.campbell@citix.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
The regex was not selecting the { when parsing JSON output of xl list -l.
It was also not selecting (domain when parsing xl list -l when SXP selected.
Pefixed { with 4 spaces, and removed an extra ( before domain in the regex
string
Added quotes around the grep strings so the spaces inserted into the string
didn't not break the grepping.
This has now been tested against 4.3RC5
Signed-off-by: Ian Murray <murrayie@yahoo.co.uk>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
XSA-55 has exposed errors for guest creation on ARM:
- domain virt_base was not defined;
- xc_dom_alloc_segment allocates pfn from 0 instead of the RAM base address.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Commit 5420f265 has some misplaced parenthesis that caused devid
to be assigned 1 or 0 based on checking return value of
libxl__device_nextid < 0, e.g.
devid = libxl__device_nextid(...) < 0
This works when only one instance of a given device type exists, but
subsequent devices of the same type will also have a devid = 1 if
libxl__device_nextid succeeds. Fix by checking the value assigned to
devid, e.g.
(devid = libxl__device_nextid(...)) < 0
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Printing them as decimal number, the more with 0x prefix, is confusing
and presumably relatively useless to most of us.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
In the original patch 7 of the series addressing XSA-45 I mistakenly
took the addition of the call to get_page_light() in alloc_page_type()
to cover two decrements that would happen: One for the PGT_partial bit
that is getting set along with the call, and the other for the page
reference the caller hold (and would be dropping on its error path).
But of course the additional page reference is tied to the PGT_partial
bit, and hence any caller of a function that may leave
->arch.old_guest_table non-NULL for error cleanup purposes has to make
sure a respective page reference gets retained.
Similar issues were then also spotted elsewhere: In effect all callers
of get_page_type_preemptible() need to deal with errors in similar
ways. To make sure error handling can work this way without leaking
page references, a respective assertion gets added to that function.
This is CVE-2013-1432 / XSA-58.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
When the CPU has the necessary capabilities, having Windows use
synthetic MSR reads/writes is bogus, as this still requires emulation
(which is pretty much guaranteed to be slower than having the hardware
carry out the operation).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Matthew Daley has observed that the PV console protocol places sensitive host
state into a guest writeable xenstore locations, this includes:
- The pty used to communicate between the console backend daemon and its
client, allowing the guest administrator to read and write arbitrary host
files.
- The output file, allowing the guest administrator to write arbitrary host
files or to target arbitrary qemu chardevs which include sockets, udp, ptr,
pipes etc (see -chardev in qemu(1) for a more complete list).
- The maximum buffer size, allowing the guest administrator to consume more
resources than the host administrator has configured.
- The backend to use (qemu vs xenconsoled), potentially allowing the guest
administrator to confuse host software.
So we arrange to make the sensitive keys in the xenstore frontend directory
read only for the guest. This is safe since the xenstore permissions model,
unlike POSIX directory permissions, does not allow the guest to remove and
recreate a node if it has write access to the containing directory.
There are a few associated wrinkles:
- The primary PV console is "special". It's xenstore node is not under the
usual /devices/ subtree and it does not use the customary xenstore state
machine protocol. Unfortunately its directory is used for other things,
including the vnc-port node, which we do not want the guest to be able to
write to. Rather than trying to track down all the possible secondary uses
of this directory just make it r/o to the guest. All newly created
subdirectories inherit these permissions and so are now safe by default.
- The other serial consoles do use the customary xenstore state machine and
therefore need write access to at least the "protocol" and "state" nodes,
however they may also want to use arbitrary "feature-foo" nodes (although
I'm not aware of any) and therefore we cannot simply lock down the entire
frontend directory. Instead we add support to libxl__device_generic_add for
frontend keys which are explicitly read only and use that to lock down the
sensitive keys.
- Minios' console frontend wants to write the "type" node, which it has no
business doing since this is a host/toolstack level decision. This fails
now that the node has become read only to the PV guest. Since the toolstack
already writes this node just remove the attempt to set it.
This is a security issue, XSA-57.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
At the moment, qemu-xen can't handle memory being relocated by
hvmloader. This may happen if a device with a large enough memory
region is passed through to the guest. At the moment, if this
happens, then at some point in the future qemu will crash and the
domain will hang. (qemu-traditional is fine.)
It's too late in the release to do a proper fix, so we try to do
damage control.
hvmloader already has mechanisms to relocate memory to 64-bit space if
it can't make a big enough MMIO hole. By default this is 2GiB; if we
just refuse to make the hole bigger if it will overlap with guest
memory, then the relocation will happen by default.
v5:
- Update comment to not refer to "this series".
v4:
- Wrap long line in libxl_dm.c
- Fix comment
v3:
- Fix polarity of comparison
- Move diagnostic messages to another patch
- Tested with xen platform pci device hacked to have different BAR sizes
{256MiB, 1GiB} x {qemu-xen, qemu-traditional} x various memory
configurations
- Add comment explaining why we default to "allow"
- Remove cast to bool
v2:
- style fixes
- fix and expand comment on the MMIO hole loop
- use "%d" rather than "%s" -> (...)?"1":"0"
- use bool instead of uint8_t
- Move 64-bit bar relocate detection to another patch
- Add more diagnostic messages
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
CC: Keir Fraser <keir@xen.org>
Allow devices with BARs less than 512MiB to be relocated to high
memory.
This will only be invoked if there is not enough low MMIO space to map
the device, and will be done preferentially to large devices first; so
in all likelihood only large devices will be remapped anyway.
This is needed to work-around the issue of qemu-xen not being able to
handle moving guest memory around to resize the MMIO hole. The
default MMIO hole size is less than 256MiB.
v3:
- Fixed minor style issue
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
Keep track of how much mmio space is left total, as well as the amount
of "low" MMIO space (<4GiB), and only load devices into high memory if
there is not enough low memory for the rest of the devices to fit.
Because devices are processed by size in order from large to small,
this should preferentially relocate devices with large BARs to 64-bit
space.
v3:
- Just use mmio_total rather than introducing a new variable.
- Port to using mem_resource directly rather than low_mmio_left
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
When deciding whether to map a device in low MMIO space (<4GiB),
hvmloader compares it with "mmio_left", which is set to the size of
the low MMIO range (pci_mem_end - pci_mem_start). However, even if it
does map a device in high MMIO space, it still removes the size of its
BAR from mmio_left.
In reality we don't need to do a separate accounting of the low memory
available -- this can be calculated from mem_resource. Just get rid
of the variable and the duplicate accounting entirely. This will make
the code more robust.
Note also that the calculation of whether to move a device to 64-bit
is fragile at the moment, depending on some unstated assumptions.
State those assumptions in a comment for future reference.
v5:
- Add comment documenting fragility of the move-to-highmem check
v3:
- Use mem_resource values directly instead of doing duplicate
accounting
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
After attempting to resize the MMIO hole, the check to determine
whether there is a need to relocate BARs into 64-bit space checks the
specific thing that caused the loop to exit (MMIO hole == 2GiB) rather
than checking whether the required MMIO will fit in the hole.
But even then it does it wrong: the polarity of the check is
backwards.
Check for the actual condition we care about (the sizeof the MMIO
hole) rather than checking for the loop exit condition.
v3:
- Move earlier in the series, before other functional changes
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
hvmloader will read hvm_info->high_mem_pgend to calculate where to
start the highmem PCI region. However, if the guest does not have any
memory in the high region, this is set to zero, which will cause
hvmloader to use the "0" for the base of the highmem region, rather
than 1 << 32.
Check to see whether hvm_info->high_mem_pgend is set; if so, do the
normal calculation; otherwise, use 1<<32.
v4:
- Handle case where hfm_info->high_mem_pgend is non-zero but doesn't
point into high memory, throwing a warning.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
The definitions here shouldn't use vtpm_ or VPTM_ as their prefixes,
the interface should instead make use of tpmif_ and TPMIF_. This
fixes a build failure after syncing the public headers to
linux-2.6.18-xen.hg (where a struct vtpm_state already exists).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Commit 899110e3 ("AMD IOMMU: include IOMMU interrupt information in 'M'
debug key output") made the AMD IOMMU MSI setup code use more of the
generic MSI setup code (as other than for VT-d this is an ordinary MSI-
capable PCI device), but failed to notice that till now interrupt setup
there _required_ the subsequent affinity setup to be done, as that was
the only point where the MSI message would get written. The generic MSI
affinity setting routine, however, does only an incremental change,
i.e. relies on this setup to have been done before.
In order to not make the code even more clumsy, introduce a new low
level helper routine __setup_msi_irq(), thus eliminating the need for
the AMD IOMMU code to directly fiddle with the IRQ descriptor.
Reported-by: Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
ed65808a8ed4 "libelf: check all pointer accesses" caused:
kernel.c: In function 'kernel_elf_load':
kernel.c:162:18: error: 'struct elf_binary' has no member named 'dest'
make[4]: *** [kernel.o] Error 1
The field is now called dest_base. We also need to populate dest_size.
This fixes the build for me although have not tested it. I have a feeling that
loading the kernel from an ELF file on ARM doesn't currently work anyway
(everyone uses the zImage loader as far as I am aware).
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
If seg->pfn is too large, the arithmetic in the range check might
overflow, defeating the range check.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
These functions take guest pfns and look them up in the p2m. They did
no range checking.
However, some callers, notably xc_dom_boot.c:setup_hypercall_page want
to pass untrusted guest-supplied value(s). It is most convenient to
detect this here and return INVALID_MFN.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Tim Deegan <tim@xen.org>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com>
v6: Check for underflow too (thanks to Andrew Cooper).
A sufficiently malformed input to libxc (such as a malformed input ELF
or other guest-controlled data) might cause one of libxc's malloc() to
fail. In this case we need to make sure we don't dereference or do
pointer arithmetic on the result.
Search for all occurrences of \b(m|c|re)alloc in libxc, and all
functions which call them, and add appropriate error checking where
missing.
This includes the functions xc_dom_malloc*, which now print a message
when they fail so that callers don't have to do so.
The function xc_cpuid_to_str wasn't provided with a sane return value
and has a pretty strange API, which now becomes a little stranger.
There are no in-tree callers.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
v8: Move a check in xc_exchange_page to the previous patch
(ie, remove it from this patch).
v7: Add a missing check for a call to alloc_str.
Add arithmetic overflow check in xc_dom_malloc.
Coding style fix.
v6: Fix a missed call `pfn_err = calloc...' in xc_domain_restore.c.
Fix a missed call `new_pfn = xc_map_foreign_range...' in
xc_offline_page.c
v5: This patch is new in this version of the series.
The return values from xc_dom_*_to_ptr and xc_map_foreign_range are
sometimes dereferenced, or subjected to pointer arithmetic, without
checking whether the relevant function failed and returned NULL.
Add an appropriate error check at every call site.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
v8: Add a missing check in xc_offline_page.c:xc_exchange_page,
which was in the next patch in v7 of the series.
Also improve the message.
I think in this particular error case it may be that the results
are a broken guest, but turning this from a possible host tools
crash into a guest problem seems to solve the potential security
problem.
v7: Simplify an error DOMPRINTF to not use "load ? : ".
Make DOMPRINTF allocation error messages consistent.
Do not set elf->dest_pages in xc_dom_load_elf_kernel
if xc_dom_seg_to_ptr_pages fails.
v5: This patch is new in this version of the series.
This is a simple binary image loader with its own metadata format.
However, it is too careless with image-supplied values.
Add the following checks:
* That the image is bigger than the metadata table; otherwise the
pointer arithmetic to calculate the metadata table location may
yield undefined and dangerous values.
* When clamping the end of the region to search, that we do not
calculate pointers beyond the end of the image. The C
specification does not permit this and compilers are becoming ever
more determined to miscompile code when they can "prove" various
falsehoods based on assertions from the C spec.
* That the supplied image is big enough for the text we are allegedly
copying from it. Otherwise we might have a read overrun and copy
the results (perhaps a lot of secret data) into the guest.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
v9: Use clearer code for calculating probe_end in find_table.
v6: Add a missing `return -EINVAL' (Matthew Daley).
Fix an error in the commit message (Matthew Daley).
v5: This patch is new in this version of the series.
Abolish ELF_PTRVAL_[CONST_]{CHAR,VOID}; change uses to elf_ptrval.
Abolish ELF_HANDLE_DECL_NONCONST; change uses to ELF_HANDLE_DECL.
Abolish ELF_OBSOLETE_VOIDP_CAST; simply remove all uses.
No functional change. (Verified by diffing assembler output.)
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
v2: New patch.
Ensure that libelf does not have any loops which can run away
indefinitely even if the input is bogus. (Grepped for \bfor, \bwhile
and \bgoto in libelf and xc_dom_*loader*.c.)
Changes needed:
* elf_note_next uses the note's unchecked alleged length, which might
wrap round. If it does, return ELF_MAX_PTRVAL (0xfff..fff) instead,
which will be beyond the end of the section and so terminate the
caller's loop. Also check that the returned psuedopointer is sane.
* In various loops over section and program headers, check that the
calculated header pointer is still within the image, and quit the
loop if it isn't.
* Some fixed limits to avoid potentially O(image_size^2) loops:
- maximum length of strings: 4K (longer ones ignored totally)
- maximum total number of ELF notes: 65536 (any more are ignored)
* Check that the total program contents (text, data) we copy or
initialise doesn't exceed twice the output image area size.
* Remove an entirely useless loop from elf_xen_parse (!)
* Replace a nested search loop in in xc_dom_load_elf_symtab in
xc_dom_elfloader.c by a precomputation of a bitmap of referenced
symtabs.
We have not changed loops which might, in principle, iterate over the
whole image - even if they might do so one byte at a time with a
nontrivial access check function in the middle.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
v8: Fix the two loops in libelf-dominfo.c; the comment about
PT_NOTE and SHT_NOTE wasn't true because the checks did
"continue", not "break".
Add a comment about elf_note_next's expectations of the caller's
loop conditions (which most plausible callers will follow anyway).
v5: Fix regression due to wrong image size loop limit calculation.
Check return value from xc_dom_malloc.
v4: Fix regression due to misplacement of test in elf_shdr_by_name
(uninitialised variable).
Introduce fixed limits.
Avoid O(size^2) loops.
Check returned psuedopointer from elf_note_next is correct.
A few style fixes.
v3: Fix a whitespace error.
v2: BUGFIX: elf_shdr_by_name, elf_note_next: Reject new <= old, not just <.
elf_shdr_by_name: Change order of checks to be a bit clearer.
elf_load_bsdsyms: shdr loop check, improve chance of brokenness detection.
Style fixes.