[PATCH] scsi/qemu-pr-helper: Fix out-of-bounds access to trnptid_list[]

* [PATCH] scsi/qemu-pr-helper: Fix out-of-bounds access to trnptid_list[]
@ 2020-02-28 15:00 Christophe de Dinechin
  2020-03-02 15:38 ` Paolo Bonzini
  2020-03-16 21:06 ` Paolo Bonzini
  0 siblings, 2 replies; 3+ messages in thread
From: Christophe de Dinechin @ 2020-02-28 15:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Fam Zheng, Paolo Bonzini, qemu-block

Compile error reported by gcc 10.0.1:

scsi/qemu-pr-helper.c: In function ‘multipath_pr_out’:
scsi/qemu-pr-helper.c:523:32: error: array subscript <unknown> is outside array bounds of ‘struct transportid *[0]’ [-Werror=array-bounds]
  523 |             paramp.trnptid_list[paramp.num_transportid++] = id;
      |             ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from scsi/qemu-pr-helper.c:36:
/usr/include/mpath_persist.h:168:22: note: while referencing ‘trnptid_list’
  168 |  struct transportid *trnptid_list[];
      |                      ^~~~~~~~~~~~
scsi/qemu-pr-helper.c:424:35: note: defined here ‘paramp’
  424 |     struct prout_param_descriptor paramp;
      |                                   ^~~~~~

This highlights an actual implementation issue in function multipath_pr_out.
The variable paramp is declared with type `struct prout_param_descriptor`,
which is a struct terminated by an empty array in mpath_persist.h:

        struct transportid *trnptid_list[];

That empty array was filled with code that looked like that:

        trnptid_list[paramp.descr.num_transportid++] = id;

This is an actual out-of-bounds access.

The fix is to replace `paramp` with an anonymous struct that adds
additional space for the data, called `trnptid_list_storage`.
That space provides MATH_MX_TIDS entries, and is not accessed directly
but through a pointer to `descr.trnptid_list`, in the unlikely case a
future compiler inserts some padding between the two structs.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 scsi/qemu-pr-helper.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 0659ceef09..01013221b3 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -421,7 +421,12 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
     int rq_servact = cdb[1];
     int rq_scope = cdb[2] >> 4;
     int rq_type = cdb[2] & 0xf;
-    struct prout_param_descriptor paramp;
+    struct
+    {
+        struct prout_param_descriptor descr;
+        struct transportid *trnptid_list_storage[MPATH_MX_TIDS];
+    } paramp;
+    struct transportid **trnptid_list = paramp.descr.trnptid_list;
     char transportids[PR_HELPER_DATA_SIZE];
     int r;
 
@@ -455,9 +460,9 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
      * do the opposite).
      */
     memset(&paramp, 0, sizeof(paramp));
-    memcpy(&paramp.key, &param[0], 8);
-    memcpy(&paramp.sa_key, &param[8], 8);
-    paramp.sa_flags = param[20];
+    memcpy(&paramp.descr.key, &param[0], 8);
+    memcpy(&paramp.descr.sa_key, &param[8], 8);
+    paramp.descr.sa_flags = param[20];
     if (sz > PR_OUT_FIXED_PARAM_SIZE) {
         size_t transportid_len;
         int i, j;
@@ -520,12 +525,13 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
                 return CHECK_CONDITION;
             }
 
-            paramp.trnptid_list[paramp.num_transportid++] = id;
+            assert(paramp.descr.num_transportid < MPATH_MX_TIDS);
+            trnptid_list[paramp.descr.num_transportid++] = id;
         }
     }
 
     r = mpath_persistent_reserve_out(fd, rq_servact, rq_scope, rq_type,
-                                     &paramp, noisy, verbose);
+                                     &paramp.descr, noisy, verbose);
     return mpath_reconstruct_sense(fd, r, sense);
 }
 #endif
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 3+ messages in thread