diff mbox

[v5,02/10] raw: Check byte range uniformly

Message ID 20180523030436.29097-3-famz@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fam Zheng May 23, 2018, 3:04 a.m. UTC 
We don't verify the request range against s->size in the I/O callbacks
except for raw_co_pwritev. This is wrong (especially for
raw_co_pwrite_zeroes and raw_co_pdiscard), so fix them.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/raw-format.c | 64 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 25 deletions(-)

Comments

Eric Blake May 23, 2018, 6:28 p.m. UTC | #1 
On 05/22/2018 10:04 PM, Fam Zheng wrote:
> We don't verify the request range against s->size in the I/O callbacks
> except for raw_co_pwritev. This is wrong (especially for
> raw_co_pwrite_zeroes and raw_co_pdiscard), so fix them.

I'd also mention ...

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/raw-format.c | 64 +++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 39 insertions(+), 25 deletions(-)

Should this cc: qemu-stable?

Do we have iotests coverage of this?

> 
> diff --git a/block/raw-format.c b/block/raw-format.c
> index fe33693a2d..b69a0674b3 100644
> --- a/block/raw-format.c
> +++ b/block/raw-format.c
> @@ -167,16 +167,37 @@ static void raw_reopen_abort(BDRVReopenState *state)
>       state->opaque = NULL;
>   }
>   
> +/* Check and adjust the offset, against 'offset' and 'size' options. */
> +static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset,
> +                                    uint64_t bytes, bool is_write)
> +{
> +    BDRVRawState *s = bs->opaque;
> +
> +    if (s->has_size && (*offset > s->size || bytes > (s->size - *offset))) {
> +        /* There's not enough space for the write, or the read request is
> +         * out-of-range. Don't read/write anything to prevent leaking out of
> +         * the size specified in options. */
> +        return is_write ? -ENOSPC : -EINVAL;;
> +    }
> +
> +    if (*offset > INT64_MAX - s->offset) {
> +        return -EINVAL;

...that this change to a 63-bit check...

> @@ -186,23 +207,11 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset,

> -    if (offset > UINT64_MAX - s->offset) {
> -        ret = -EINVAL;
> -        goto fail;
> -    }

...from a previous 64-bit check is intentional.

With improved commit message, and ideally with followup commits that add 
iotest coverage,

Reviewed-by: Eric Blake <eblake@redhat.com>
Fam Zheng May 24, 2018, 8:28 a.m. UTC | #2 
On Wed, 05/23 13:28, Eric Blake wrote:
> On 05/22/2018 10:04 PM, Fam Zheng wrote:
> > We don't verify the request range against s->size in the I/O callbacks
> > except for raw_co_pwritev. This is wrong (especially for
> > raw_co_pwrite_zeroes and raw_co_pdiscard), so fix them.
> 
> I'd also mention ...
> 
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >   block/raw-format.c | 64 +++++++++++++++++++++++++++++++++---------------------
> >   1 file changed, 39 insertions(+), 25 deletions(-)
> 
> Should this cc: qemu-stable?
> 
> Do we have iotests coverage of this?

No for both questions.

This is not reachable from the user interface. In that sense "wrong" is not
accurate in the commit message, "inconsistent" or "confusing" might be more
appropriate.

Out of range requests can only reach us if blk_set_allow_write_beyond_eof(blk,
true) was called, which can only happen when creating image, where the I/O are
all generated internally.

So there is no behavior change in this patch. But verifying and returning error
is still better than asserting, IMO, since we don't want to assert how block
layer uses the driver callbacks. I'll tweak the commit message.

> 
> > 
> > diff --git a/block/raw-format.c b/block/raw-format.c
> > index fe33693a2d..b69a0674b3 100644
> > --- a/block/raw-format.c
> > +++ b/block/raw-format.c
> > @@ -167,16 +167,37 @@ static void raw_reopen_abort(BDRVReopenState *state)
> >       state->opaque = NULL;
> >   }
> > +/* Check and adjust the offset, against 'offset' and 'size' options. */
> > +static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset,
> > +                                    uint64_t bytes, bool is_write)
> > +{
> > +    BDRVRawState *s = bs->opaque;
> > +
> > +    if (s->has_size && (*offset > s->size || bytes > (s->size - *offset))) {
> > +        /* There's not enough space for the write, or the read request is
> > +         * out-of-range. Don't read/write anything to prevent leaking out of
> > +         * the size specified in options. */
> > +        return is_write ? -ENOSPC : -EINVAL;;
> > +    }
> > +
> > +    if (*offset > INT64_MAX - s->offset) {
> > +        return -EINVAL;
> 
> ...that this change to a 63-bit check...
> 
> > @@ -186,23 +207,11 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset,
> 
> > -    if (offset > UINT64_MAX - s->offset) {
> > -        ret = -EINVAL;
> > -        goto fail;
> > -    }
> 
> ...from a previous 64-bit check is intentional.

OK!

> 
> With improved commit message, and ideally with followup commits that add
> iotest coverage,
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Fam
Stefan Hajnoczi May 25, 2018, 3:30 p.m. UTC | #3 
On Wed, May 23, 2018 at 11:04:28AM +0800, Fam Zheng wrote:
> We don't verify the request range against s->size in the I/O callbacks
> except for raw_co_pwritev. This is wrong (especially for
> raw_co_pwrite_zeroes and raw_co_pdiscard), so fix them.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-format.c | 64 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 39 insertions(+), 25 deletions(-)

The uint64_t vs int64_t inconsistency is ugly but pre-existing due to
the interfaces:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox

Patch

diff --git a/block/raw-format.c b/block/raw-format.c
index fe33693a2d..b69a0674b3 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -167,16 +167,37 @@  static void raw_reopen_abort(BDRVReopenState *state)
     state->opaque = NULL;
 }
 
+/* Check and adjust the offset, against 'offset' and 'size' options. */
+static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset,
+                                    uint64_t bytes, bool is_write)
+{
+    BDRVRawState *s = bs->opaque;
+
+    if (s->has_size && (*offset > s->size || bytes > (s->size - *offset))) {
+        /* There's not enough space for the write, or the read request is
+         * out-of-range. Don't read/write anything to prevent leaking out of
+         * the size specified in options. */
+        return is_write ? -ENOSPC : -EINVAL;;
+    }
+
+    if (*offset > INT64_MAX - s->offset) {
+        return -EINVAL;
+    }
+    *offset += s->offset;
+
+    return 0;
+}
+
 static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset,
                                       uint64_t bytes, QEMUIOVector *qiov,
                                       int flags)
 {
-    BDRVRawState *s = bs->opaque;
+    int ret;
 
-    if (offset > UINT64_MAX - s->offset) {
-        return -EINVAL;
+    ret = raw_adjust_offset(bs, &offset, bytes, false);
+    if (ret) {
+        return ret;
     }
-    offset += s->offset;
 
     BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
     return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
@@ -186,23 +207,11 @@  static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset,
                                        uint64_t bytes, QEMUIOVector *qiov,
                                        int flags)
 {
-    BDRVRawState *s = bs->opaque;
     void *buf = NULL;
     BlockDriver *drv;
     QEMUIOVector local_qiov;
     int ret;
 
-    if (s->has_size && (offset > s->size || bytes > (s->size - offset))) {
-        /* There's not enough space for the data. Don't write anything and just
-         * fail to prevent leaking out of the size specified in options. */
-        return -ENOSPC;
-    }
-
-    if (offset > UINT64_MAX - s->offset) {
-        ret = -EINVAL;
-        goto fail;
-    }
-
     if (bs->probed && offset < BLOCK_PROBE_BUF_SIZE && bytes) {
         /* Handling partial writes would be a pain - so we just
          * require that guests have 512-byte request alignment if
@@ -237,7 +246,10 @@  static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset,
         qiov = &local_qiov;
     }
 
-    offset += s->offset;
+    ret = raw_adjust_offset(bs, &offset, bytes, true);
+    if (ret) {
+        goto fail;
+    }
 
     BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
     ret = bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
@@ -267,22 +279,24 @@  static int coroutine_fn raw_co_pwrite_zeroes(BlockDriverState *bs,
                                              int64_t offset, int bytes,
                                              BdrvRequestFlags flags)
 {
-    BDRVRawState *s = bs->opaque;
-    if (offset > UINT64_MAX - s->offset) {
-        return -EINVAL;
+    int ret;
+
+    ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes, true);
+    if (ret) {
+        return ret;
     }
-    offset += s->offset;
     return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
 }
 
 static int coroutine_fn raw_co_pdiscard(BlockDriverState *bs,
                                         int64_t offset, int bytes)
 {
-    BDRVRawState *s = bs->opaque;
-    if (offset > UINT64_MAX - s->offset) {
-        return -EINVAL;
+    int ret;
+
+    ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes, true);
+    if (ret) {
+        return ret;
     }
-    offset += s->offset;
     return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
 }