raw_bsd: Fixing use of the 'offset' option without 'size'

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

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.

---
 block/raw_bsd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

index 7c9bebb..cf7a560 100644
@@ -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

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.

---
 block/raw_bsd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index cf7a560..8a5b9b0 100644
@@ -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

Am 03.11.2016 um 14:47 hat Tomáš Golembiovský geschrieben: