| Message ID | 20240307042645.2827201-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: mpi3mr: Avoid memcpy field-spanning write WARNING | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Wed, Mar 6, 2024 at 9:26 PM Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> wrote: > > When the "storcli2 show" command is executed for eHBA-9600, mpi3mr > driver prints this WARNING: > > memcpy: detected field-spanning write (size 128) of single field "bsg_reply_buf->reply_buf" at drivers/scsi/mpi3mr/mpi3mr_app.c:1658 (size 1) > WARNING: CPU: 0 PID: 12760 at drivers/scsi/mpi3mr/mpi3mr_app.c:1658 mpi3mr_bsg_request+0x6b12/0x7f10 [mpi3mr] > > This is caused by the 128 bytes memcpy to the 1 byte size struct field > replay_buf in the struct mpi3mr_bsg_in_reply_buf. The field is intended > to be a variable length buffer, then the WARN is a false positive. > > One approach to suppress the WARN is to remove the constant '1' from the > replay_buf array declaration to clarify the array size is variable. > However, the array is defined in include/uapi/scsi/scsi_bsg_mpi3mr.h and > the change will break UAPI compatibility. As another approach, divide > the memcpy call into two memcpy calls: one call for the 1 byte size of > the array declaration, and the other call for the left over. While at > it, replace the magic number 1 with sizeof(bsg_reply_buf->reply_buf); > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > --- > drivers/scsi/mpi3mr/mpi3mr_app.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c > index 0380996b5ad2..7fa0710c7574 100644 > --- a/drivers/scsi/mpi3mr/mpi3mr_app.c > +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c > @@ -1233,6 +1233,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) > u8 *din_buf = NULL, *dout_buf = NULL; > u8 *sgl_iter = NULL, *sgl_din_iter = NULL, *sgl_dout_iter = NULL; > u16 rmc_size = 0, desc_count = 0; > + int declared_size; > > bsg_req = job->request; > karg = (struct mpi3mr_bsg_mptcmd *)&bsg_req->cmd.mptcmd; > @@ -1643,9 +1644,11 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) > > if ((mpirep_offset != 0xFF) && > drv_bufs[mpirep_offset].bsg_buf_len) { > + declared_size = sizeof(bsg_reply_buf->reply_buf); > drv_buf_iter = &drv_bufs[mpirep_offset]; > - drv_buf_iter->kern_buf_len = (sizeof(*bsg_reply_buf) - 1 + > - mrioc->reply_sz); > + drv_buf_iter->kern_buf_len = (sizeof(*bsg_reply_buf) > + - declared_size > + + mrioc->reply_sz); > bsg_reply_buf = kzalloc(drv_buf_iter->kern_buf_len, GFP_KERNEL); > > if (!bsg_reply_buf) { > @@ -1655,8 +1658,13 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) > if (mrioc->bsg_cmds.state & MPI3MR_CMD_REPLY_VALID) { > bsg_reply_buf->mpi_reply_type = > MPI3MR_BSG_MPI_REPLY_BUFTYPE_ADDRESS; > + /* Divide memcpy to avoid field-spanning write WARN */ > memcpy(bsg_reply_buf->reply_buf, > - mrioc->bsg_cmds.reply, mrioc->reply_sz); > + mrioc->bsg_cmds.reply, > + declared_size); > + memcpy(bsg_reply_buf->reply_buf + declared_size, > + (u8 *)mrioc->bsg_cmds.reply + declared_size, > + mrioc->reply_sz - declared_size); > } else { > bsg_reply_buf->mpi_reply_type = > MPI3MR_BSG_MPI_REPLY_BUFTYPE_STATUS; > -- > 2.43.0 > The changes look good, however, The uapi structures are not used by any broadcom applications so far and those use their internal files and AFAIK there is no third party developed applications using uapi headers, so can we declare this as a flexible length array to be more clean?
On Mar 19, 2024 / 22:54, Sathya Prakash Veerichetty wrote: > On Wed, Mar 6, 2024 at 9:26 PM Shin'ichiro Kawasaki ... > The changes look good, however, The uapi structures are not used by > any broadcom applications so far and those use their internal files > and AFAIK there is no third party developed applications using uapi > headers, so can we declare this as a flexible length array to be more > clean? Thanks for the comment. I was not sure if the structure is used by applications or not. Since they are not used, I agree that that modification in the UAPI header will be cleaner. Will send v2.
diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c index 0380996b5ad2..7fa0710c7574 100644 --- a/drivers/scsi/mpi3mr/mpi3mr_app.c +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c @@ -1233,6 +1233,7 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) u8 *din_buf = NULL, *dout_buf = NULL; u8 *sgl_iter = NULL, *sgl_din_iter = NULL, *sgl_dout_iter = NULL; u16 rmc_size = 0, desc_count = 0; + int declared_size; bsg_req = job->request; karg = (struct mpi3mr_bsg_mptcmd *)&bsg_req->cmd.mptcmd; @@ -1643,9 +1644,11 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) if ((mpirep_offset != 0xFF) && drv_bufs[mpirep_offset].bsg_buf_len) { + declared_size = sizeof(bsg_reply_buf->reply_buf); drv_buf_iter = &drv_bufs[mpirep_offset]; - drv_buf_iter->kern_buf_len = (sizeof(*bsg_reply_buf) - 1 + - mrioc->reply_sz); + drv_buf_iter->kern_buf_len = (sizeof(*bsg_reply_buf) + - declared_size + + mrioc->reply_sz); bsg_reply_buf = kzalloc(drv_buf_iter->kern_buf_len, GFP_KERNEL); if (!bsg_reply_buf) { @@ -1655,8 +1658,13 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job) if (mrioc->bsg_cmds.state & MPI3MR_CMD_REPLY_VALID) { bsg_reply_buf->mpi_reply_type = MPI3MR_BSG_MPI_REPLY_BUFTYPE_ADDRESS; + /* Divide memcpy to avoid field-spanning write WARN */ memcpy(bsg_reply_buf->reply_buf, - mrioc->bsg_cmds.reply, mrioc->reply_sz); + mrioc->bsg_cmds.reply, + declared_size); + memcpy(bsg_reply_buf->reply_buf + declared_size, + (u8 *)mrioc->bsg_cmds.reply + declared_size, + mrioc->reply_sz - declared_size); } else { bsg_reply_buf->mpi_reply_type = MPI3MR_BSG_MPI_REPLY_BUFTYPE_STATUS;
When the "storcli2 show" command is executed for eHBA-9600, mpi3mr driver prints this WARNING: memcpy: detected field-spanning write (size 128) of single field "bsg_reply_buf->reply_buf" at drivers/scsi/mpi3mr/mpi3mr_app.c:1658 (size 1) WARNING: CPU: 0 PID: 12760 at drivers/scsi/mpi3mr/mpi3mr_app.c:1658 mpi3mr_bsg_request+0x6b12/0x7f10 [mpi3mr] This is caused by the 128 bytes memcpy to the 1 byte size struct field replay_buf in the struct mpi3mr_bsg_in_reply_buf. The field is intended to be a variable length buffer, then the WARN is a false positive. One approach to suppress the WARN is to remove the constant '1' from the replay_buf array declaration to clarify the array size is variable. However, the array is defined in include/uapi/scsi/scsi_bsg_mpi3mr.h and the change will break UAPI compatibility. As another approach, divide the memcpy call into two memcpy calls: one call for the 1 byte size of the array declaration, and the other call for the left over. While at it, replace the magic number 1 with sizeof(bsg_reply_buf->reply_buf); Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- drivers/scsi/mpi3mr/mpi3mr_app.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)