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

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

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