[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 03/19] qemu-nbd: Sanity check partition bound
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 03/19] qemu-nbd: Sanity check partition bounds |
Date: |
Tue, 15 Jan 2019 12:08:25 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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
- [Qemu-devel] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t, (continued)
[Qemu-devel] [PATCH v3 03/19] qemu-nbd: Sanity check partition bounds, Eric Blake, 2019/01/12
[Qemu-devel] [PATCH v3 14/19] nbd/client: Pull out oldstyle size determination, Eric Blake, 2019/01/12
[Qemu-devel] [PATCH v3 08/19] nbd/client: Move export name into NBDExportInfo, Eric Blake, 2019/01/12
[Qemu-devel] [PATCH v3 12/19] nbd/client: Refactor return of nbd_receive_negotiate(), Eric Blake, 2019/01/12
[Qemu-devel] [PATCH v3 11/19] nbd/client: Split out nbd_receive_one_meta_context(), Eric Blake, 2019/01/12