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@xxxxxxx>
>> ---
>> 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
