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