On Feb 09, 2023 / 12:50, Sathya Prakash Veerichetty wrote:
> On Thu, Jan 26, 2023 at 11:35 PM Shin'ichiro Kawasaki
> <shinichiro.kawasaki@xxxxxxx> wrote:
> >
> > To allocate bitmaps, the mpi3mr driver calculates sizes of bitmaps using
> > byte as unit. However, bitmap helper functions assume that bitmaps are
> > allocated using unsigned long as unit. This gap causes memory access
> > beyond the bitmap sizes 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 miroc->evtack_cmds_bitmap.
> >
> > To fix the BUG, do not use bytes to manage bitmap sizes. Instead, use
> > number of bits, and call bitmap helper functions which take number of
> > bits as arguments. For memory allocation, call bitmap_zalloc instead of
> > kzalloc. For zero clear, call bitmap_clear instead of memset. For
> > resize, call bitmap_zalloc and bitmap_copy instead of krealloc.
> >
> > Remove three fields for bitmap byte sizes in struct scmd_priv, which are
> > no longer required. Replace the field dev_handle_bitmap_sz with
> > dev_handle_bitmap_bits to keep number of bits of removepend_bitmap
> > across resize.
> >
> >>Thanks for getting this changed, can you please change the kfree for the bitmaps to bitmap_free for consistency of the API.
> > 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")
> > Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> > ---
> > drivers/scsi/mpi3mr/mpi3mr.h | 10 +----
> > drivers/scsi/mpi3mr/mpi3mr_fw.c | 68 ++++++++++++++-------------------
> > 2 files changed, 30 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
> > index def4c5e15cd8..8a438f248a82 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr.h
> > +++ b/drivers/scsi/mpi3mr/mpi3mr.h
> > @@ -955,19 +955,16 @@ struct scmd_priv {
> > * @chain_buf_count: Chain buffer count
> > * @chain_buf_pool: Chain buffer pool
> > * @chain_sgl_list: Chain SGL list
> > - * @chain_bitmap_sz: Chain buffer allocator bitmap size
> > * @chain_bitmap: Chain buffer allocator bitmap
> > * @chain_buf_lock: Chain buffer list lock
> > * @bsg_cmds: Command tracker for BSG command
> > * @host_tm_cmds: Command tracker for task management commands
> > * @dev_rmhs_cmds: Command tracker for device removal commands
> > * @evtack_cmds: Command tracker for event ack commands
> > - * @devrem_bitmap_sz: Device removal bitmap size
> > * @devrem_bitmap: Device removal bitmap
> > - * @dev_handle_bitmap_sz: Device handle bitmap size
> > + * @dev_handle_bitmap_bits: Number of bits in device handle bitmap
> > * @removepend_bitmap: Remove pending bitmap
> > * @delayed_rmhs_list: Delayed device removal list
> > - * @evtack_cmds_bitmap_sz: Event Ack bitmap size
> > * @evtack_cmds_bitmap: Event Ack bitmap
> > * @delayed_evtack_cmds_list: Delayed event acknowledgment list
> > * @ts_update_counter: Timestamp update counter
> > @@ -1128,7 +1125,6 @@ struct mpi3mr_ioc {
> > u32 chain_buf_count;
> > struct dma_pool *chain_buf_pool;
> > struct chain_element *chain_sgl_list;
> > - u16 chain_bitmap_sz;
> > void *chain_bitmap;
> > spinlock_t chain_buf_lock;
> >
> > @@ -1136,12 +1132,10 @@ struct mpi3mr_ioc {
> > struct mpi3mr_drv_cmd host_tm_cmds;
> > struct mpi3mr_drv_cmd dev_rmhs_cmds[MPI3MR_NUM_DEVRMCMD];
> > struct mpi3mr_drv_cmd evtack_cmds[MPI3MR_NUM_EVTACKCMD];
> > - u16 devrem_bitmap_sz;
> > void *devrem_bitmap;
> > - u16 dev_handle_bitmap_sz;
> > + u16 dev_handle_bitmap_bits;
> > void *removepend_bitmap;
> > struct list_head delayed_rmhs_list;
> > - u16 evtack_cmds_bitmap_sz;
> > void *evtack_cmds_bitmap;
> > struct list_head delayed_evtack_cmds_list;
> >
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > index 286a44506578..d25cd0382e20 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > @@ -1128,7 +1128,6 @@ static int mpi3mr_issue_and_process_mur(struct mpi3mr_ioc *mrioc,
> > static int
> > mpi3mr_revalidate_factsdata(struct mpi3mr_ioc *mrioc)
> > {
> > - u16 dev_handle_bitmap_sz;
> > void *removepend_bitmap;
> >
> > if (mrioc->facts.reply_sz > mrioc->reply_sz) {
> > @@ -1160,25 +1159,24 @@ 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++;
> > - if (dev_handle_bitmap_sz > mrioc->dev_handle_bitmap_sz) {
> > - removepend_bitmap = krealloc(mrioc->removepend_bitmap,
> > - dev_handle_bitmap_sz, GFP_KERNEL);
> > + if (mrioc->facts.max_devhandle > mrioc->dev_handle_bitmap_bits) {
> >>Free the existing removepend_bitmap prior the alloc.
Thanks for catching this. The existing removepend_bitmap should be freed. I
think the free should be done after the alloc, since the alloc may fail. I'll
add bitmap_free after the bitmap_zalloc() result check.
> > + removepend_bitmap = bitmap_zalloc(mrioc->facts.max_devhandle,
> > + GFP_KERNEL);
> > if (!removepend_bitmap) {
> > ioc_err(mrioc,
> > - "failed to increase removepend_bitmap sz from: %d to %d\n",
> > - mrioc->dev_handle_bitmap_sz, dev_handle_bitmap_sz);
> > + "failed to increase removepend_bitmap bits from %d to %d\n",
> > + mrioc->dev_handle_bitmap_bits,
> > + mrioc->facts.max_devhandle);
> > return -EPERM;
> > }
> > - memset(removepend_bitmap + mrioc->dev_handle_bitmap_sz, 0,
> > - dev_handle_bitmap_sz - mrioc->dev_handle_bitmap_sz);
> > + bitmap_copy(removepend_bitmap, mrioc->removepend_bitmap,
> > + mrioc->dev_handle_bitmap_bits);
> >>This copy is not needed as the data in the removepend_bitmap is not valid after reset and the zalloc already cleared the memory.
Okay, will remove it.
--
Shin'ichiro Kawasaki
