[Qemu-devel] Re: [PATCH][RFC] Fix CVE-2008-0928 - insufficient block device address range checking

[Eduardo Habkost <ehabkost@raisama.net>]

On Fri, Feb 27, 2009 at 10:20:23AM -0600, Anthony Liguori wrote:
> Introduce a growable flag that's set by bdrv_file_open().  Block devices should
> never be growable, only files that are being used by block devices.
> 
> I went through Fabrice's early comments about the patch that was first applied.
> While I disagree with that patch, I also disagree with Fabrice's suggestion.
> 
> There's no good reason to do the checks in the block drivers themselves.  It
> just increases the possibility that this bug could show up again.  Since we're
> calling bdrv_getlength() to determine the length, we're giving the block drivers
> a chance to chime in and let us know what range is valid.

Agreed.

> 
> Basically, this patch makes the BlockDriver API guarantee that all requests are
> within 0..bdrv_getlength() which to me seems like a Good Thing.
> 
> What do others think?
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/block.c b/block.c
> index 4f4bf7c..ab88d05 100644
> --- a/block.c
> +++ b/block.c
> @@ -318,6 +318,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
>          bdrv_delete(bs);
>          return ret;
>      }
> +    bs->growable = 1;

Is this really safe on all places where bdrv_file_open() is called? The
original patch has added a BDRV_O_AUTOGROW and it was used on most
bdrv_file_open() calls, but not on block-vpc.c.


>      *pbs = bs;
>      return 0;
>  }
> @@ -519,6 +520,39 @@ int bdrv_commit(BlockDriverState *bs)
>      return 0;
>  }
>  
> +static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
> +                                   size_t size)
> +{
> +    int64_t len;
> +
> +    if (!bdrv_is_inserted(bs))
> +        return -ENOMEDIUM;
> +
> +    if (bs->growable)
> +        return 0;

The original fix didn't allow out-of-bound read requests even on growable
devices. But I think the above is safe: raw_pread() should return an
error on out-of-bound read requests.


> +
> +    len = bdrv_getlength(bs);

Cool, this helps solving two problems with the original approach:

- The block-based vs. byte-based range checking
  (https://bugzilla.redhat.com/show_bug.cgi?id=485148)
- Removable devices where we can't be sure the media hasn't changed
  since the last time we checked the length

The only thing that I am worried about is the performance impact
of calling raw_getlength() on every request for raw devices such as
CD-ROMs. I hope it won't trigger some expensive operation on the physical
device every time we ask for the media size.

> +
> +    if ((offset + size) > len)
> +        return -EIO;
> +
> +    return 0;
> +}
> +
> +static int bdrv_check_request(BlockDriverState *bs, int64_t sector_num,
> +                              int nb_sectors)
> +{
> +    int64_t offset;
> +
> +    /* Deal with byte accesses */
> +    if (sector_num < 0)
> +        offset = -sector_num;

Uh? Where is this feature used?


> +    else
> +        offset = sector_num * 512;
> +
> +    return bdrv_check_byte_request(bs, offset, nb_sectors * 512);
> +}
> +
>  /* return < 0 if error. See bdrv_write() for the return codes */
>  int bdrv_read(BlockDriverState *bs, int64_t sector_num,
>                uint8_t *buf, int nb_sectors)
> @@ -527,6 +561,8 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num,
>  
>      if (!drv)
>          return -ENOMEDIUM;
> +    if (bdrv_check_request(bs, sector_num, nb_sectors))
> +        return -EIO;
>  
>      if (drv->bdrv_pread) {
>          int ret, len;
> @@ -560,6 +596,9 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>          return -ENOMEDIUM;
>      if (bs->read_only)
>          return -EACCES;
> +    if (bdrv_check_request(bs, sector_num, nb_sectors))
> +        return -EIO;
> +
>      if (drv->bdrv_pwrite) {
>          int ret, len, count = 0;
>          len = nb_sectors * 512;
> @@ -681,6 +720,9 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
>  
>      if (!drv)
>          return -ENOMEDIUM;
> +    if (bdrv_check_byte_request(bs, offset, count1))
> +        return -EIO;
> +
>      if (!drv->bdrv_pread)
>          return bdrv_pread_em(bs, offset, buf1, count1);
>      return drv->bdrv_pread(bs, offset, buf1, count1);
> @@ -696,6 +738,9 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
>  
>      if (!drv)
>          return -ENOMEDIUM;
> +    if (bdrv_check_byte_request(bs, offset, count1))
> +        return -EIO;
> +
>      if (!drv->bdrv_pwrite)
>          return bdrv_pwrite_em(bs, offset, buf1, count1);
>      return drv->bdrv_pwrite(bs, offset, buf1, count1);
> @@ -1299,6 +1344,9 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
>                                   QEMUIOVector *iov, int nb_sectors,
>                                   BlockDriverCompletionFunc *cb, void *opaque)
>  {
> +    if (bdrv_check_request(bs, sector_num, nb_sectors))
> +        return NULL;
> +
>      return bdrv_aio_rw_vector(bs, sector_num, iov, nb_sectors,
>                                cb, opaque, 0);
>  }
> @@ -1307,6 +1355,9 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>                                    QEMUIOVector *iov, int nb_sectors,
>                                    BlockDriverCompletionFunc *cb, void *opaque)
>  {
> +    if (bdrv_check_request(bs, sector_num, nb_sectors))
> +        return NULL;
> +
>      return bdrv_aio_rw_vector(bs, sector_num, iov, nb_sectors,
>                                cb, opaque, 1);
>  }
> @@ -1320,6 +1371,8 @@ BlockDriverAIOCB *bdrv_aio_read(BlockDriverState *bs, int64_t sector_num,
>  
>      if (!drv)
>          return NULL;
> +    if (bdrv_check_request(bs, sector_num, nb_sectors))
> +        return NULL;
>  
>      ret = drv->bdrv_aio_read(bs, sector_num, buf, nb_sectors, cb, opaque);
>  
> @@ -1343,6 +1396,8 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDriverState *bs, int64_t sector_num,
>          return NULL;
>      if (bs->read_only)
>          return NULL;
> +    if (bdrv_check_request(bs, sector_num, nb_sectors))
> +        return NULL;
>  
>      ret = drv->bdrv_aio_write(bs, sector_num, buf, nb_sectors, cb, opaque);
>  
> diff --git a/block_int.h b/block_int.h
> index 781789c..e1943aa 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -121,6 +121,9 @@ struct BlockDriverState {
>      uint64_t rd_ops;
>      uint64_t wr_ops;
>  
> +    /* Whether the disk can expand beyond total_sectors */
> +    int growable;
> +
>      /* NOTE: the following infos are only hints for real hardware
>         drivers. They are not used by the block driver */
>      int cyls, heads, secs, translation;
> 
> 

-- 
Eduardo