12.01.2019 20:57, 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 <ebl...@redhat.com>
> ---
> 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;
This reuse of file-size variable as export-size is a source of errors, like your
assert in the following part.
>
> if (partition != -1) {
> - ret = find_partition(blk, partition, &dev_offset, &fd_size);
> + off_t limit;
> +
> + if (dev_offset) {
> + error_report("Cannot request partition and offset together");
> + exit(EXIT_FAILURE);
> + }
> + ret = find_partition(blk, partition, &dev_offset, &limit);
> if (ret < 0) {
> error_report("Could not find partition %d: %s", partition,
> strerror(-ret));
> exit(EXIT_FAILURE);
> }
> + /* partition limits are (32-bit << 9); can't overflow 64 bits */
> + assert(dev_offset >= 0 && dev_offset + limit >= dev_offset);
> + if (dev_offset + limit > fd_size) {
> + error_report("Discovered partition %d at offset %lld size %lld, "
> + "but size exceeds file length %lld", partition,
> + (long long int) dev_offset, (long long int) limit,
> + (long long int) fd_size);
> + exit(EXIT_FAILURE);
> + }
> + fd_size = limit;
> }
>
> export = nbd_export_new(bs, dev_offset, fd_size, export_name,
>
Ok, anyway, finally I understand the point, thank you for detailed explanation:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
--
Best regards,
Vladimir
