[PATCH v2 0/3] scsi: mpi3mr: fix issues found by KASAN

[PATCH v2 0/3] scsi: mpi3mr: fix issues found by KASAN

[Shin'ichiro Kawasaki]

While I downloaded new firmware to eHBA-9600 on KASAN enabled kernel,
I observed three BUGs. This series addresses them.

Changes from v1:
* 2nd patch: Modified to use bitmap helper functions and number of bits
* 1st/3rd patches: Reflected a comment on the list and added Reviewed-by tags

Shin'ichiro Kawasaki (3):
  scsi: mpi3mr: fix alltgt_info copy size in mpi3mr_get_all_tgt_info
  scsi: mpi3mr: use number of bits to manage bitmap sizes
  scsi: mpi3mr: fix missing mrioc->evtack_cmds initialization

 drivers/scsi/mpi3mr/mpi3mr.h     | 10 +----
 drivers/scsi/mpi3mr/mpi3mr_app.c |  9 ++++-
 drivers/scsi/mpi3mr/mpi3mr_fw.c  | 68 +++++++++++++-------------------
 drivers/scsi/mpi3mr/mpi3mr_os.c  |  4 ++
 4 files changed, 42 insertions(+), 49 deletions(-)

-- 
2.37.1

[PATCH v2 1/3] scsi: mpi3mr: fix alltgt_info copy size in mpi3mr_get_all_tgt_info

[Shin'ichiro Kawasaki]

The function mpi3mr_get_all_tgt_info calculates size of alltgt_info and
allocate memory for it. After preparing valid data in alltgt_info, it
calls sg_copy_from_buffer to copy alltgt_info to job->request_payload,
specifying length of the payload as copy length. This length is larger
than the calculated alltgt_info size. It causes memory access to invalid
address and results in "BUG: KASAN: slab-out-of-bounds". The BUG was
observed during boot using systems with eHBA-9600. By updating the HBA
firmware to latest version 8.3.1.0 the BUG was not observed during boot,
but still observed when command "storcli2 /c0 show" is executed.

Fix the BUG by specifying the calculated alltgt_info size as copy
length. Also check that the copy destination payload length is larger
than the copy length.

Fixes: f5e6d5a34376 ("scsi: mpi3mr: Add support for driver commands")
Cc: stable@vger.kernel.org
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/mpi3mr/mpi3mr_app.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
index 9baac224b213..2e35b0fece9c 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_app.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
@@ -322,6 +322,13 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
 
 	kern_entrylen = (num_devices - 1) * sizeof(*devmap_info);
 	size = sizeof(*alltgt_info) + kern_entrylen;
+
+	if (size > job->request_payload.payload_len) {
+		dprint_bsg_err(mrioc, "%s: payload length is too small\n",
+			       __func__);
+		return rval;
+	}
+
 	alltgt_info = kzalloc(size, GFP_KERNEL);
 	if (!alltgt_info)
 		return -ENOMEM;
@@ -358,7 +365,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, size);
 	rval = 0;
 out:
 	kfree(alltgt_info);
-- 
2.37.1

[PATCH v2 2/3] scsi: mpi3mr: use number of bits to manage bitmap sizes

[Shin'ichiro Kawasaki]

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.

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>
---
 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 0c4aabaefdcc..8ed4566772ca 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) {
+		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);
 		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 +2955,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 +3404,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;
@@ -4190,10 +4176,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++) {
@@ -4887,9 +4874,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.37.1

[PATCH v2 3/3] scsi: mpi3mr: fix missing mrioc->evtack_cmds initialization

[Shin'ichiro Kawasaki]

The commit c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
introduced an array mrioc->evtack_cmds. But initialization of the array
elements was missed. They are just zero cleared. The function
mpi3mr_complete_evt_ack refers host_tag field of the elements. Due to
zero value of the host_tag field, the functions calls clear_bit for
mrico->evtack_cmds_bitmap with wrong bit index. This results in memory
access to invalid address and "BUG: KASAN: use-after-free". This BUG was
observed at eHBA-9600 firmware update to version 8.3.1.0. To fix it, add
the missing initialization of mrioc->evtack_cmds.

Fixes: c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
Cc: stable@vger.kernel.org
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/mpi3mr/mpi3mr_os.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 3306de7170f6..6eaeba41072c 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -4952,6 +4952,10 @@ mpi3mr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		mpi3mr_init_drv_cmd(&mrioc->dev_rmhs_cmds[i],
 		    MPI3MR_HOSTTAG_DEVRMCMD_MIN + i);
 
+	for (i = 0; i < MPI3MR_NUM_EVTACKCMD; i++)
+		mpi3mr_init_drv_cmd(&mrioc->evtack_cmds[i],
+				    MPI3MR_HOSTTAG_EVTACKCMD_MIN + i);
+
 	if (pdev->revision)
 		mrioc->enable_segqueue = true;
 
-- 
2.37.1

Re: [PATCH v2 2/3] scsi: mpi3mr: use number of bits to manage bitmap sizes

[Damien Le Moal]

On 12/13/22 09:52, Shin'ichiro Kawasaki 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.
> 
> 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>

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/3] scsi: mpi3mr: fix missing mrioc->evtack_cmds initialization

[Sathya Prakash Veerichetty]

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]

