Skip to content

Commit

Permalink
scsi/qemu-pr-helper: Fix out-of-bounds access to trnptid_list[]
Browse files Browse the repository at this point in the history
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 malloc `paramp`.

Signed-off-by: Christophe de Dinechin <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
  • Loading branch information
c3d authored and bonzini committed Mar 16, 2020
1 parent a98135f commit 4ce1e15
Showing 1 changed file with 10 additions and 7 deletions.
17 changes: 10 additions & 7 deletions scsi/qemu-pr-helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -421,10 +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 prout_param_descriptor paramp;
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.
Expand Down Expand Up @@ -454,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.key, &param[0], 8);
memcpy(&paramp.sa_key, &param[8], 8);
paramp.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;
Expand Down Expand Up @@ -520,12 +522,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->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, noisy, verbose);
paramp, noisy, verbose);
return mpath_reconstruct_sense(fd, r, sense);
}
#endif
Expand Down

0 comments on commit 4ce1e15

Please sign in to comment.