[PATCH 0/3] scsi: mpi3mr: fix issues found by KASAN

[PATCH 0/3] scsi: mpi3mr: fix issues found by KASAN

[Shin'ichiro Kawasaki]

While I downloaded new firmware to eHBA-9600 on KASAN enabled kernel,
I observed three BUGs. This series addresses them.

Shin'ichiro Kawasaki (3):
  scsi: mpi3mr: fix alltgt_info copy size in mpi3mr_get_all_tgt_info
  scsi: mpi3mr: fix bitmap memory size calculation
  scsi: mpi3mr: fix missing mrioc->evtack_cmds initialization

 drivers/scsi/mpi3mr/mpi3mr_app.c |  9 ++++++++-
 drivers/scsi/mpi3mr/mpi3mr_fw.c  | 25 ++++++++++---------------
 drivers/scsi/mpi3mr/mpi3mr_os.c  |  4 ++++
 3 files changed, 22 insertions(+), 16 deletions(-)

-- 
2.37.1

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

[Shin'ichiro Kawasaki]

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",
+			       __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);
-- 
2.37.1

[PATCH 2/3] scsi: mpi3mr: fix bitmap memory size calculation

[Shin'ichiro Kawasaki]

To allocate memory for bitmaps, the mpi3mr driver calculates sizes of
each bitmap using byte as unit. However, bit operation helper functions
assume that bitmaps are allocated using unsigned long as unit. This gap
causes memory access beyond the bitmap memory size and results in "BUG:
KASAN: slab-out-of-bounds". The BUG was observed at firmware download to
eHBA-9600. Call trace indicated that the out-of-bounds access happened
in find_first_zero_bit called from mpi3mr_send_event_ack for the bitmap
miroc->evtack_cmds_bitmap.

To avoid the BUG, fix bitmap size calculations using unsigned long as
unit. Apply this fix to five places to cover all bitmap size
calculations in the driver.

Fixes: c5758fc72b92 ("scsi: mpi3mr: Gracefully handle online FW update operation")
Fixes: e844adb1fbdc ("scsi: mpi3mr: Implement SCSI error handler hooks")
Fixes: c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
Fixes: 824a156633df ("scsi: mpi3mr: Base driver code")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 0c4aabaefdcc..272c318387b7 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -1160,9 +1160,8 @@ mpi3mr_revalidate_factsdata(struct mpi3mr_ioc *mrioc)
 		    "\tcontroller while sas transport support is enabled at the\n"
 		    "\tdriver, please reboot the system or reload the driver\n");
 
-	dev_handle_bitmap_sz = mrioc->facts.max_devhandle / 8;
-	if (mrioc->facts.max_devhandle % 8)
-		dev_handle_bitmap_sz++;
+	dev_handle_bitmap_sz = sizeof(unsigned long) *
+		DIV_ROUND_UP(mrioc->facts.max_devhandle, BITS_PER_LONG);
 	if (dev_handle_bitmap_sz > mrioc->dev_handle_bitmap_sz) {
 		removepend_bitmap = krealloc(mrioc->removepend_bitmap,
 		    dev_handle_bitmap_sz, GFP_KERNEL);
@@ -2957,25 +2956,22 @@ static int mpi3mr_alloc_reply_sense_bufs(struct mpi3mr_ioc *mrioc)
 	if (!mrioc->pel_abort_cmd.reply)
 		goto out_failed;
 
-	mrioc->dev_handle_bitmap_sz = mrioc->facts.max_devhandle / 8;
-	if (mrioc->facts.max_devhandle % 8)
-		mrioc->dev_handle_bitmap_sz++;
+	mrioc->dev_handle_bitmap_sz = sizeof(unsigned long) *
+		DIV_ROUND_UP(mrioc->facts.max_devhandle, BITS_PER_LONG);
 	mrioc->removepend_bitmap = kzalloc(mrioc->dev_handle_bitmap_sz,
 	    GFP_KERNEL);
 	if (!mrioc->removepend_bitmap)
 		goto out_failed;
 
-	mrioc->devrem_bitmap_sz = MPI3MR_NUM_DEVRMCMD / 8;
-	if (MPI3MR_NUM_DEVRMCMD % 8)
-		mrioc->devrem_bitmap_sz++;
+	mrioc->devrem_bitmap_sz = sizeof(unsigned long) *
+		DIV_ROUND_UP(MPI3MR_NUM_DEVRMCMD, BITS_PER_LONG);
 	mrioc->devrem_bitmap = kzalloc(mrioc->devrem_bitmap_sz,
 	    GFP_KERNEL);
 	if (!mrioc->devrem_bitmap)
 		goto out_failed;
 
-	mrioc->evtack_cmds_bitmap_sz = MPI3MR_NUM_EVTACKCMD / 8;
-	if (MPI3MR_NUM_EVTACKCMD % 8)
-		mrioc->evtack_cmds_bitmap_sz++;
+	mrioc->evtack_cmds_bitmap_sz = sizeof(unsigned long) *
+		DIV_ROUND_UP(MPI3MR_NUM_EVTACKCMD, BITS_PER_LONG);
 	mrioc->evtack_cmds_bitmap = kzalloc(mrioc->evtack_cmds_bitmap_sz,
 	    GFP_KERNEL);
 	if (!mrioc->evtack_cmds_bitmap)
@@ -3415,9 +3411,8 @@ static int mpi3mr_alloc_chain_bufs(struct mpi3mr_ioc *mrioc)
 		if (!mrioc->chain_sgl_list[i].addr)
 			goto out_failed;
 	}
-	mrioc->chain_bitmap_sz = num_chains / 8;
-	if (num_chains % 8)
-		mrioc->chain_bitmap_sz++;
+	mrioc->chain_bitmap_sz = sizeof(unsigned long) *
+		DIV_ROUND_UP(num_chains, BITS_PER_LONG);
 	mrioc->chain_bitmap = kzalloc(mrioc->chain_bitmap_sz, GFP_KERNEL);
 	if (!mrioc->chain_bitmap)
 		goto out_failed;
-- 
2.37.1

[PATCH 3/3] scsi: mpi3mr: fix missing mrioc->evtack_cmds initialization

[Shin'ichiro Kawasaki]

The commit c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
introduced an array mrioc->evtack_cmds. But initialization of the array
elements was missed. They are just zero cleared. The function
mpi3mr_complete_evt_ack refers host_tag field of the elements. Due to
zero value of the host_tag field, the functions calls clear_bit for
mrico->evtack_cmds_bitmap with wrong bit index. This results in memory
access to invalid address and "BUG: KASAN: use-after-free". This BUG was
observed at eHBA-9600 firmware update to version 8.3.1.0. To fix it, add
the missing initialization of mrioc->evtack_cmds.

Fixes: c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
Cc: stable@vger.kernel.org
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 drivers/scsi/mpi3mr/mpi3mr_os.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 3306de7170f6..6eaeba41072c 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -4952,6 +4952,10 @@ mpi3mr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		mpi3mr_init_drv_cmd(&mrioc->dev_rmhs_cmds[i],
 		    MPI3MR_HOSTTAG_DEVRMCMD_MIN + i);
 
+	for (i = 0; i < MPI3MR_NUM_EVTACKCMD; i++)
+		mpi3mr_init_drv_cmd(&mrioc->evtack_cmds[i],
+				    MPI3MR_HOSTTAG_EVTACKCMD_MIN + i);
+
 	if (pdev->revision)
 		mrioc->enable_segqueue = true;
 
-- 
2.37.1

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

[Damien Le Moal]

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

Re: [PATCH 2/3] scsi: mpi3mr: fix bitmap memory size calculation

[Damien Le Moal]

On 12/12/22 10:57, Shin'ichiro Kawasaki wrote:
> To allocate memory for bitmaps, the mpi3mr driver calculates sizes of
> each bitmap using byte as unit. However, bit operation helper functions
> assume that bitmaps are allocated using unsigned long as unit. This gap
> causes memory access beyond the bitmap memory size and results in "BUG:
> KASAN: slab-out-of-bounds". The BUG was observed at firmware download to
> eHBA-9600. Call trace indicated that the out-of-bounds access happened
> in find_first_zero_bit called from mpi3mr_send_event_ack for the bitmap
> miroc->evtack_cmds_bitmap.
> 
> To avoid the BUG, fix bitmap size calculations using unsigned long as
> unit. Apply this fix to five places to cover all bitmap size
> calculations in the driver.
> 
> Fixes: c5758fc72b92 ("scsi: mpi3mr: Gracefully handle online FW update operation")
> Fixes: e844adb1fbdc ("scsi: mpi3mr: Implement SCSI error handler hooks")
> Fixes: c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
> Fixes: 824a156633df ("scsi: mpi3mr: Base driver code")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  drivers/scsi/mpi3mr/mpi3mr_fw.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 0c4aabaefdcc..272c318387b7 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -1160,9 +1160,8 @@ mpi3mr_revalidate_factsdata(struct mpi3mr_ioc *mrioc)
>  		    "\tcontroller while sas transport support is enabled at the\n"
>  		    "\tdriver, please reboot the system or reload the driver\n");
>  
> -	dev_handle_bitmap_sz = mrioc->facts.max_devhandle / 8;
> -	if (mrioc->facts.max_devhandle % 8)
> -		dev_handle_bitmap_sz++;
> +	dev_handle_bitmap_sz = sizeof(unsigned long) *
> +		DIV_ROUND_UP(mrioc->facts.max_devhandle, BITS_PER_LONG);
>  	if (dev_handle_bitmap_sz > mrioc->dev_handle_bitmap_sz) {
>  		removepend_bitmap = krealloc(mrioc->removepend_bitmap,
>  		    dev_handle_bitmap_sz, GFP_KERNEL);
> @@ -2957,25 +2956,22 @@ static int mpi3mr_alloc_reply_sense_bufs(struct mpi3mr_ioc *mrioc)
>  	if (!mrioc->pel_abort_cmd.reply)
>  		goto out_failed;
>  
> -	mrioc->dev_handle_bitmap_sz = mrioc->facts.max_devhandle / 8;
> -	if (mrioc->facts.max_devhandle % 8)
> -		mrioc->dev_handle_bitmap_sz++;

mrioc->dev_handle_bitmap_sz = (mrioc->facts.max_devhandle + 7) >> 3;

would be a lot simpler...

> +	mrioc->dev_handle_bitmap_sz = sizeof(unsigned long) *
> +		DIV_ROUND_UP(mrioc->facts.max_devhandle, BITS_PER_LONG);
>  	mrioc->removepend_bitmap = kzalloc(mrioc->dev_handle_bitmap_sz,
>  	    GFP_KERNEL);

What about using bitmap_alloc() here and keep the dev_handle_bitmap_sz
value as is ?

(same for the other bitmaps)

>  	if (!mrioc->removepend_bitmap)
>  		goto out_failed;
>  
> -	mrioc->devrem_bitmap_sz = MPI3MR_NUM_DEVRMCMD / 8;
> -	if (MPI3MR_NUM_DEVRMCMD % 8)
> -		mrioc->devrem_bitmap_sz++;
> +	mrioc->devrem_bitmap_sz = sizeof(unsigned long) *
> +		DIV_ROUND_UP(MPI3MR_NUM_DEVRMCMD, BITS_PER_LONG);
>  	mrioc->devrem_bitmap = kzalloc(mrioc->devrem_bitmap_sz,
>  	    GFP_KERNEL);
>  	if (!mrioc->devrem_bitmap)
>  		goto out_failed;
>  
> -	mrioc->evtack_cmds_bitmap_sz = MPI3MR_NUM_EVTACKCMD / 8;
> -	if (MPI3MR_NUM_EVTACKCMD % 8)
> -		mrioc->evtack_cmds_bitmap_sz++;
> +	mrioc->evtack_cmds_bitmap_sz = sizeof(unsigned long) *
> +		DIV_ROUND_UP(MPI3MR_NUM_EVTACKCMD, BITS_PER_LONG);
>  	mrioc->evtack_cmds_bitmap = kzalloc(mrioc->evtack_cmds_bitmap_sz,
>  	    GFP_KERNEL);
>  	if (!mrioc->evtack_cmds_bitmap)
> @@ -3415,9 +3411,8 @@ static int mpi3mr_alloc_chain_bufs(struct mpi3mr_ioc *mrioc)
>  		if (!mrioc->chain_sgl_list[i].addr)
>  			goto out_failed;
>  	}
> -	mrioc->chain_bitmap_sz = num_chains / 8;
> -	if (num_chains % 8)
> -		mrioc->chain_bitmap_sz++;
> +	mrioc->chain_bitmap_sz = sizeof(unsigned long) *
> +		DIV_ROUND_UP(num_chains, BITS_PER_LONG);
>  	mrioc->chain_bitmap = kzalloc(mrioc->chain_bitmap_sz, GFP_KERNEL);
>  	if (!mrioc->chain_bitmap)
>  		goto out_failed;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] scsi: mpi3mr: fix missing mrioc->evtack_cmds initialization

