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;
>       |                                   ^~~~~~

Ouch.  Very nice new warning.

Cc: qemu-sta...@nongnu.org

Queued, thanks.

> 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 <dinec...@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
>