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

[Anthony Liguori <aliguori@us.ibm.com>]

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

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