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

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

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