[Damien Le Moal]

On 12/12/22 10:57, Shin'ichiro Kawasaki wrote:
> The commit c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
> introduced an array mrioc->evtack_cmds. But initialization of the array
> elements was missed. They are just zero cleared. The function
> mpi3mr_complete_evt_ack refers host_tag field of the elements. Due to
> zero value of the host_tag field, the functions calls clear_bit for
> mrico->evtack_cmds_bitmap with wrong bit index. This results in memory
> access to invalid address and "BUG: KASAN: use-after-free". This BUG was
> observed at eHBA-9600 firmware update to version 8.3.1.0. To fix it, add
> the missing initialization of mrioc->evtack_cmds.
> 
> Fixes: c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Looks OK to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>


> ---
>  drivers/scsi/mpi3mr/mpi3mr_os.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 3306de7170f6..6eaeba41072c 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -4952,6 +4952,10 @@ mpi3mr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		mpi3mr_init_drv_cmd(&mrioc->dev_rmhs_cmds[i],
>  		    MPI3MR_HOSTTAG_DEVRMCMD_MIN + i);
>  
> +	for (i = 0; i < MPI3MR_NUM_EVTACKCMD; i++)
> +		mpi3mr_init_drv_cmd(&mrioc->evtack_cmds[i],
> +				    MPI3MR_HOSTTAG_EVTACKCMD_MIN + i);
> +
>  	if (pdev->revision)
>  		mrioc->enable_segqueue = true;
>  

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] scsi: mpi3mr: fix bitmap memory size calculation

