| Message ID | 20230214005019.1897251-1-shinichiro.kawasaki@wdc.com |
|---|---|
| Headers | show |
| Series | scsi: mpi3mr: fix issues found by KASAN | expand |
On Mon, Feb 13, 2023 at 5:50 PM Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> wrote: > > The function mpi3mr_get_all_tgt_info has four issues as follow. > > 1) It calculates valid entry length in alltgt_info assuming the header > part of the struct mpi3mr_device_map_info would equal to sizeof(u32). > The correct size is sizeof(u64). > 2) When it calculates the valid entry length kern_entrylen, it excludes > one entry by subtracting 1 from num_devices. > 3) It copies num_device by calling memcpy. Substitution is enough. > 4) It does not specify the calculated length to sg_copy_from_buffer(). > Instead, it specifies the payload length which is larger than the > alltgt_info size. It causes "BUG: KASAN: slab-out-of-bounds". > > Fix the issues by using the correct header size, removing the subtract > from num_devices, replacing the memcpy with substitution and specifying > the correct length to sg_copy_from_buffer. > > Fixes: f5e6d5a34376 ("scsi: mpi3mr: Add support for driver commands") > Cc: stable@vger.kernel.org > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Acked-by: Sathya Prakash Veerichetty <sathya.prakash@broadcom.com> > > --- > drivers/scsi/mpi3mr/mpi3mr_app.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c > index 9baac224b213..72054e3a26cb 100644 > --- a/drivers/scsi/mpi3mr/mpi3mr_app.c > +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c > @@ -312,7 +312,7 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, > num_devices++; > spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags); > > - if ((job->request_payload.payload_len == sizeof(u32)) || > + if ((job->request_payload.payload_len <= sizeof(u64)) || > list_empty(&mrioc->tgtdev_list)) { > sg_copy_from_buffer(job->request_payload.sg_list, > job->request_payload.sg_cnt, > @@ -320,14 +320,14 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, > return 0; > } > > - kern_entrylen = (num_devices - 1) * sizeof(*devmap_info); > - size = sizeof(*alltgt_info) + kern_entrylen; > + kern_entrylen = num_devices * sizeof(*devmap_info); > + size = sizeof(u64) + kern_entrylen; > alltgt_info = kzalloc(size, GFP_KERNEL); > if (!alltgt_info) > return -ENOMEM; > > devmap_info = alltgt_info->dmi; > - memset((u8 *)devmap_info, 0xFF, (kern_entrylen + sizeof(*devmap_info))); > + memset((u8 *)devmap_info, 0xFF, kern_entrylen); > spin_lock_irqsave(&mrioc->tgtdev_lock, flags); > list_for_each_entry(tgtdev, &mrioc->tgtdev_list, list) { > if (i < num_devices) { > @@ -344,9 +344,10 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, > num_devices = i; > spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags); > > - memcpy(&alltgt_info->num_devices, &num_devices, sizeof(num_devices)); > + alltgt_info->num_devices = num_devices; > > - usr_entrylen = (job->request_payload.payload_len - sizeof(u32)) / sizeof(*devmap_info); > + usr_entrylen = (job->request_payload.payload_len - sizeof(u64)) / > + sizeof(*devmap_info); > usr_entrylen *= sizeof(*devmap_info); > min_entrylen = min(usr_entrylen, kern_entrylen); > if (min_entrylen && (!memcpy(&alltgt_info->dmi, devmap_info, min_entrylen))) { > @@ -358,7 +359,7 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, > > sg_copy_from_buffer(job->request_payload.sg_list, > job->request_payload.sg_cnt, > - alltgt_info, job->request_payload.payload_len); > + alltgt_info, (min_entrylen + sizeof(u64))); > rval = 0; > out: > kfree(alltgt_info); > -- > 2.38.1 >
On Mon, Feb 13, 2023 at 5:50 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 and krealloc. For memory free, call bitmap_free instead of > kfree. For zero clear, call bitmap_clear instead of memset. > > 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. > > 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> Acked-by: Sathya Prakash Veerichetty <sathya.prakash@broadcom.com> > --- > drivers/scsi/mpi3mr/mpi3mr.h | 10 +---- > drivers/scsi/mpi3mr/mpi3mr_fw.c | 75 ++++++++++++++------------------- > 2 files changed, 33 insertions(+), 52 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..758f7ca9e0ee 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,23 @@ 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) { > + 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_free(mrioc->removepend_bitmap); > mrioc->removepend_bitmap = removepend_bitmap; > ioc_info(mrioc, > - "increased dev_handle_bitmap_sz from %d to %d\n", > - mrioc->dev_handle_bitmap_sz, dev_handle_bitmap_sz); > - mrioc->dev_handle_bitmap_sz = dev_handle_bitmap_sz; > + "increased bits of dev_handle_bitmap from %d to %d\n", > + mrioc->dev_handle_bitmap_bits, > + mrioc->facts.max_devhandle); > + mrioc->dev_handle_bitmap_bits = mrioc->facts.max_devhandle; > } > > return 0; > @@ -2957,27 +2954,18 @@ 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->removepend_bitmap = kzalloc(mrioc->dev_handle_bitmap_sz, > - GFP_KERNEL); > + mrioc->dev_handle_bitmap_bits = mrioc->facts.max_devhandle; > + mrioc->removepend_bitmap = bitmap_zalloc(mrioc->dev_handle_bitmap_bits, > + GFP_KERNEL); > 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 = kzalloc(mrioc->devrem_bitmap_sz, > - GFP_KERNEL); > + mrioc->devrem_bitmap = bitmap_zalloc(MPI3MR_NUM_DEVRMCMD, 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 = kzalloc(mrioc->evtack_cmds_bitmap_sz, > - GFP_KERNEL); > + mrioc->evtack_cmds_bitmap = bitmap_zalloc(MPI3MR_NUM_EVTACKCMD, > + GFP_KERNEL); > if (!mrioc->evtack_cmds_bitmap) > goto out_failed; > > @@ -3415,10 +3403,7 @@ 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 = kzalloc(mrioc->chain_bitmap_sz, GFP_KERNEL); > + mrioc->chain_bitmap = bitmap_zalloc(num_chains, GFP_KERNEL); > if (!mrioc->chain_bitmap) > goto out_failed; > return retval; > @@ -4189,10 +4174,11 @@ void mpi3mr_memset_buffers(struct mpi3mr_ioc *mrioc) > for (i = 0; i < MPI3MR_NUM_EVTACKCMD; i++) > memset(mrioc->evtack_cmds[i].reply, 0, > sizeof(*mrioc->evtack_cmds[i].reply)); > - memset(mrioc->removepend_bitmap, 0, mrioc->dev_handle_bitmap_sz); > - memset(mrioc->devrem_bitmap, 0, mrioc->devrem_bitmap_sz); > - memset(mrioc->evtack_cmds_bitmap, 0, > - mrioc->evtack_cmds_bitmap_sz); > + bitmap_clear(mrioc->removepend_bitmap, 0, > + mrioc->dev_handle_bitmap_bits); > + bitmap_clear(mrioc->devrem_bitmap, 0, MPI3MR_NUM_DEVRMCMD); > + bitmap_clear(mrioc->evtack_cmds_bitmap, 0, > + MPI3MR_NUM_EVTACKCMD); > } > > for (i = 0; i < mrioc->num_queues; i++) { > @@ -4318,16 +4304,16 @@ void mpi3mr_free_mem(struct mpi3mr_ioc *mrioc) > mrioc->evtack_cmds[i].reply = NULL; > } > > - kfree(mrioc->removepend_bitmap); > + bitmap_free(mrioc->removepend_bitmap); > mrioc->removepend_bitmap = NULL; > > - kfree(mrioc->devrem_bitmap); > + bitmap_free(mrioc->devrem_bitmap); > mrioc->devrem_bitmap = NULL; > > - kfree(mrioc->evtack_cmds_bitmap); > + bitmap_free(mrioc->evtack_cmds_bitmap); > mrioc->evtack_cmds_bitmap = NULL; > > - kfree(mrioc->chain_bitmap); > + bitmap_free(mrioc->chain_bitmap); > mrioc->chain_bitmap = NULL; > > kfree(mrioc->transport_cmds.reply); > @@ -4886,9 +4872,10 @@ int mpi3mr_soft_reset_handler(struct mpi3mr_ioc *mrioc, > > mpi3mr_flush_delayed_cmd_lists(mrioc); > mpi3mr_flush_drv_cmds(mrioc); > - memset(mrioc->devrem_bitmap, 0, mrioc->devrem_bitmap_sz); > - memset(mrioc->removepend_bitmap, 0, mrioc->dev_handle_bitmap_sz); > - memset(mrioc->evtack_cmds_bitmap, 0, mrioc->evtack_cmds_bitmap_sz); > + bitmap_clear(mrioc->devrem_bitmap, 0, MPI3MR_NUM_DEVRMCMD); > + bitmap_clear(mrioc->removepend_bitmap, 0, > + mrioc->dev_handle_bitmap_bits); > + bitmap_clear(mrioc->evtack_cmds_bitmap, 0, MPI3MR_NUM_EVTACKCMD); > mpi3mr_flush_host_io(mrioc); > mpi3mr_cleanup_fwevt_list(mrioc); > mpi3mr_invalidate_devhandles(mrioc); > -- > 2.38.1 >