[Qemu-devel] [PATCH for-2.8 0/2] raw_bsd: Fixing use of the 'offset' option without 'size'

[Qemu-devel] [PATCH for-2.8 0/2] raw_bsd: Fixing use of the 'offset' option without 'size'

[Tomáš Golembiovský]

The patch set contains two patches related to the use of 'offset' option:

-  The first patch is purely cosmetic. Although it touches overflow
   check it only affects what error message is produced.

-  Second patch lessens the restriction on the size alignment when only
   'offset' is specified. The restriction is not only unneeded, it makes
   it difficult to use the 'offset' option.

Tomáš Golembiovský (2):
  raw_bsd: move check to prevent overflow
  raw_bsd: don't check size alignment when only offset is set

 block/raw_bsd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.10.1

[Qemu-devel] [PATCH for-2.8 1/2] raw_bsd: move check to prevent overflow

[Tomáš Golembiovský]

When only offset is specified but no size and the offset is greater than
the real size of the containing device an overflow occurs when parsing
the options. This overflow is harmless because we do check for this
exact situation little bit later, but it leads to an error message with
weird values. It is better to do the check is sooner and prevent the
overflow.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 block/raw_bsd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 7c9bebb..cf7a560 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -91,6 +91,14 @@ static int raw_read_options(QDict *options, BlockDriverState *bs,
     }
 
     s->offset = qemu_opt_get_size(opts, "offset", 0);
+    if (s->offset > real_size) {
+        error_setg(errp, "Offset (%" PRIu64 ") cannot be greater than "
+            "size of the containing file (%" PRId64 ")",
+            s->offset, real_size);
+        ret = -EINVAL;
+        goto end;
+    }
+
     if (qemu_opt_find(opts, "size") != NULL) {
         s->size = qemu_opt_get_size(opts, "size", 0);
         s->has_size = true;
@@ -100,7 +108,7 @@ static int raw_read_options(QDict *options, BlockDriverState *bs,
     }
 
     /* Check size and offset */
-    if (real_size < s->offset || (real_size - s->offset) < s->size) {
+    if ((real_size - s->offset) < s->size) {
         error_setg(errp, "The sum of offset (%" PRIu64 ") and size "
             "(%" PRIu64 ") has to be smaller or equal to the "
             " actual size of the containing file (%" PRId64 ")",
-- 
2.10.1

[Qemu-devel] [PATCH for-2.8 2/2] raw_bsd: don't check size alignment when only offset is set

[Tomáš Golembiovský]

We make sure that the size is aligned to sector length to prevent any
round ups. Otherwise we could end up reading/writing data outside the
area specified by user. This is only needed when user supplies the size
option to avoid any surprises. It is not necessary when only offset is
set.

More over, the check made it difficult to use the offset option without
size option. The check puts unneeded restriction on the offset which had
to be aligned too. Because bdrv_getlength() returns aligned value having
unaligned offset would make the check fail.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 block/raw_bsd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index cf7a560..8a5b9b0 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -119,7 +119,7 @@ static int raw_read_options(QDict *options, BlockDriverState *bs,
 
     /* Make sure size is multiple of BDRV_SECTOR_SIZE to prevent rounding
      * up and leaking out of the specified area. */
-    if (!QEMU_IS_ALIGNED(s->size, BDRV_SECTOR_SIZE)) {
+    if (s->has_size && !QEMU_IS_ALIGNED(s->size, BDRV_SECTOR_SIZE)) {
         error_setg(errp, "Specified size is not multiple of %llu",
             BDRV_SECTOR_SIZE);
         ret = -EINVAL;
-- 
2.10.1

Re: [Qemu-devel] [PATCH for-2.8 0/2] raw_bsd: Fixing use of the 'offset' option without 'size'

[Kevin Wolf]

Am 03.11.2016 um 14:47 hat Tomáš Golembiovský geschrieben:
> The patch set contains two patches related to the use of 'offset' option:
> 
> -  The first patch is purely cosmetic. Although it touches overflow
>    check it only affects what error message is produced.
> 
> -  Second patch lessens the restriction on the size alignment when only
>    'offset' is specified. The restriction is not only unneeded, it makes
>    it difficult to use the 'offset' option.

Thanks, applied to the block branch.

Kevin