Avoid freeing info then returning it to the caller.
This is XSA-88.
Coverity-ID: 1056192
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(cherry picked from commit d883c179a7)
The remote (hostile) process can set ring buffer indices to any value
at any time. If that happens, it is possible to get "buffer space"
(either for writing data, or ready for reading) negative or greater
than buffer size. This will end up with buffer overflow in the second
memcpy inside of do_send/do_recv.
Fix this by introducing new available bytes accessor functions
raw_get_data_ready and raw_get_buffer_space which are robust against
mad ring states, and only return sanitised values.
Proof sketch of correctness:
Now {rd,wr}_{cons,prod} are only ever used in the raw available bytes
functions, and in do_send and do_recv.
The raw available bytes functions do unsigned arithmetic on the
returned values. If the result is "negative" or too big it will be
>ring_size (since we used unsigned arithmetic). Otherwise the result
is a positive in-range value representing a reasonable ring state, in
which case we can safely convert it to int (as the rest of the code
expects).
do_send and do_recv immediately mask the ring index value with the
ring size. The result is always going to be plausible. If the ring
state has become mad, the worst case is that our behaviour is
inconsistent with the peer's ring pointer. I.e. we read or write to
arguably-incorrect parts of the ring - but always parts of the ring.
And of course if a peer misoperates the ring they can achieve this
effect anyway.
So the security problem is fixed.
This is XSA-86.
(The patch is essentially Ian Jackson's work, although parts of the
commit message are by Marek.)
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
master commit: 2efcb0193b
master date: 2014-02-06 16:44:41 +0100
If the console or store mfn chunks are not present in the migration stream,
stack junk gets reported for the mfns.
XenServer had a very hard to track down VM corruption issue caused by exactly
this issue. Xenconsoled would connect to a junk mfn and incremented the ring
pointer if the junk happend to look like a valid gfn.
Coverity ID: 1056093 1056094
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
(cherry picked from commit 592b614f34)
The current behavior is:
42sh> xl uptime
'xl uptime' requires at least 1 argument.
Usage: xl [-v] uptime [-s] [Domain]
The normal behavior should list uptime for each domain when there is no
parameters.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
(cherry picked from commit 3533972f6d)
This is for the client-side receiving messages from xenstored, so there
is no security impact, unlike XSA-72.
Coverity-ID: 1055449
Coverity-ID: 1056028
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
(cherry picked from commit 8da1ed9031)
If initializing the ctx's lock fails, don't keep going, but instead
error out.
Coverity-ID: 1055289
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
(cherry picked from commit b1cb2bdde1)
Checking the logfile_w fd for -1 on failure is no longer true, because
libxl__create_qemu_logfile will now return ERROR_FAIL on failure which
is -3.
While there also add an error check for opening /dev/null.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 3b88d95e9c)
Conflicts:
tools/libxl/libxl_dm.c
Coverity ID: 1055290
Calling LIBXL__LOG_ERRNO(ctx,) with a ctx pointer we have just failed to
allocate is going to end badly. Opencode a suitable use of xtl_log() instead.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
(cherry picked from commit 1677af03c1)
Coverity ID 1055563
In the original function, mmap() could be called with a length of -1 if the
second lseek failed and the caller had not provided max_size.
While fixing up this error, improve the logging of other error paths. I know
from personal experience that debugging failures function is rather difficult
given only "xc_dom_malloc_filemap: failed (on file <somefile>)" in the logs.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
(cherry picked from commit c635c1ef78)
The ptr calculation shall take the offset into the page into account
when ptr is valid.
Reported regression on NetBSD's port-xen with last known working libxen
being rev 2.9. This corrupts the kernel symbol table when the table is
not loaded on a page boundary.
Issue was tracked down by FastIce and Jeff Rizzo. See also
http://mail-index.netbsd.org/port-xen/2013/10/16/msg008088.html
Signed-off-by: Jean-Yves Migeon <jym@NetBSD.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
master commit: cb08944a48
master date: 2013-11-29 11:07:01 +0000
If the named path is a leaf node, libxl__xs_directory can succeed,
returning non-null, but set *nb to 0.
In three places in libxl this may result in a zero size argument being
passed to malloc() or realloc(), which is not adviseable.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(cherry picked from commit 48249a1403)
libxl__datacopier_prefixdata would prepend a deliberately short block
(not just a half-full one, but one with a short buffer) to the
dc->bufs queue. However, this is wrong because datacopier_readable
will find it and try to continue to fill it up.
Instead, allocate a full-sized buffer.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Tested-by: Chunyan Liu <cyliu@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(cherry picked from commit f6d2a87f05)
Without this, code interrupted by SIGCHLD may experience strange
values of errno. (As far as I know this is not the cause of any
reported bugs.)
This fix should be backported in due course.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(cherry picked from commit 704d10289b)
Commit 00a4b65f85 Sep 7 2010
"libxc: provide notification of final checkpoint to restore end"
broke migration from any version of Xen using tools from prior to that commit
Older tools have no idea about an XC_SAVE_ID_LAST_CHECKPOINT, causing newer
tools xc_domain_restore() to start reading the qemu save record, as
ctx->last_checkpoint is 0.
The failure looks like:
xc: error: Max batch size exceeded (1970103633). Giving up.
where 1970103633 = 0x756d6551 = *(uint32_t*)"Qemu"
With this fix in place, the behaviour for normal migrations is reverted to how
it was before the regression; the migration is considered non-checkpointed
right from the start. A XC_SAVE_ID_LAST_CHECKPOINT chunk seen in the
migration stream is a nop. For checkpointed migrations the behaviour is
unchanged.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca> (Remus bits)
master commit: 7051d5c872
master date: Thu, 10 Oct 2013 11:28:21 +0000
Backport to Xen-4.3:
As remus was not a supported feature in libxl in 4.3 and earlier, this
backport hardcodes a non-checkpointed stream from libxl, removing the need to
any playing around with the IDL and API.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
This fixes the issue for both C and ocaml xenstored, however only the ocaml
xenstored is vulnerable in its default configuration.
Adding a new error appears to be safe, since bit libxenstore and the Linux
driver at least treat an unknown error code as EINVAL.
This is XSA-72 / CVE-2013-4416.
Original ocaml patch by Jerome Maloberti <jerome.maloberti@citrix.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Thomas Sanders <thomas.sanders@citrix.com>
(cherry picked from commit 8b2c441a1b)
since it errors out, asking for at least one argument, and does
not display any useful output, which is wrong (we want the list
and the info about all the existing cpupools).
IOW, the output is as follows:
~# xl cpupool-list -c
'xl cpupool-list' requires at least 1 argument.
...
While it should be as follows:
~# xl cpupool-list -c
Name CPU list
Pool-0 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: Juergen Gross <juergen.gross@ts.fujitsu.com>
(cherry picked from commit 3998afdbf9)
...otherwise it will return freed memory. All the current users of this
function check already for a NULL return, so use that.
Coverity-ID: 1056194
This is CVE-2013-4371 / XSA-70
Signed-off-by: Matthew Daley <mattjd@gmail.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(cherry picked from commit 4c37ed5622)
Not sure how it got there...
Coverity-ID: 1056196
This is CVE-2013-4370 / XSA-69
Signed-off-by: Matthew Daley <mattjd@gmail.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(cherry picked from commit 3cd10fd212)
strtok can return NULL here. We don't need to use strtok anyway, so just
use a simple strchr method.
Coverity-ID: 1055642
This is CVE-2013-4369 / XSA-68
Signed-off-by: Matthew Daley <mattjd@gmail.com>
Fix type. Add test case
Signed-off-by: Ian Campbell <Ian.campbell@citrix.com>
(cherry picked from commit c53702cee1)
The switch-over logic from one page directory to the next was wrong;
it needs to be deferred until we actually reach the last page within
a given region, instead of being done when the last entry of a page
directory gets started with.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
master commit: 06d0868321
master date: 2013-09-12 17:41:04 +0200
The return value of vasprintf must be checked. This check is enforced
with the compiler options used in Debian by request and in Ubuntu by
default.
Check the return value and abort on error.
Signed-off-by: Bastian Blank <waldi@debian.org>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
(cherry picked from commit 1efe90faa3)
The SeaBIOS ROM image may validly exceed 128k in size, it's only our
interface code that so far assumed that it wouldn't. Remove that
restriction by setting the base address depending on image size.
Add a check to HVM loader so that too big images won't result in silent
guest failure anymore.
Uncomment the intended build-time size check for rombios, moving it
into a function so that it would actually compile.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
master commit: 5f2875739b
master date: 2013-09-05 11:47:03 +0200
A file descriptor leak was detected after creating multiple domUs with
pass-through PCI devices. This patch fixes the issue.
Signed-off-by: Xi Xiong <xixiong@amazon.com>
Reviewed-by: Matt Wilson <msw@amazon.com>
[msw: adjusted commit message]
Signed-off-by: Matt Wilson <msw@amazon.com>
master commit: 749019afca
master date: 2013-09-03 16:36:52 +0100
Newer PCI standards (e.g., PCI-X 2.0 and PCIe) introduce extended
configuration space which is larger than 256 bytes. This patch uses
stat() to determine the amount of space used to correctly save all of
the PCI configuration space. Resets handled by the xen-pciback driver
don't have this problem, as that code correctly handles saving
extended configuration space.
Signed-off-by: Steven Noonan <snoonan@amazon.com>
Reviewed-by: Matt Wilson <msw@amazon.com>
[msw: adjusted commit message]
Signed-off-by: Matt Wilson <msw@amazon.com>
master commit: 1893cf7799
master date: 2013-09-03 16:36:47 +0100
Fixes regression indirectly introduced by c/s 4d23036e70
That changeset added some smbios tables which were option based on the
toolstack providing appropriate xenstore keys. The do_struct() macro would
unconditionally increment nr_structs, even if a table was not actually
written.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Keir Fraser <keir@xen.org>
master commit: 4aa19549e1
master date: 2013-08-30 10:40:29 +0200
- we used the C4/C5 (first prefix) byte instead of the apparent ModR/M
one as the second prefix byte
- early decoding normalized vex.reg, thus corrupting it for the main
consumer (copy_REX_VEX()), resulting in #UD on the two-operand
instructions we emulate
Also add respective test cases to the testing utility plus
- fix get_fpu() (the fall-through order was inverted)
- add cpu_has_avx2, even if it's currently unused (as in the new test
cases I decided to refrain from using AVX2 instructions in order to
be able to actually run all the tests on the hardware I have)
- slightly tweak cpu_has_avx to more consistently express the outputs
we don't care about (sinking them all into the same variable)
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
master commit: 062919448e
master date: 2013-08-28 17:03:50 +0200
add check logic when read from IO ring, and if error happens,
then mark the reading connection as "bad", Unless vm reboot,
oxenstored will not handle message from this connection any more.
xs_ring_stubs.c: add a more strict check on ring reading
connection.ml, domain.ml: add getter and setter for bad flag
process.ml: if exception raised when reading from domain's ring,
mark this domain as "bad"
xenstored.ml: if a domain is marked as "bad", do not handle it.
Signed-off-by: John Liu <john.liuqiming@huawei.com>
Acked-by: David Scott <dave.scott@eu.citrix.com>
(cherry picked from commit 704302ce94)
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>
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>
Andrew Cooper
Marek Marczykowski-Górecki
Julien Grall
Matthew Daley
Roger Pau Monne
Jean-Yves Migeon
Ian Jackson
Dario Faggioli
Matthew Daley
Jan Beulich
Bastian Blank
George Dunlap
Xi Xiong
Steven Noonan
Ian Campbell
John Liu
Vincent Bernardoff
Frediano Ziglio
Marcel J.E. Mol
Ian Murray
Anthony PERARD
Ian Campbell
Jim Fehlig