[Qemu-devel] Re: [PATCH] Make default invocation of block drivers safer

[Kevin Wolf <kwolf@redhat.com>]

Am 14.07.2010 18:12, schrieb Anthony Liguori:
> CVE-2008-2004 described a vulnerability in QEMU whereas a malicious user could
> trick the block probing code into accessing arbitrary files in a guest.  To
> mitigate this, we added an explicit format parameter to -drive which disabling
> block probing.
> 
> Fast forward to today, and the vast majority of users do not use this parameter.
> libvirt does not use this by default nor does virt-manager.
> 
> Most users want block probing so we should try to make it safer.
> 
> This patch adds some logic to the raw device which attempts to detect a write
> operation to the beginning of a raw device.  If the first 4 bytes happen to
> match an image file that has a backing file that we support, it scrubs the
> signature to all zeros.  If a user specifies an explicit format parameter, this
> behavior is disabled.
> 
> I contend that while a legitimate guest could write such a signature to the
> header, we would behave incorrectly anyway upon the next invocation of QEMU.
> This simply changes the incorrect behavior to not involve a security
> vulnerability.
> 
> I've tested this pretty extensively both in the positive and negative case.  I'm
> not 100% confident in the block layer's ability to deal with zero sized writes
> particularly with respect to the aio functions so some additional eyes would be
> appreciated.
> 
> Even in the case of a single sector write, we have to make sure to invoked the
> completion from a bottom half so just removing the zero sized write is not an
> option.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

I guess something like this makes sense, and the approach looks okay in
general. With the check that we have really probed the format, we still
allow legitimate use cases (whatever they might be).

However, I wonder why you even bother with adjusting buffers and
requests and stuff instead of just returning a straight -EIO. Doing so
would have the additional advantage that the expectation of the guest OS
matches what is really on the disk (garbage) instead of silently
corrupting things.

>  static BlockDriverAIOCB *raw_aio_writev(BlockDriverState *bs,
>      int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
>      BlockDriverCompletionFunc *cb, void *opaque)
>  {
> +    if (check_write_unsafe(bs, sector_num, qiov->iov[0].iov_base, nb_sectors)) {

Have you checked that the bad value is always in iov[0]? Could a guest
construct a zero-length iov[0] and do the bad access in iov[1]? Or use
two two-byte buffers to write the magic number?

I'm not saying that any of these work, I honestly don't know, but did
you consider them?

Kevin