diff mbox series

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

Message ID 20221212015706.2609544-3-shinichiro.kawasaki@wdc.com
State New
Headers show
Series scsi: mpi3mr: fix issues found by KASAN | expand

Commit Message

Shinichiro Kawasaki Dec. 12, 2022, 1:57 a.m. UTC 
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(-)

Comments

Shinichiro Kawasaki Dec. 12, 2022, 12:15 p.m. UTC | #1 
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.
diff mbox series

Patch

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;