| Message ID | 20200228150059.2644362-1-dinechin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi/qemu-pr-helper: Fix out-of-bounds access to trnptid_list[] | expand |
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-stable@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 <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(¶mp, 0, sizeof(paramp)); > - memcpy(¶mp.key, ¶m[0], 8); > - memcpy(¶mp.sa_key, ¶m[8], 8); > - paramp.sa_flags = param[20]; > + memcpy(¶mp.descr.key, ¶m[0], 8); > + memcpy(¶mp.descr.sa_key, ¶m[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, > - ¶mp, noisy, verbose); > + ¶mp.descr, noisy, verbose); > return mpath_reconstruct_sense(fd, r, sense); > } > #endif >
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(¶mp, 0, sizeof(paramp)); - memcpy(¶mp.descr.key, ¶m[0], 8); - memcpy(¶mp.descr.sa_key, ¶m[8], 8); - paramp.descr.sa_flags = param[20]; + memcpy(¶mp->key, ¶m[0], 8); + memcpy(¶mp->sa_key, ¶m[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, - ¶mp.descr, noisy, verbose); + paramp, noisy, verbose); return mpath_reconstruct_sense(fd, r, sense); } #endif
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(¶mp, 0, sizeof(paramp)); - memcpy(¶mp.key, ¶m[0], 8); - memcpy(¶mp.sa_key, ¶m[8], 8); - paramp.sa_flags = param[20]; + memcpy(¶mp.descr.key, ¶m[0], 8); + memcpy(¶mp.descr.sa_key, ¶m[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, - ¶mp, noisy, verbose); + ¶mp.descr, noisy, verbose); return mpath_reconstruct_sense(fd, r, sense); } #endif
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(-)