On Mon, Dec 12, 2022 at 5:52 PM Shin'ichiro Kawasaki
<shinichiro.kawasaki@wdc.com> wrote:
>
> The commit c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
> introduced an array mrioc->evtack_cmds. But initialization of the array
> elements was missed. They are just zero cleared. The function
> mpi3mr_complete_evt_ack refers host_tag field of the elements. Due to
> zero value of the host_tag field, the functions calls clear_bit for
> mrico->evtack_cmds_bitmap with wrong bit index. This results in memory
> access to invalid address and "BUG: KASAN: use-after-free". This BUG was
> observed at eHBA-9600 firmware update to version 8.3.1.0. To fix it, add
> the missing initialization of mrioc->evtack_cmds.
>
> Fixes: c1af985d27da ("scsi: mpi3mr: Add Event acknowledgment logic")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/scsi/mpi3mr/mpi3mr_os.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 3306de7170f6..6eaeba41072c 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -4952,6 +4952,10 @@ mpi3mr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>                 mpi3mr_init_drv_cmd(&mrioc->dev_rmhs_cmds[i],
>                     MPI3MR_HOSTTAG_DEVRMCMD_MIN + i);
>
> +       for (i = 0; i < MPI3MR_NUM_EVTACKCMD; i++)
> +               mpi3mr_init_drv_cmd(&mrioc->evtack_cmds[i],
> +                                   MPI3MR_HOSTTAG_EVTACKCMD_MIN + i);
> +
>         if (pdev->revision)
>                 mrioc->enable_segqueue = true;
>
> --
> 2.37.1
>
Acked-by: Sathya Prakash Veerichetty <sathya.prakash@broadcom.com>

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4227 bytes --]

Re: [PATCH v2 2/3] scsi: mpi3mr: use number of bits to manage bitmap sizes

[Sathya Prakash Veerichetty]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

On Mon, Dec 12, 2022 at 9:12 PM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 12/13/22 09:52, Shin'ichiro Kawasaki 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.
> >
> > 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>
>
> Looks good to me.
>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>
> --
> Damien Le Moal
> Western Digital Research
>
The changes look good, however, I will need sometime to test this
patch locally, Will ACK it sometime during the winter break.

-Sathya

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4227 bytes --]

Re: [PATCH v2 1/3] scsi: mpi3mr: fix alltgt_info copy size in mpi3mr_get_all_tgt_info

[Sathya Prakash Veerichetty]

