When a block device read or write request is made by an HVM guest, nothing checks that the request is within the range supported by the block backend driver in ioemu, but the code in the backend driver typically assumes that the request is sensible. Depending on the backend, this can allow the guest to read and write arbitrary memory locations in qemu, and possibly gain control over the qemu process, escaping from the virtualisation. I have demonstrated to my own satisfaction that there is problem, using a modified Linux kernel as a guest with an instrumented CVS head qemu. I haven''t yet reproduced the bug with xen-unstable but I think it''s almost certainly there too. I have prepared a patch which I have checked prevents my test case, and adjusted it to fit and compile against xen-unstable. I''m subjecting it to some testing as I write. I contacted privately five of the main qemu developers but the only response was from Andrzej Zaborowski who didn''t consider the problem serious, saying Qemu never claimed to be secure. Of course it''s better to be secure than not if it doesn''t add a bad overhead. and suggesting that I should just post to the qemu-devel mailing list. Ian. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel P. Berrange
2008-Feb-26 20:41 UTC
Re: [Xen-devel] [PATCH] ioemu block device extent checks
On Tue, Feb 19, 2008 at 04:38:24PM +0000, Ian Jackson wrote: Content-Description: message body text> When a block device read or write request is made by an HVM guest, > nothing checks that the request is within the range supported by the > block backend driver in ioemu, but the code in the backend driver > typically assumes that the request is sensible. > > Depending on the backend, this can allow the guest to read and write > arbitrary memory locations in qemu, and possibly gain control over the > qemu process, escaping from the virtualisation. > > > I have demonstrated to my own satisfaction that there is problem, > using a modified Linux kernel as a guest with an instrumented CVS head > qemu. I haven''t yet reproduced the bug with xen-unstable but I think > it''s almost certainly there too. I have prepared a patch which I have > checked prevents my test case, and adjusted it to fit and compile > against xen-unstable. I''m subjecting it to some testing as I write.FYI, this patch causes massive unrecoverable data loss / corruption on QCow2 files. The checks themselves are OK in terms of the first level of bdrv_* calls from the guest. The qcow driver though calls back into the raw driver for performing I/O on its underlying file. The qcow driver relies on this file being grow-on-demand for purposes of allocating new qcow sectors. The safety checks cause this allocation to fail and it all goes downhill from there :-( Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=| _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2008-Feb-27 11:28 UTC
Re: [Xen-devel] [PATCH] ioemu block device extent checks
Daniel P. Berrange writes ("Re: [Xen-devel] [PATCH] ioemu block device
extent checks"):> The qcow driver though calls back into
> the raw driver for performing I/O on its underlying file. The qcow
> driver relies on this file being grow-on-demand for purposes of allocating
> new qcow sectors. The safety checks cause this allocation to fail and
> it all goes downhill from there :-(
Oh dear. (I''m a bit surprised that it''s taken this long to
spot!)
Here is a patch for xen-unstable which I think will fix it. Could you
give it a quick spin, if you have a suitable test setup ?
Sadly it''s rather more intrusive than ideal, since it needs all of the
drivers which are going to extend files via their parents to announce
this, and a couple of bits of necessary infrastructure needed adding.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Daniel P. Berrange
2008-Feb-27 12:57 UTC
Re: [Xen-devel] [PATCH] ioemu block device extent checks
On Wed, Feb 27, 2008 at 11:28:05AM +0000, Ian Jackson wrote: Content-Description: message body text> Daniel P. Berrange writes ("Re: [Xen-devel] [PATCH] ioemu block device extent checks"): > > The qcow driver though calls back into > > the raw driver for performing I/O on its underlying file. The qcow > > driver relies on this file being grow-on-demand for purposes of allocating > > new qcow sectors. The safety checks cause this allocation to fail and > > it all goes downhill from there :-( > > Oh dear. (I''m a bit surprised that it''s taken this long to spot!) > Here is a patch for xen-unstable which I think will fix it. Could you > give it a quick spin, if you have a suitable test setup ? > > Sadly it''s rather more intrusive than ideal, since it needs all of the > drivers which are going to extend files via their parents to announce > this, and a couple of bits of necessary infrastructure needed adding.I don''t think this is correct - it allows a -ve size / nb_sectors value when autoextenable is set, and allows out of bounds reads. I sent a patch to qemu-devel yuesterday which also uses the auto-extend flag, but has separate checks for read vs writes. When doing a write that would extend the device it increases the total_sectors count so that the subsequent reads can be validated to be within the written bounds. http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00497.html Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=| _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2008-Feb-27 13:14 UTC
Re: [Xen-devel] [PATCH] ioemu block device extent checks
Daniel P. Berrange writes ("Re: [Xen-devel] [PATCH] ioemu block device
extent checks"):> I don''t think this is correct - it allows a -ve size / nb_sectors
> value when autoextenable is set, and allows out of bounds reads.
That''s fine because it''s only called like that as a the parent
block
driver. Perhaps a different name would have been better.
Out of bounds reads have to be permitted because the block size in the
header may remain un-updated, so reads of newly-cloned blocks may
fail.
> I sent a patch to qemu-devel yuesterday which also uses the auto-extend
> flag, but has separate checks for read vs writes. When doing a write that
> would extend the device it increases the total_sectors count so that the
> subsequent reads can be validated to be within the written bounds.
Well, that seems like makework to me but fine if upstream accept it.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Daniel P. Berrange
2008-Feb-27 13:21 UTC
Re: [Xen-devel] [PATCH] ioemu block device extent checks
On Wed, Feb 27, 2008 at 01:14:16PM +0000, Ian Jackson wrote:> Daniel P. Berrange writes ("Re: [Xen-devel] [PATCH] ioemu block device extent checks"): > > I don''t think this is correct - it allows a -ve size / nb_sectors > > value when autoextenable is set, and allows out of bounds reads. > > That''s fine because it''s only called like that as a the parent block > driver. Perhaps a different name would have been better. > > Out of bounds reads have to be permitted because the block size in the > header may remain un-updated, so reads of newly-cloned blocks may > fail.Which is why I updated the the total_sectors count during writes...> > > I sent a patch to qemu-devel yuesterday which also uses the auto-extend > > flag, but has separate checks for read vs writes. When doing a write that > > would extend the device it increases the total_sectors count so that the > > subsequent reads can be validated to be within the written bounds. > > Well, that seems like makework to me but fine if upstream accept it.It could help prevent future bugs if some new code is written which is expecting the total_sectors value to be correct - admitedly the existing code has incorrect total_sectors count already, but I figure its worth fixing this to avoid unexpected surprises in the future. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=| _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Mar-04 10:08 UTC
Re: [Xen-devel] [PATCH] ioemu block device extent checks
On 4/3/08 10:16, "Kevin Wolf" <kwolf@suse.de> wrote:> Daniel P. Berrange schrieb: >> FYI, this patch causes massive unrecoverable data loss / corruption on >> QCow2 files. The checks themselves are OK in terms of the first level >> of bdrv_* calls from the guest. The qcow driver though calls back into >> the raw driver for performing I/O on its underlying file. The qcow >> driver relies on this file being grow-on-demand for purposes of allocating >> new qcow sectors. The safety checks cause this allocation to fail and >> it all goes downhill from there :-( > > I just hit this problem. What''s the status? If I understand correctly, > you discussed two different solutions and as a result no fix at all got > committed.Current xen-unstable tip should have it fixed (by a combination of changesets 17133 and 17177). -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel P. Berrange schrieb:> FYI, this patch causes massive unrecoverable data loss / corruption on > QCow2 files. The checks themselves are OK in terms of the first level > of bdrv_* calls from the guest. The qcow driver though calls back into > the raw driver for performing I/O on its underlying file. The qcow > driver relies on this file being grow-on-demand for purposes of allocating > new qcow sectors. The safety checks cause this allocation to fail and > it all goes downhill from there :-(I just hit this problem. What''s the status? If I understand correctly, you discussed two different solutions and as a result no fix at all got committed. Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser schrieb:> On 4/3/08 10:16, "Kevin Wolf" <kwolf@suse.de> wrote: >> I just hit this problem. What''s the status? If I understand correctly, >> you discussed two different solutions and as a result no fix at all got >> committed. > > Current xen-unstable tip should have it fixed (by a combination of > changesets 17133 and 17177). > > -- KeirOh, right... Sorry for the noise. Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel