Re: [PATCH] block: Fix pad_request's request restriction

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2909 bytes --]

On Fri, Jun 09, 2023 at 10:33:16AM +0200, Hanna Czenczek wrote:
> bdrv_pad_request() relies on requests' lengths not to exceed SIZE_MAX,
> which bdrv_check_qiov_request() does not guarantee.
> 
> bdrv_check_request32() however will guarantee this, and both of
> bdrv_pad_request()'s callers (bdrv_co_preadv_part() and
> bdrv_co_pwritev_part()) already run it before calling
> bdrv_pad_request().  Therefore, bdrv_pad_request() can safely call
> bdrv_check_request32() without expecting error, too.
> 
> There is one difference between bdrv_check_qiov_request() and
> bdrv_check_request32(): The former takes an errp, the latter does not,
> so we can no longer just pass &error_abort.  Instead, we need to check
> the returned value.  While we do expect success (because the callers
> have already run this function), an assert(ret == 0) is not much simpler
> than just to return an error if it occurs, so let us handle errors by
> returning them up the stack now.

Is this patch intended to silence a Coverity warning or can this be
triggered by a guest?

I find this commit description and patch confusing. Instead of checking
the actual SIZE_MAX value that bdrv_pad_request() relies on, we use a
32-bit offsets/lengths helper because it checks INT_MAX or SIZE_MAX (but
really INT_MAX, because that's always smaller on host architectures that
QEMU supports).

Vladimir: Is this the intended use of bdrv_check_request32()?

> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 18743311b829cafc1737a5f20bc3248d5f91ee2a
>        ("block: Collapse padded I/O vecs exceeding IOV_MAX")
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  block/io.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

> 
> diff --git a/block/io.c b/block/io.c
> index 30748f0b59..e43b4ad09b 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1710,7 +1710,11 @@ static int bdrv_pad_request(BlockDriverState *bs,
>      int sliced_niov;
>      size_t sliced_head, sliced_tail;
>  
> -    bdrv_check_qiov_request(*offset, *bytes, *qiov, *qiov_offset, &error_abort);
> +    /* Should have been checked by the caller already */
> +    ret = bdrv_check_request32(*offset, *bytes, *qiov, *qiov_offset);
> +    if (ret < 0) {
> +        return ret;
> +    }
>  
>      if (!bdrv_init_padding(bs, *offset, *bytes, write, pad)) {
>          if (padded) {
> @@ -1723,7 +1727,7 @@ static int bdrv_pad_request(BlockDriverState *bs,
>                                    &sliced_head, &sliced_tail,
>                                    &sliced_niov);
>  
> -    /* Guaranteed by bdrv_check_qiov_request() */
> +    /* Guaranteed by bdrv_check_request32() */
>      assert(*bytes <= SIZE_MAX);
>      ret = bdrv_create_padded_qiov(bs, pad, sliced_iov, sliced_niov,
>                                    sliced_head, *bytes);
> -- 
> 2.40.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]