[-- Attachment #1: Type: text/plain, Size: 3481 bytes --]

On Mon, Dec 12, 2022 at 5:52 PM Shin'ichiro Kawasaki
<shinichiro.kawasaki@wdc.com> wrote:
>
> The function mpi3mr_get_all_tgt_info calculates size of alltgt_info and
> allocate memory for it. After preparing valid data in alltgt_info, it
> calls sg_copy_from_buffer to copy alltgt_info to job->request_payload,
> specifying length of the payload as copy length. This length is larger
> than the calculated alltgt_info size. It causes memory access to invalid
> address and results in "BUG: KASAN: slab-out-of-bounds". The BUG was
> observed during boot using systems with eHBA-9600. By updating the HBA
> firmware to latest version 8.3.1.0 the BUG was not observed during boot,
> but still observed when command "storcli2 /c0 show" is executed.
>
> Fix the BUG by specifying the calculated alltgt_info size as copy
> length. Also check that the copy destination payload length is larger
> than the copy length.
>
> Fixes: f5e6d5a34376 ("scsi: mpi3mr: Add support for driver commands")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Thanks for the patch, though this code needs a fix, the changes are
not correct and needs modification.
> ---
>  drivers/scsi/mpi3mr/mpi3mr_app.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 9baac224b213..2e35b0fece9c 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -322,6 +322,13 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
>
>         kern_entrylen = (num_devices - 1) * sizeof(*devmap_info);
>         size = sizeof(*alltgt_info) + kern_entrylen;
> +
> +       if (size > job->request_payload.payload_len) {
> +               dprint_bsg_err(mrioc, "%s: payload length is too small\n",
> +                              __func__);
> +               return rval;
> +       }
> +
This check is not needed, this is already handled by reducing the size
to be copied to the given payload size
>         alltgt_info = kzalloc(size, GFP_KERNEL);
>         if (!alltgt_info)
>                 return -ENOMEM;
> @@ -358,7 +365,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, size);
instead of size, this should be min_entry_len+sizeof(u32).
>         rval = 0;
>  out:
>         kfree(alltgt_info);
> --
> 2.37.1
>

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4227 bytes --]

Re: [PATCH v2 1/3] scsi: mpi3mr: fix alltgt_info copy size in mpi3mr_get_all_tgt_info

[Shinichiro Kawasaki]

Hello Sathya, thanks for the comment.

On Dec 12, 2022 / 22:38, Sathya Prakash Veerichetty wrote:
> On Mon, Dec 12, 2022 at 5:52 PM Shin'ichiro Kawasaki
> <shinichiro.kawasaki@wdc.com> wrote:
> >
> > The function mpi3mr_get_all_tgt_info calculates size of alltgt_info and
> > allocate memory for it. After preparing valid data in alltgt_info, it
> > calls sg_copy_from_buffer to copy alltgt_info to job->request_payload,
> > specifying length of the payload as copy length. This length is larger
> > than the calculated alltgt_info size. It causes memory access to invalid
> > address and results in "BUG: KASAN: slab-out-of-bounds". The BUG was
> > observed during boot using systems with eHBA-9600. By updating the HBA
> > firmware to latest version 8.3.1.0 the BUG was not observed during boot,
> > but still observed when command "storcli2 /c0 show" is executed.
> >
> > Fix the BUG by specifying the calculated alltgt_info size as copy
> > length. Also check that the copy destination payload length is larger
> > than the copy length.
> >
> > Fixes: f5e6d5a34376 ("scsi: mpi3mr: Add support for driver commands")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 
> Thanks for the patch, though this code needs a fix, the changes are
> not correct and needs modification.
> > ---
> >  drivers/scsi/mpi3mr/mpi3mr_app.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> > index 9baac224b213..2e35b0fece9c 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> > @@ -322,6 +322,13 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
> >
> >         kern_entrylen = (num_devices - 1) * sizeof(*devmap_info);
> >         size = sizeof(*alltgt_info) + kern_entrylen;
> > +
> > +       if (size > job->request_payload.payload_len) {
> > +               dprint_bsg_err(mrioc, "%s: payload length is too small\n",
> > +                              __func__);
> > +               return rval;
> > +       }
> > +
> This check is not needed, this is already handled by reducing the size
> to be copied to the given payload size

