Re: [PATCH 1/3] scsi: mpi3mr: fix alltgt_info copy size in mpi3mr_get_all_tgt_info

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 12/12/22 10:57, Shin'ichiro Kawasaki wrote:
> The function mpi3mr_get_all_tgt_info calculates size of alltgt_info and
> allocate memory for it. After preparing valid data in alltgt_info, it
> calls sg_copy_from_buffer to copy alltgt_info to job->request_payload,
> specifying length of the payload as copy length. This length is larger
> than the calculated alltgt_info size. It causes memory access to invalid
> address and results in "BUG: KASAN: slab-out-of-bounds". The BUG was
> observed during boot using systems with eHBA-9600. By updating the HBA
> firmware to latest version 8.3.1.0 the BUG was not observed during boot,
> but still observed when command "storcli2 /c0 show" is executed.
> 
> Fix the BUG by specifying the calculated alltgt_info size as copy
> length. Also check that the copy destination payload length is larger
> than the copy length.
> 
> Fixes: f5e6d5a34376 ("scsi: mpi3mr: Add support for driver commands")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  drivers/scsi/mpi3mr/mpi3mr_app.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 9baac224b213..f14556d50832 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -322,6 +322,13 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
>  
>  	kern_entrylen = (num_devices - 1) * sizeof(*devmap_info);
>  	size = sizeof(*alltgt_info) + kern_entrylen;
> +
> +	if (size > job->request_payload.payload_len) {
> +		dprint_bsg_err(mrioc, "%s: too small payload length\n",

"%s: payload length is too small\n" maybe ?

> +			       __func__);
> +		return rval;
> +	}
> +
>  	alltgt_info = kzalloc(size, GFP_KERNEL);
>  	if (!alltgt_info)
>  		return -ENOMEM;
> @@ -358,7 +365,7 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
>  
>  	sg_copy_from_buffer(job->request_payload.sg_list,
>  			    job->request_payload.sg_cnt,
> -			    alltgt_info, job->request_payload.payload_len);
> +			    alltgt_info, size);
>  	rval = 0;
>  out:
>  	kfree(alltgt_info);

Otherwise, looks OK to me.

Reviewed-by: Damien Le Moal

-- 
Damien Le Moal
Western Digital Research