Re: [PATCH] scsi: mpi3mr: Avoid memcpy field-spanning write WARNING

[Sathya Prakash Veerichetty <sathya.prakash@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 4091 bytes --]

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?

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4227 bytes --]