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

[Stefan Hajnoczi]

On Wed, Jul 12, 2023 at 09:41:05AM +0200, Hanna Czenczek wrote:
> On 11.07.23 22:23, Stefan Hajnoczi wrote:
> > 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?
> 
> Neither.  There was a Coverity warning about the `assert(*bytes <=
> SIZE_MAX)`, which is always true on 32-bit architectures. Regardless of
> Coverity, Peter inquired how bdrv_check_qiov_request() would guarantee this
> condition (as the comments I’ve put above the assertions say).  It doesn’t,
> only bdrv_check_request32() does, which I was thinking of, and just confused
> the two.

It's unclear to me whether this patch silences a Coverity warning or
not? You said "neither", but then you acknowledged there was a Coverity
warning. Maybe "was" (past-tense) means something else already fixed it
but I don't see any relevant commits in the git log.

> As the commit message says, all callers already run bdrv_check_request32(),
> so I expect this change to functionally be a no-op.  (That is why the
> pre-patch code runs bdrv_check_qiov_request() with `&error_abort`.)

Okay, this means a guest cannot trigger the assertion failure.

Please mention the intent in the commit description: a code cleanup
requested by Peter and/or a Coverity warning fix, but definitely not
guest triggerable assertion failure.

> 
> > 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).
> 
> I preferred to use a bounds-checking function that we already use for
> requests, and that happens to be used to limit all I/O that ends up here in
> bdrv_pad_request() anyway, instead of adding a new specific limit.
> 
> It doesn’t matter to me, though.  The callers already ensure that everything
> is in bounds, so I’d be happy with anything, ranging from keeping the bare
> assertions with no checks beforehand, over specifically checking SIZE_MAX
> and returning an error then, to bdrv_check_request32().
> 
> (I thought repeating the simple bounds check that all callers already did
> for verbosity would be the most robust and obvious way to do it, but now I’m
> biting myself for not just using bare assertions annotated with “Caller must
> guarantee this” from the start...)

Okay. I looked at the code more and don't see a cleanup for the overall
problem of duplicated checks and type mismatches (size_t vs int64_t)
that is appropriate for this patch.

I'm okay with this fix, but please clarify the intent as mentioned above.

> 
> Hanna
> 
> > 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
> > > 
>

signature.asc
Description: PGP signature