Ah, I see that min_entrylen is prepared to copy bytes smaller than the payload
size.

> >         alltgt_info = kzalloc(size, GFP_KERNEL);
> >         if (!alltgt_info)
> >                 return -ENOMEM;
> > @@ -358,7 +365,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, size);
> instead of size, this should be min_entry_len+sizeof(u32).

Thanks for the comment. I read through mpi3mr_get_all_tgt_info() again. I still
have three unclear points. Your comments on them will be appreciated.

1) copy length

The pointer alltgt_info points to the struct below, which is defined in
include/uapi/scsi/scsi_bsg_mpi3mr.h (I refer kernel code at v6.1):

struct mpi3mr_all_tgt_info {
	__u16	num_devices;
	__u16	rsvd1;
	__u32	rsvd2;
	struct mpi3mr_device_map_info dmi[1];
};

When we copy "min_entrylen+sizeof(u32)", it looks for me that the struct is
copied partially. The expected length is as follows, isn't it?

  "min_entrylen + sizeof(u16) + sizeof(u16) + sizeof(u32)"

Regarding the min_entrylen, I find code in mpi3mr_get_all_tgt_info:

	usr_entrylen = (job->request_payload.payload_len - sizeof(u32)) / sizeof(*devmap_info);
	usr_entrylen *= sizeof(*devmap_info);
	min_entrylen = min(usr_entrylen, kern_entrylen);

The usr_entrylen calculation subtracts sizeof(u32). I guess the line also
needs change to subtract sizeof(u16) + sizeof(u16) + sizeof(u32).


2) kern_entrylen

usr_entrylen is compared with kern_entrylen to get min_etnrylen. And
kern_entrylen covers (num_devices - 1) entries:

	kern_entrylen = (num_devices - 1) * sizeof(*devmap_info);
	size = sizeof(*alltgt_info) + kern_entrylen;

Is it ok to cover only (num_devices - 1) for comparison with usr_entrylen?
Don't we need to cover all num_devices?


3) memcpy from devmap_info to alltgt_info->dmi

Also regarding the min_entrylen, I find a line below:

	if (min_entrylen && (!memcpy(&alltgt_info->dmi, devmap_info, min_entrylen))) {

The memcpy copies data from devmap_info to alltgt_inf->dmi, but it looks for me
that these two points to same address. Do we really need this memcpy?


I'm new to the mpi3mr driver. If I overlook or misunderstand anything, please
let me know.

-- 
Shin'ichiro Kawasaki

Re: [PATCH v2 1/3] scsi: mpi3mr: fix alltgt_info copy size in mpi3mr_get_all_tgt_info

[Shinichiro Kawasaki]

On Dec 13, 2022 / 07:17, Shinichiro Kawasaki wrote:
> Hello Sathya, thanks for the comment.
> 
> On Dec 12, 2022 / 22:38, Sathya Prakash Veerichetty wrote:
> > On Mon, Dec 12, 2022 at 5:52 PM Shin'ichiro Kawasaki

[...]

> > >         alltgt_info = kzalloc(size, GFP_KERNEL);
> > >         if (!alltgt_info)
> > >                 return -ENOMEM;
> > > @@ -358,7 +365,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, size);
> > instead of size, this should be min_entry_len+sizeof(u32).
> 
> Thanks for the comment. I read through mpi3mr_get_all_tgt_info() again. I still
> have three unclear points. Your comments on them will be appreciated.

I've posted v3 using min_entrylen as you suggested. Also I added two more
patches to the series, based on my understanding on the three points I had
noted. Your review will be appreciated.

-- 
Shin'ichiro Kawasaki