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

[Vladimir Sementsov-Ogievskiy]

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 <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;
> +
> +        if (dev_offset) {
> +            error_report("Cannot request partition and offset together");

hmm, but you still allow to request partition and set -o 0, I think it's better
to forbid it too.

> +            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);

hmm, so these values are read from file and may be whatsoever. Why to assert 
instead
of error_report and exit() ?

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


-- 
Best regards,
Vladimir