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

[Richard W.M. Jones]

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.

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

But it's not a big deal, so:

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

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v