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

[Paolo Bonzini]

On 28/02/20 16:00, Christophe de Dinechin wrote:
> 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 unfortunately causes a compilation warning-turned-error because 
of a flexible array member in the middle of a struct.  The simplest fix 
is just to malloc the struct:

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 0101322..65a8261 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -421,15 +421,13 @@ 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
-    {
-        struct prout_param_descriptor descr;
-        struct transportid *trnptid_list_storage[MPATH_MX_TIDS];
-    } paramp;
-    struct transportid **trnptid_list = paramp.descr.trnptid_list;
+    g_autofree struct prout_param_descriptor *paramp = NULL;
     char transportids[PR_HELPER_DATA_SIZE];
     int r;
 
+    paramp = g_malloc0(sizeof(struct prout_param_descriptor)
+                      + sizeof(struct transportid *) * MPATH_MX_TIDS);
+
     if (sz < PR_OUT_FIXED_PARAM_SIZE) {
         /* Illegal request, Parameter list length error.  This isn't fatal;
          * we have read the data, send an error without closing the socket.
@@ -459,10 +457,9 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, 
uint8_t *sense,
      * used by libmpathpersist (which, of course, will immediately
      * do the opposite).
      */
-    memset(&paramp, 0, sizeof(paramp));
-    memcpy(&paramp.descr.key, &param[0], 8);
-    memcpy(&paramp.descr.sa_key, &param[8], 8);
-    paramp.descr.sa_flags = param[20];
+    memcpy(&paramp->key, &param[0], 8);
+    memcpy(&paramp->sa_key, &param[8], 8);
+    paramp->sa_flags = param[20];
     if (sz > PR_OUT_FIXED_PARAM_SIZE) {
         size_t transportid_len;
         int i, j;
@@ -525,13 +522,13 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, 
uint8_t *sense,
                 return CHECK_CONDITION;
             }
 
-            assert(paramp.descr.num_transportid < MPATH_MX_TIDS);
-            trnptid_list[paramp.descr.num_transportid++] = id;
+            assert(paramp->num_transportid < MPATH_MX_TIDS);
+            paramp->trnptid_list[paramp->num_transportid++] = id;
         }
     }
 
     r = mpath_persistent_reserve_out(fd, rq_servact, rq_scope, rq_type,
-                                     &paramp.descr, noisy, verbose);
+                                     paramp, noisy, verbose);
     return mpath_reconstruct_sense(fd, r, sense);
 }
 #endif