[Damien Le Moal]

On 12/12/22 14:35, Damien Le Moal wrote:
> On 12/12/22 10:57, Shin'ichiro Kawasaki wrote:
>> To allocate memory for bitmaps, the mpi3mr driver calculates sizes of
>> each bitmap using byte as unit. However, bit operation helper functions
>> assume that bitmaps are allocated using unsigned long as unit. This gap
>> causes memory access beyond the bitmap memory size and results in "BUG:
>> KASAN: slab-out-of-bounds". The BUG was observed at firmware download to
>> eHBA-9600. Call trace indicated that the out-of-bounds access happened
>> in find_first_zero_bit called from mpi3mr_send_event_ack for the bitmap
>> miroc->evtack_cmds_bitmap.
>>
>> To avoid the BUG, fix bitmap size calculations using unsigned long as
>> unit. Apply this fix to five places to cover all bitmap size
>> calculations in the driver.
>>
>> Fixes: c5758fc72b92 ("scsi: mpi3mr: Gracefully handle online FW update operation")
>> Fixes: e844adb1fbdc ("scsi: mpi3mr: Implement SCSI error handler hooks")
>> Fixes: c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
>> Fixes: 824a156633df ("scsi: mpi3mr: Base driver code")
>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>> ---
>>  drivers/scsi/mpi3mr/mpi3mr_fw.c | 25 ++++++++++---------------
>>  1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
>> index 0c4aabaefdcc..272c318387b7 100644
>> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
>> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
>> @@ -1160,9 +1160,8 @@ mpi3mr_revalidate_factsdata(struct mpi3mr_ioc *mrioc)
>>  		    "\tcontroller while sas transport support is enabled at the\n"
>>  		    "\tdriver, please reboot the system or reload the driver\n");
>>  
>> -	dev_handle_bitmap_sz = mrioc->facts.max_devhandle / 8;
>> -	if (mrioc->facts.max_devhandle % 8)
>> -		dev_handle_bitmap_sz++;
>> +	dev_handle_bitmap_sz = sizeof(unsigned long) *
>> +		DIV_ROUND_UP(mrioc->facts.max_devhandle, BITS_PER_LONG);
>>  	if (dev_handle_bitmap_sz > mrioc->dev_handle_bitmap_sz) {
>>  		removepend_bitmap = krealloc(mrioc->removepend_bitmap,
>>  		    dev_handle_bitmap_sz, GFP_KERNEL);
>> @@ -2957,25 +2956,22 @@ static int mpi3mr_alloc_reply_sense_bufs(struct mpi3mr_ioc *mrioc)
>>  	if (!mrioc->pel_abort_cmd.reply)
>>  		goto out_failed;
>>  
>> -	mrioc->dev_handle_bitmap_sz = mrioc->facts.max_devhandle / 8;
>> -	if (mrioc->facts.max_devhandle % 8)
>> -		mrioc->dev_handle_bitmap_sz++;
> 
> mrioc->dev_handle_bitmap_sz = (mrioc->facts.max_devhandle + 7) >> 3;
> 
> would be a lot simpler...

Actually, if the code is changed to use bitmap_zalloc(), no rounding up to
8 or BITS_PER_LONG is needed at all, so the code would be really simpler.

> 
>> +	mrioc->dev_handle_bitmap_sz = sizeof(unsigned long) *
>> +		DIV_ROUND_UP(mrioc->facts.max_devhandle, BITS_PER_LONG);
>>  	mrioc->removepend_bitmap = kzalloc(mrioc->dev_handle_bitmap_sz,
>>  	    GFP_KERNEL);
> 
> What about using bitmap_alloc() here and keep the dev_handle_bitmap_sz
> value as is ?
> 
> (same for the other bitmaps)
> 
>>  	if (!mrioc->removepend_bitmap)
>>  		goto out_failed;
>>  
>> -	mrioc->devrem_bitmap_sz = MPI3MR_NUM_DEVRMCMD / 8;
>> -	if (MPI3MR_NUM_DEVRMCMD % 8)
>> -		mrioc->devrem_bitmap_sz++;
>> +	mrioc->devrem_bitmap_sz = sizeof(unsigned long) *
>> +		DIV_ROUND_UP(MPI3MR_NUM_DEVRMCMD, BITS_PER_LONG);
>>  	mrioc->devrem_bitmap = kzalloc(mrioc->devrem_bitmap_sz,
>>  	    GFP_KERNEL);
>>  	if (!mrioc->devrem_bitmap)
>>  		goto out_failed;
>>  
>> -	mrioc->evtack_cmds_bitmap_sz = MPI3MR_NUM_EVTACKCMD / 8;
>> -	if (MPI3MR_NUM_EVTACKCMD % 8)
>> -		mrioc->evtack_cmds_bitmap_sz++;
>> +	mrioc->evtack_cmds_bitmap_sz = sizeof(unsigned long) *
>> +		DIV_ROUND_UP(MPI3MR_NUM_EVTACKCMD, BITS_PER_LONG);
>>  	mrioc->evtack_cmds_bitmap = kzalloc(mrioc->evtack_cmds_bitmap_sz,
>>  	    GFP_KERNEL);
>>  	if (!mrioc->evtack_cmds_bitmap)
>> @@ -3415,9 +3411,8 @@ static int mpi3mr_alloc_chain_bufs(struct mpi3mr_ioc *mrioc)
>>  		if (!mrioc->chain_sgl_list[i].addr)
>>  			goto out_failed;
>>  	}
>> -	mrioc->chain_bitmap_sz = num_chains / 8;
>> -	if (num_chains % 8)
>> -		mrioc->chain_bitmap_sz++;
>> +	mrioc->chain_bitmap_sz = sizeof(unsigned long) *
>> +		DIV_ROUND_UP(num_chains, BITS_PER_LONG);
>>  	mrioc->chain_bitmap = kzalloc(mrioc->chain_bitmap_sz, GFP_KERNEL);
>>  	if (!mrioc->chain_bitmap)
>>  		goto out_failed;
> 

