Re: [Qemu-block] [PATCH v4 02/10] raw: Check byte range uniformly

[Fam Zheng]

On Fri, 05/11 08:59, Eric Blake wrote:
> On 05/11/2018 07:08 AM, 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.
> 
> Did you bother identifying how long the bug has been present (but read
> below, because I'm not sure there was even a bug)?
> 
> CC: qemu-sta...@nongnu.org
> 
> > 
> > Signed-off-by: Fam Zheng 
> > ---
> >   block/raw-format.c | 63 
> > --
> >   1 file changed, 38 insertions(+), 25 deletions(-)
> > 
> > diff --git a/block/raw-format.c b/block/raw-format.c
> > index a378547c99..803083f1a1 100644
> > --- a/block/raw-format.c
> > +++ b/block/raw-format.c
> > @@ -167,16 +167,36 @@ 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)
> > +{
> > +BDRVRawState *s = bs->opaque;
> > +
> > +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;
> > +}
> 
> Can this even trigger? The block layer should already be clamping requests
> according to the device's reported size, and we already report a smaller
> size according to s->size and s->offset.  This could probably be an
> assertion instead.

There is the "write_beyond_eof" semantics in block layer, so the requested range
can escape the allowed here. 

> 
> > +
> > +if (*offset > UINT64_MAX - s->offset) {
> > +return -EINVAL;
> 
> Should this be against INT64_MAX instead?  After all, we really do use off_t
> (a 63-bit quantity, since it is signed), rather than uint64_t, as our
> maximum (theoretical) image size.  But again, can it even trigger, or can it
> be an assertion?
> 
> > +}
> > +*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, , bytes);
> 
> If I'm right and we can assert instead of failing, then raw_adjust_offset()
> doesn't return failure.  If I'm wrong, then there is now a code path where
> we can return ENOSPC on a read, which is unusual and probably wrong.

Yeah, EINVAL is the right value I think.

> 
> > +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 +206,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;
> > -}
> 
> Okay, so you're just doing code refactoring; perhaps we could have done
> assertions here.
> 
> > -
> >   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 +245,10 @@ static int coroutine_fn 
> > raw_co_pwritev(BlockDriverState *bs, uint64_t offset,
> >   qiov = _qiov;
> >   }
> > -offset += s->offset;
> > +ret = raw_adjust_offset(bs, , bytes);
> > +if (ret) {
> > +goto fail;
> > +}
> >   BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
> >   ret = bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
> > @@ -267,22 +278,24 @@ static int coroutine_fn 
> > raw_co_pwrite_zeroes(BlockDriverState *bs,
> >int64_t offset, int bytes,
> >BdrvRequestFlags flags)
> >   {
> > -BDRVRawState *s = bs->opaque;

Re: [Qemu-block] [PATCH v4 02/10] raw: Check byte range uniformly

[Eric Blake]

On 05/11/2018 07:08 AM, 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.


Did you bother identifying how long the bug has been present (but read 
below, because I'm not sure there was even a bug)?


CC: qemu-sta...@nongnu.org



Signed-off-by: Fam Zheng 
---
  block/raw-format.c | 63 --
  1 file changed, 38 insertions(+), 25 deletions(-)

diff --git a/block/raw-format.c b/block/raw-format.c
index a378547c99..803083f1a1 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -167,16 +167,36 @@ 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)
+{
+BDRVRawState *s = bs->opaque;
+
+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;
+}


Can this even trigger? The block layer should already be clamping 
requests according to the device's reported size, and we already report 
a smaller size according to s->size and s->offset.  This could probably 
be an assertion instead.



+
+if (*offset > UINT64_MAX - s->offset) {
+return -EINVAL;


Should this be against INT64_MAX instead?  After all, we really do use 
off_t (a 63-bit quantity, since it is signed), rather than uint64_t, as 
our maximum (theoretical) image size.  But again, can it even trigger, 
or can it be an assertion?



+}
+*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, , bytes);


If I'm right and we can assert instead of failing, then 
raw_adjust_offset() doesn't return failure.  If I'm wrong, then there is 
now a code path where we can return ENOSPC on a read, which is unusual 
and probably wrong.



+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 +206,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;
-}


Okay, so you're just doing code refactoring; perhaps we could have done 
assertions here.



-
  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 +245,10 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
  qiov = _qiov;
  }
  
-offset += s->offset;

+ret = raw_adjust_offset(bs, , bytes);
+if (ret) {
+goto fail;
+}
  
  BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);

  ret = bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
@@ -267,22 +278,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 *), bytes);
+if (ret) {
+return ret;
  }
-offset += s->offset;


If I'm right and raw_adjust_offset() can't fail, then this didn't add 
any protection.  If I'm wrong and it is possible to get the block layer 
to send a request beyond our advertised size, then this is indeed a bug 
fix worthy of being on the stable branch.



  return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
  }
  
  static int coroutine_fn