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

[Anthony Liguori]

Eduardo Habkost wrote:
> > 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 <address@hidden>
> >
> > 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.
>   

Yes. This is what bdrv_file_open() means as opposed to bdrv_open().

FWIW, I didn't write the original patch, but my guess is that 
block-vpc.c write support is relatively new. Before write support was 
added (less than a month ago), there was no reason to allow a VPC to 
grow. But this definitely demonstrates a problem with the old patch. It 
would have broken block-vpc write support.

> 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.
>   

An out of bound read request will return an error, so, yeah, I'm 
perfectly happy with it.

> > +
> > +    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 it does, then we'll fix that properly. bdrv_getlength() needs to be 
fast to be useful.

> > +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?
>   

SCSI generic pass through. Avi has patches to eliminate this an 
introduce a new API but they haven't been merged yet.

Regards,

Anthony Liguori