Skip to contentSkip to conversation

qcow2.c: Check if backing file name length is valid

PATCH In review Deepak Kathayat 2 participants 2 messages Mar 19, 2014 – Apr 28, 2014 Block layer

lore
Share & tools
lore

Deepak Kathayat

author
#0 lore raw

PATCH 1/1 qcow2.c: Check if backing file name length is valid

Signed-off-by Deepak Kathayat <[email protected]>

---
The len variable is a signed integer whereas the backing file name
length in the image header is unsigned. Therefore, it may overflow. Furthermore, backing file name length cannot be zero. These two cases must be handled explicitly.

 block/qcow2.c |    5 +++++
 1 file changed, 5 insertions(+)
block/qcow2.c +5 -0
index 945c9d6..7b6f65c 100644
@@ -625,6 +625,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     /* read the backing file name */
     if (header.backing_file_offset != 0) {
         len = header.backing_file_size;
+        if (len <= 0) {
+            error_setg(errp, "Invalid backing file name length: %d", len);
+            ret = -EINVAL;
+            goto fail;
+        }
         if (len > 1023) {
             len = 1023;
         }
Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 15 lines · Deepak Kathayat
Signed-off-by Deepak Kathayat <[email protected]>

---
The len variable is a signed integer whereas the backing file name
length in the image header is unsigned. Therefore, it may overflow. Furthermore, backing file name length cannot be zero. These two cases must be handled explicitly.

 block/qcow2.c |    5 +++++
 1 file changed, 5 insertions(+)
block/qcow2.c +0 -0
index 945c9d6..7b6f65c 100644
@@ -625,6 +625,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     /* read the backing file name */

block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, · line 635

Quoted 10 lines
block/qcow2.c +5 -0
     if (header.backing_file_offset != 0) {
         len = header.backing_file_size;
+        if (len <= 0) {
+            error_setg(errp, "Invalid backing file name length: %d", len);
+            ret = -EINVAL;
+            goto fail;
+        }
         if (len > 1023) {
             len = 1023;
         }

A better fix has been implemented meanwhile, as a part of input format validation series:

commit 6d33e8e7dc9d40ea105feed4b39caa3e641569e8

Author: Kevin Wolf <[email protected]>
Date:   Wed Mar 26 13:05:47 2014 +0100

qcow2: Fix backing file name length check

len could become negative and would pass the check then. Nothing bad happened because bdrv_pread() happens to return an error for negative length values, but make variables for sizes unsigned anyway.

This patch also changes the behaviour to error out on invalid lengths instead of silently truncating it to 1023.

Thanks,

/mjt