Re: [Qemu-devel] [PATCH v3 03/19] qemu-nbd: Sanity check partition bounds

[Eric Blake]

On 1/15/19 12:00 PM, Richard W.M. Jones wrote:
> On Sat, Jan 12, 2019 at 11:57:56AM -0600, Eric Blake wrote:
>> When the user requests a partition, we were using data read
>> from the disk as disk offsets without a bounds check. We got
>> lucky that even when computed offsets are out-of-bounds,
>> blk_pread() will gracefully catch the error later (so I don't
>> think a malicious image can crash or exploit qemu-nbd, and am
>> not treating this as a security flaw), but it's better to
>> flag the problem up front than to risk permanent EIO death of
>> the block device down the road.  Also, note that the
>> partition code blindly overwrites any offset passed in by the
>> user; so make the -o/-P combo an error for less confusion.
>>
>> This can be tested with nbdkit:
>> $ echo hi > file
>> $ nbdkit -fv --filter=truncate partitioning file truncate=64k
>>
>> Pre-patch:
>> $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809 &
>> $ qemu-io -f raw nbd://localhost:10810
>> qemu-io> r -v 0 1
>> Disconnect client, due to: Failed to send reply: reading from file failed: 
>> Input/output error
>> Connection closed
>> read failed: Input/output error
>> qemu-io> q
>> [1]+  Done                    qemu-nbd -p 10810 -P 1 -f raw 
>> nbd://localhost:10809
>>
>> Post-patch:
>> $ qemu-nbd -p 10810 -P 1 -f raw nbd://localhost:10809
>> qemu-nbd: Discovered partition 1 at offset 1048576 size 512, but size 
>> exceeds file length 65536
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>> v3: new patch
>> ---
>>  qemu-nbd.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 51b55f2e066..ff4adb9b3eb 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -1013,12 +1013,28 @@ int main(int argc, char **argv)
>>      fd_size -= dev_offset;
>>
>>      if (partition != -1) {
>> -        ret = find_partition(blk, partition, &dev_offset, &fd_size);
>> +        off_t limit;
> 
> I was only vaguely following the other review comments, but off_t does
> seem odd here.  Even though we can assume that _FILE_OFFSET_BITS=64
> maybe we should just use {u,}int64_t?  Does this represent an offset
> in a host file?  Only in the case where qemu-nbd is serving a raw
> format file.  In other cases (say, qcow2) the partition size exists in
> an abstract virtual space.

Yes, later patches switch it to int64_t.  Here, it remains off_t because
find_partition(&limit) still expects off_t.  I suppose in my later
patches, I could use uint64_t limit in spite of keeping int64_t fd_size,
and change the signature of find_partition() accordingly, since I've
decoupled the MBR partition lookup (which is a 41-bit value, always
positive) from the file size checks.

> 
> But it's not a big deal, so:

Yeah, no need to rewrite this patch, since later patches improve the
type anyway (whether or not I stick to int64_t or uint64_t for the
find_partition() code).

> 
> Reviewed-by: Richard W.M. Jones <address@hidden>
> 
> Rich.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature