Re: [PATCH v4 4/5] scsi: mpi3mr: use number of bits to manage bitmap sizes

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

On Feb 09, 2023 / 12:50, Sathya Prakash Veerichetty wrote:
> On Thu, Jan 26, 2023 at 11:35 PM Shin'ichiro Kawasaki
> <shinichiro.kawasaki@wdc.com> 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@opensource.wdc.com>
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >  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