Re: [PATCH v4 07/16] block/io: improve bdrv_check_request: check qiov too

[Eric Blake]

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> Operations with qiov add more restrictions on bytes, let's cover it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 46 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 4a057660f8..42e687a388 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -898,8 +898,14 @@ static bool coroutine_fn 
> bdrv_wait_serialising_requests(BdrvTrackedRequest *self
>      return waited;
>  }
>  
> -int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
> +static int bdrv_check_qiov_request(int64_t offset, int64_t bytes,
> +                                   QEMUIOVector *qiov, size_t qiov_offset,
> +                                   Error **errp)
>  {
> +    /*
> +     * Check generic offset/bytes correctness
> +     */
> +
>      if (offset < 0) {
>          error_setg(errp, "offset is negative: %" PRIi64, offset);
>          return -EIO;
> @@ -929,12 +935,38 @@ int bdrv_check_request(int64_t offset, int64_t bytes, 
> Error **errp)
>          return -EIO;
>      }
>  
> +    if (!qiov) {
> +        return 0;
> +    }

I guess this short circuit is for write zeroes...

> +
> +    /*
> +     * Check qiov and qiov_offset
> +     */
> +
> +    if (qiov_offset > qiov->size) {
> +        error_setg(errp, "qiov_offset(%zu) overflow io vector size(%zu)",
> +                   qiov_offset, qiov->size);
> +        return -EIO;
> +    }
> +
> +    if (bytes > qiov->size - qiov_offset) {
> +        error_setg(errp, "bytes(%" PRIi64 ") + qiov_offset(%zu) overflow io "
> +                   "vector size(%zu)", bytes, qiov_offset, qiov->size);
> +        return -EIO;
> +    }

Yes, worthwhile additions.

> @@ -3135,7 +3167,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>      if (!dst || !dst->bs || !bdrv_is_inserted(dst->bs)) {
>          return -ENOMEDIUM;
>      }
> -    ret = bdrv_check_request32(dst_offset, bytes);
> +    ret = bdrv_check_request32(dst_offset, bytes, NULL, 0);

...ah, it's also for copy_range; basically any caller that isn't using a
qiov and therefore can't overflow qiov bounds.

Reviewed-by: Eric Blake <eblake@redhat.com>

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