-- 
Damien Le Moal
Western Digital Research

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

[Shinichiro Kawasaki]

On Dec 12, 2022 / 14:27, Damien Le Moal wrote:
> 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 ?

Thanks. Will refelect to v2.

> 
> > +			       __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
> 

-- 
Shin'ichiro Kawasaki

Re: [PATCH 2/3] scsi: mpi3mr: fix bitmap memory size calculation

[Shinichiro Kawasaki]

On Dec 12, 2022 / 14:46, Damien Le Moal wrote:
> On 12/12/22 14:35, Damien Le Moal wrote:
> > On 12/12/22 10:57, Shin'ichiro Kawasaki wrote:
> >> To allocate memory for bitmaps, the mpi3mr driver calculates sizes of
> >> each bitmap using byte as unit. However, bit operation helper functions
> >> assume that bitmaps are allocated using unsigned long as unit. This gap
> >> causes memory access beyond the bitmap memory size and results in "BUG:
> >> KASAN: slab-out-of-bounds". The BUG was observed at firmware download to
> >> eHBA-9600. Call trace indicated that the out-of-bounds access happened
> >> in find_first_zero_bit called from mpi3mr_send_event_ack for the bitmap
> >> miroc->evtack_cmds_bitmap.
> >>
> >> To avoid the BUG, fix bitmap size calculations using unsigned long as
> >> unit. Apply this fix to five places to cover all bitmap size
> >> calculations in the driver.
> >>
> >> Fixes: c5758fc72b92 ("scsi: mpi3mr: Gracefully handle online FW update operation")
> >> Fixes: e844adb1fbdc ("scsi: mpi3mr: Implement SCSI error handler hooks")
> >> Fixes: c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
> >> Fixes: 824a156633df ("scsi: mpi3mr: Base driver code")
> >> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> >> ---
> >>  drivers/scsi/mpi3mr/mpi3mr_fw.c | 25 ++++++++++---------------
> >>  1 file changed, 10 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> >> index 0c4aabaefdcc..272c318387b7 100644
> >> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> >> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> >> @@ -1160,9 +1160,8 @@ mpi3mr_revalidate_factsdata(struct mpi3mr_ioc *mrioc)
> >>  		    "\tcontroller while sas transport support is enabled at the\n"
> >>  		    "\tdriver, please reboot the system or reload the driver\n");
> >>  
> >> -	dev_handle_bitmap_sz = mrioc->facts.max_devhandle / 8;
> >> -	if (mrioc->facts.max_devhandle % 8)
> >> -		dev_handle_bitmap_sz++;
> >> +	dev_handle_bitmap_sz = sizeof(unsigned long) *
> >> +		DIV_ROUND_UP(mrioc->facts.max_devhandle, BITS_PER_LONG);
> >>  	if (dev_handle_bitmap_sz > mrioc->dev_handle_bitmap_sz) {
> >>  		removepend_bitmap = krealloc(mrioc->removepend_bitmap,
> >>  		    dev_handle_bitmap_sz, GFP_KERNEL);
> >> @@ -2957,25 +2956,22 @@ static int mpi3mr_alloc_reply_sense_bufs(struct mpi3mr_ioc *mrioc)
> >>  	if (!mrioc->pel_abort_cmd.reply)
> >>  		goto out_failed;
> >>  
> >> -	mrioc->dev_handle_bitmap_sz = mrioc->facts.max_devhandle / 8;
> >> -	if (mrioc->facts.max_devhandle % 8)
> >> -		mrioc->dev_handle_bitmap_sz++;
> > 
> > mrioc->dev_handle_bitmap_sz = (mrioc->facts.max_devhandle + 7) >> 3;
> > 
> > would be a lot simpler...
> 
> Actually, if the code is changed to use bitmap_zalloc(), no rounding up to
> 8 or BITS_PER_LONG is needed at all, so the code would be really simpler.

Ah, yes, I agree that bitmap_zalloc() is the simplest way.

> 
> > 
> >> +	mrioc->dev_handle_bitmap_sz = sizeof(unsigned long) *
> >> +		DIV_ROUND_UP(mrioc->facts.max_devhandle, BITS_PER_LONG);
> >>  	mrioc->removepend_bitmap = kzalloc(mrioc->dev_handle_bitmap_sz,
> >>  	    GFP_KERNEL);
> > 
> > What about using bitmap_alloc() here and keep the dev_handle_bitmap_sz
> > value as is ?
> > 
> > (same for the other bitmaps)

It does not look good for me to keep *_bitmap_sz values as they are. This
driver uses various *_bitmap_sz to keep size of bitmaps in byte unit, and they
are passed for kzalloc(), krealloc() and memset(). It is not clear for me if all
of the function calls are ok with numbers unaligned to sizeof(unsigned long).

Instead, I suggest to manage bitmap size using number of bits. The kzalloc(),
krealloc() and memset() calls can be replaced with bitmap helper functions
bitmap_salloc(), bitmap_copy() and bitmap_clear() which take number of bits as
arguments. In this way, the driver no longer needs to manage bitmap size in
bytes. I'll prepare v2 patch with this approach.

-- 
Shin'ichiro Kawasaki