* [PATCH v5 1/6] scsi: mvumi: bound the per-event parameter and sense dumps
2026-10-14 15:19 [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies Yehyeong Lee
@ 2026-10-14 15:19 ` Yehyeong Lee
2026-10-14 15:32 ` sashiko-bot
2026-10-14 15:19 ` [PATCH v5 2/6] scsi: mvumi: reject an out-of-range handshake frame length Yehyeong Lee
` (5 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Yehyeong Lee @ 2026-10-14 15:19 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, linux-kernel, stable, yhlee
mvumi_show_event() prints param_count u32 words from params[] and
sense_data_length bytes from sense_data[], but both counts come from the
firmware event (struct mvumi_driver_event) and are never checked against
the arrays, which hold four words and 30 bytes. A firmware that reports
the maximum (255 each) makes the loops read far past the event and past
the 512-byte buffer the event block is allocated in, printing the result
to the kernel log.
Stop each loop at the size of its array.
Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
drivers/scsi/mvumi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index e70d336b4ab39..38a92f8c25f5b 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -1361,7 +1361,8 @@ static void mvumi_show_event(struct mvumi_hba *mhba,
if (ptr->param_count) {
printk(KERN_WARNING "Event param(len 0x%x): ",
ptr->param_count);
- for (i = 0; i < ptr->param_count; i++)
+ for (i = 0; i < ptr->param_count &&
+ i < ARRAY_SIZE(ptr->params); i++)
printk(KERN_WARNING "0x%x ", ptr->params[i]);
printk(KERN_WARNING "\n");
@@ -1370,7 +1371,8 @@ static void mvumi_show_event(struct mvumi_hba *mhba,
if (ptr->sense_data_length) {
printk(KERN_WARNING "Event sense data(len 0x%x): ",
ptr->sense_data_length);
- for (i = 0; i < ptr->sense_data_length; i++)
+ for (i = 0; i < ptr->sense_data_length &&
+ i < sizeof(ptr->sense_data); i++)
printk(KERN_WARNING "0x%x ", ptr->sense_data[i]);
printk(KERN_WARNING "\n");
}
--
2.43.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 1/6] scsi: mvumi: bound the per-event parameter and sense dumps
2026-10-14 15:19 ` [PATCH v5 1/6] scsi: mvumi: bound the per-event parameter and sense dumps Yehyeong Lee
@ 2026-10-14 15:32 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-10-14 15:32 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mvumi: missing endianness conversion for firmware event fields
--
commit edf8fe53e2e86275574e11fc999d2a119ef7acd5
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
scsi: mvumi: bound the per-event parameter and sense dumps
This commit adds bounds checking to the event parameter and sense data
loops in mvumi_show_event() to prevent out-of-bounds reads when printing
firmware events to the system log.
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..38a92f8c25f5b 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -1361,7 +1361,8 @@ static void mvumi_show_event(struct mvumi_hba *mhba,
> if (ptr->param_count) {
> printk(KERN_WARNING "Event param(len 0x%x): ",
> ptr->param_count);
> - for (i = 0; i < ptr->param_count; i++)
> + for (i = 0; i < ptr->param_count &&
> + i < ARRAY_SIZE(ptr->params); i++)
> printk(KERN_WARNING "0x%x ", ptr->params[i]);
[Severity: Medium]
As this patch modifies the parameter array loop in mvumi_show_event(), does
this code miss an endianness conversion when reading from the hardware DMA
buffer?
The u32 parameter fields from the event payload (ptr->params[i]) are
printed natively. On Big-Endian architectures, this could lead to
byte-swapped values appearing in the system logs whenever the firmware
reports an event.
Additionally, just prior to this loop in mvumi_show_event(), other
multi-byte fields from the hardware event structure (sequence_no, event_id,
and device_id) are also printed natively. Can these fields cause garbage
values to be logged on Big-Endian systems if they are not converted using
functions like le32_to_cpu() or le16_to_cpu()?
>
> printk(KERN_WARNING "\n");
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261014151914.3632701-1-yhlee@isslab.korea.ac.kr?part=1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 2/6] scsi: mvumi: reject an out-of-range handshake frame length
2026-10-14 15:19 [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies Yehyeong Lee
2026-10-14 15:19 ` [PATCH v5 1/6] scsi: mvumi: bound the per-event parameter and sense dumps Yehyeong Lee
@ 2026-10-14 15:19 ` Yehyeong Lee
2026-10-14 15:33 ` sashiko-bot
2026-10-14 15:19 ` [PATCH v5 3/6] scsi: mvumi: bound the hotplug event bitmap Yehyeong Lee
` (4 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Yehyeong Lee @ 2026-10-14 15:19 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, linux-kernel, stable, yhlee
mvumi_hs_process_page() checksums and parses a handshake page using
frame_length from the page header, a u16 the firmware fills in, without
checking it against the HSP_MAX_SIZE buffer the page lives in.
mvumi_calculate_checksum() then walks frame_length bytes of frame_content
with an unsigned char loop counter, so a frame_length of 256 or more never
terminates and the probe path hangs; values between the buffer size and
255 read past the page.
Reject a frame_length that does not fit the handshake buffer before using
it.
Read frame_length once into a local and use it for both the bound check
and the checksum, so the device cannot change the value in the DMA page
between the two, and use READ_ONCE() so the single fetch is guaranteed
rather than left to the compiler.
Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
drivers/scsi/mvumi.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 38a92f8c25f5b..2378b676a7a78 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -1039,9 +1039,15 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
{
struct mvumi_hs_page1 *hs_page1;
unsigned char page_checksum;
+ u16 frame_length = READ_ONCE(hs_header->frame_length);
- page_checksum = mvumi_calculate_checksum(hs_header,
- hs_header->frame_length);
+ if (frame_length > HSP_MAX_SIZE - sizeof(struct mvumi_hs_header)) {
+ dev_err(&mhba->pdev->dev, "invalid handshake frame length %d\n",
+ frame_length);
+ return -1;
+ }
+
+ page_checksum = mvumi_calculate_checksum(hs_header, frame_length);
if (page_checksum != hs_header->checksum) {
dev_err(&mhba->pdev->dev, "checksum error\n");
return -1;
--
2.43.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 2/6] scsi: mvumi: reject an out-of-range handshake frame length
2026-10-14 15:19 ` [PATCH v5 2/6] scsi: mvumi: reject an out-of-range handshake frame length Yehyeong Lee
@ 2026-10-14 15:33 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-10-14 15:33 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mvumi: TOCTOU on DMA payload bypasses checksum and bounds
- [Medium] scsi: mvumi: missing endianness conversion for frame_length
--
commit 31d1a951cd494fae608e39897206b687a75bc558
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
scsi: mvumi: reject an out-of-range handshake frame length
This patch fixes an out-of-bounds read and potential hang in the mvumi
driver by validating the frame_length from the handshake page header. It
ensures the length fits within the handshake buffer before calculating the
checksum, and uses READ_ONCE() to prevent the device from changing the
value mid-validation.
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 38a92f8c25f5b..2378b676a7a78 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -1039,9 +1039,15 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
> {
> struct mvumi_hs_page1 *hs_page1;
> unsigned char page_checksum;
> + u16 frame_length = READ_ONCE(hs_header->frame_length);
[Severity: Medium]
Is an endianness conversion needed here? Hardware devices typically provide
data in little-endian format.
If run on a big-endian architecture, a valid small little-endian length
might be interpreted as a heavily byte-swapped value. This could inevitably
fail the newly added bounds check, preventing driver initialization entirely
on big-endian hosts.
> - page_checksum = mvumi_calculate_checksum(hs_header,
> - hs_header->frame_length);
> + if (frame_length > HSP_MAX_SIZE - sizeof(struct mvumi_hs_header)) {
> + dev_err(&mhba->pdev->dev, "invalid handshake frame length %d\n",
> + frame_length);
> + return -1;
> + }
> +
> + page_checksum = mvumi_calculate_checksum(hs_header, frame_length);
> if (page_checksum != hs_header->checksum) {
> dev_err(&mhba->pdev->dev, "checksum error\n");
> return -1;
> }
[Severity: High]
Does validating the checksum over the shared DMA buffer guarantee the
payload's integrity when it is read later?
After validation, mvumi_hs_process_page() and mvumi_handshake() read fields
directly from the shared DMA buffer without using READ_ONCE() or making a
local copy:
mvumi_hs_process_page() {
...
switch (hs_header->page_code) {
case HS_PAGE_FIRM_CAP:
hs_page1 = (struct mvumi_hs_page1 *) hs_header;
mhba->max_io = hs_page1->max_io_support;
...
mhba->ib_max_size_setting = hs_page1->cl_in_max_entry_size;
...
}
Could a malicious device concurrently modify these fields immediately after
the checksum is verified, bypassing the integrity check entirely?
This might allow the device to inject values that lead to undefined behavior
or severely corrupt the driver state machine.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261014151914.3632701-1-yhlee@isslab.korea.ac.kr?part=2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 3/6] scsi: mvumi: bound the hotplug event bitmap
2026-10-14 15:19 [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies Yehyeong Lee
2026-10-14 15:19 ` [PATCH v5 1/6] scsi: mvumi: bound the per-event parameter and sense dumps Yehyeong Lee
2026-10-14 15:19 ` [PATCH v5 2/6] scsi: mvumi: reject an out-of-range handshake frame length Yehyeong Lee
@ 2026-10-14 15:19 ` Yehyeong Lee
2026-10-14 15:35 ` sashiko-bot
2026-10-14 15:19 ` [PATCH v5 4/6] scsi: mvumi: reject handshake sizes that overflow the list allocations Yehyeong Lee
` (3 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Yehyeong Lee @ 2026-10-14 15:19 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, linux-kernel, stable, yhlee
mvumi_proc_msg() takes size from the hotplug event the firmware DMAs into
the event buffer and uses it twice: as the bit count for two
find_next_zero_bit() scans, and as the byte offset of the second bitmap.
Nothing checks it against the buffer, which mvumi_get_event() allocates at
a fixed 512 bytes, and size is a u16, so a firmware reporting 65535 makes
both scans run roughly 8 KiB past the allocation and places the second
bitmap past it to begin with.
Reject a size whose bitmaps do not fit the buffer. The second bitmap
starts size bits in and find_next_zero_bit() reads a long at a time, so
that span is the furthest either scan reaches; compare it against the
buffer and give the buffer size a name so the bound and the allocation
cannot drift apart. Read size once, since it lives in memory the
firmware can still write.
Fixes: bd756ddea18e ("[SCSI] mvumi: Add support for Marvell SAS/SATA RAID-on-Chip(ROC) 88RC9580")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
drivers/scsi/mvumi.c | 16 ++++++++++++++--
drivers/scsi/mvumi.h | 3 +++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 2378b676a7a78..9e342db4b8383 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -1652,11 +1652,23 @@ static int mvumi_rescan_bus(void *data)
static void mvumi_proc_msg(struct mvumi_hba *mhba,
struct mvumi_hotplug_event *param)
{
- u16 size = param->size;
+ u16 size = READ_ONCE(param->size);
const unsigned long *ar_bitmap;
const unsigned long *re_bitmap;
int index;
+ /*
+ * The second bitmap starts size bits into the event buffer and
+ * find_next_zero_bit() reads it a long at a time, so that is the
+ * furthest the two scans reach.
+ */
+ if (sizeof(*param) + (size >> 3) + BITS_TO_LONGS(size) * sizeof(long) >
+ MVUMI_EVENT_BUF_SIZE) {
+ dev_err(&mhba->pdev->dev,
+ "invalid hotplug event bitmap size %u\n", size);
+ return;
+ }
+
if (mhba->fw_flag & MVUMI_FW_ATTACH) {
index = -1;
ar_bitmap = (const unsigned long *) param->bitmap;
@@ -1708,7 +1720,7 @@ static int mvumi_get_event(struct mvumi_hba *mhba, unsigned char msg)
struct mvumi_cmd *cmd;
struct mvumi_msg_frame *frame;
- cmd = mvumi_create_internal_cmd(mhba, 512);
+ cmd = mvumi_create_internal_cmd(mhba, MVUMI_EVENT_BUF_SIZE);
if (!cmd)
return -1;
cmd->scmd = NULL;
diff --git a/drivers/scsi/mvumi.h b/drivers/scsi/mvumi.h
index 1306a4abf19ad..5d45f0d99e639 100644
--- a/drivers/scsi/mvumi.h
+++ b/drivers/scsi/mvumi.h
@@ -124,6 +124,9 @@ enum {
#define APICDB1_HOST_GETEVENT 1
#define MAX_EVENTS_RETURNED 6
+/* size of the buffer the driver hands the firmware for event replies */
+#define MVUMI_EVENT_BUF_SIZE 512
+
#define DEVICE_OFFLINE 0
#define DEVICE_ONLINE 1
--
2.43.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 3/6] scsi: mvumi: bound the hotplug event bitmap
2026-10-14 15:19 ` [PATCH v5 3/6] scsi: mvumi: bound the hotplug event bitmap Yehyeong Lee
@ 2026-10-14 15:35 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-10-14 15:35 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: mvumi: unaligned access, endianness, and truncation in bitmap scan
- [High] scsi: mvumi: DMA address truncation in mvumi_delete_internal_cmd
- [Medium] scsi: mvumi: incomplete bounds check causes phantom hotplug events
--
commit a0f2016a838932df1a44f506a29ddeb029ce97e6
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
scsi: mvumi: bound the hotplug event bitmap
This commit introduces bounds checking for the hotplug event bitmap in
mvumi_proc_msg to prevent scans from exceeding the allocated buffer size.
It also adds a defined macro for the event buffer size to keep the
allocation and bounds check synchronized.
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -1652,11 +1652,23 @@ static int mvumi_rescan_bus(void *data)
> static void mvumi_proc_msg(struct mvumi_hba *mhba,
> struct mvumi_hotplug_event *param)
> {
> - u16 size = param->size;
> + u16 size = READ_ONCE(param->size);
[Severity: High]
Since param->size is supplied by the firmware, does this need an endianness
conversion, such as le16_to_cpu(), to prevent incorrect size bounds on
big-endian systems?
> const unsigned long *ar_bitmap;
> const unsigned long *re_bitmap;
> int index;
>
> + /*
> + * The second bitmap starts size bits into the event buffer and
> + * find_next_zero_bit() reads it a long at a time, so that is the
> + * furthest the two scans reach.
> + */
> + if (sizeof(*param) + (size >> 3) + BITS_TO_LONGS(size) * sizeof(long) >
> + MVUMI_EVENT_BUF_SIZE) {
[Severity: Medium]
The bounds check allows sizes up to MVUMI_EVENT_BUF_SIZE (512 bytes).
However, looking at the DMA transfer configuration in mvumi_get_event(),
the data_transfer_length is strictly limited to
sizeof(struct mvumi_event_req) which is 388 bytes.
Could this mismatch cause find_next_zero_bit() to scan unwritten,
zero-filled memory between 388 and 512 bytes, treating the zeroes as
device events and incorrectly triggering phantom SCSI hotplug additions?
> + dev_err(&mhba->pdev->dev,
> + "invalid hotplug event bitmap size %u\n", size);
> + return;
> + }
> +
> if (mhba->fw_flag & MVUMI_FW_ATTACH) {
> index = -1;
> ar_bitmap = (const unsigned long *) param->bitmap;
> re_bitmap = (const unsigned long *) ¶m->bitmap[size >> 3];
[Severity: High]
Does casting param->bitmap, which is at a 4-byte offset in the DMA buffer,
to an unsigned long pointer risk causing an alignment fault on 64-bit
architectures with strict alignment rules?
Additionally, if the firmware provides a size that is not a multiple of 8,
does the bitwise shift size >> 3 truncate the byte offset, causing the
re_bitmap offline scan to incorrectly overlap with the ar_bitmap online
scan?
[ ... ]
> @@ -1708,7 +1720,7 @@ static int mvumi_get_event(struct mvumi_hba *mhba, unsigned char msg)
> struct mvumi_cmd *cmd;
> struct mvumi_msg_frame *frame;
>
> - cmd = mvumi_create_internal_cmd(mhba, 512);
> + cmd = mvumi_create_internal_cmd(mhba, MVUMI_EVENT_BUF_SIZE);
> if (!cmd)
> return -1;
[ ... ]
> if (cmd->cmd_status != SAM_STAT_GOOD)
> dev_err(&mhba->pdev->dev, "get event failed, status=0x%x.\n",
> cmd->cmd_status);
> else
> mvumi_notification(mhba, cmd->frame->cdb[1], cmd->data_buf);
>
> mvumi_delete_internal_cmd(mhba, cmd);
[Severity: High]
Looking at mvumi_delete_internal_cmd() called here during cleanup:
drivers/scsi/mvumi.c:mvumi_delete_internal_cmd() {
...
phy_addr = (dma_addr_t) m_sg->baseaddr_l |
(dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
...
}
Since baseaddr_h is a 32-bit u32, does this double shift evaluate in a
32-bit unsigned int context, causing the upper 32 bits to be zeroed on
64-bit systems before the cast to dma_addr_t?
Could this truncated physical address lead to a DMA unmap memory
corruption or resource leak when reconstructing the original address?
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261014151914.3632701-1-yhlee@isslab.korea.ac.kr?part=3
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 4/6] scsi: mvumi: reject handshake sizes that overflow the list allocations
2026-10-14 15:19 [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies Yehyeong Lee
` (2 preceding siblings ...)
2026-10-14 15:19 ` [PATCH v5 3/6] scsi: mvumi: bound the hotplug event bitmap Yehyeong Lee
@ 2026-10-14 15:19 ` Yehyeong Lee
2026-10-14 15:31 ` sashiko-bot
2026-10-14 15:19 ` [PATCH v5 5/6] scsi: mvumi: validate the response frame tag before using it Yehyeong Lee
` (2 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Yehyeong Lee @ 2026-10-14 15:19 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, linux-kernel, stable, yhlee
mvumi_hs_process_page() takes max_io, the two command entry sizes and the
list depth from the firmware handshake page. The entry sizes are u8 fields
used as shift counts, and 1 is an int, so a count of 29 or more makes the
<< 2 applied to the shift overflow an int; a count of 32 or more is
undefined outright. max_io is a u16, and the sizes derived from the shifts
are multiplied by it in mvumi_init_data() to size three unsigned int
allocations, so a large entry size wraps the product and the loops that
follow then walk and write past the short allocation. The list depth is
used as a shift count too, but only when the firmware sets
HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF; a count of 32 or more is undefined
there as well. Nothing bounds the entry sizes from below either, and an
entry has to hold what the driver puts in it: a command frame and at least
one scatter-gather element going in, a response frame and sense data coming
back. An entry size of 32 bytes or less makes mvumi_internal_cmd_sgl()
write its element past an inbound entry, since it writes one whatever
host->sg_tablesize says, and makes mvumi_complete_cmd() read sense data
past an outbound one; it also makes the max_sg subtraction in
mvumi_io_attach() wrap, which hands mvumi_build_frame() and
mvumi_send_command() a table size no entry can hold. The depth also sets
how many entries the inbound and outbound rings hold, and the per-command
slot index wraps on that count while the rings are allocated for max_io
entries, so a depth yielding more entries than max_io -- or none -- walks
the index past them. max_devices_support sizes the target_map bitmap in
the same allocation. A host reset runs the handshake again, but
mvumi_init_data() returns early once MVUMI_FW_ALLOC is set, so the buffers
keep the sizes they were allocated with while the newly reported values
drive the indexing: mvumi_flush_cache() then reads target_map past its end
and mvumi_sdev_configure() sets a bit past it. The handshake is retried as
well, because mvumi_handshake_event() discards the error, so a page that
was already rejected can be presented again.
Reject an entry size above 29, which keeps 1U << n and the shift that
follows it inside an unsigned int, and reject one too small to hold those
frames. Then compute the worst-case allocation size in u64 at the top of
mvumi_init_data() and reject it if it does not fit an unsigned int. That
total bounds all three allocations: the inbound list total, the outbound
data pool and the tag arrays, whose sizes are the same products plus
smaller terms. Reject a list depth of 32 or more on the shifted path so
the shift stays defined, and reject a resulting entry count that is zero or
above max_io so the slot index cannot walk past the rings. The shifts also
use 1U now so that a legal count cannot overflow an int. Take the page's
values into locals and run every check on those. Once MVUMI_FW_ALLOC is
set the buffers already exist, so only check that no reported size,
max_devices_support included, is larger than the one its buffer was
allocated for, and leave the size fields alone: they already hold the sizes
those buffers have. Only the first handshake, whose values size the
allocations, assigns them.
No realistic controller is rejected: a controller reporting 4096 commands
with 64 KiB entries and a depth of 12 needs 513 MiB and passes unchanged.
Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
drivers/scsi/mvumi.c | 87 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 75 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 9e342db4b8383..e63b3ba7eaabd 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -912,10 +912,23 @@ static int mvumi_init_data(struct mvumi_hba *mhba)
unsigned int tmp_size, offset, i;
void *virmem, *v;
dma_addr_t p;
+ u64 total;
if (mhba->fw_flag & MVUMI_FW_ALLOC)
return 0;
+ /* These are firmware-supplied and size the allocations below. */
+ total = (u64)mhba->ib_max_size * mhba->max_io +
+ (u64)mhba->ob_max_size * mhba->max_io +
+ sizeof(struct mvumi_dyn_list_entry) * (u64)mhba->max_io +
+ 160;
+ if (total > UINT_MAX) {
+ dev_err(&mhba->pdev->dev,
+ "handshake sizes too large: io %u, in %u, out %u\n",
+ mhba->max_io, mhba->ib_max_size, mhba->ob_max_size);
+ return -1;
+ }
+
tmp_size = mhba->ib_max_size * mhba->max_io;
if (mhba->hba_capability & HS_CAPABILITY_SUPPORT_DYN_SRC)
tmp_size += sizeof(struct mvumi_dyn_list_entry) * mhba->max_io;
@@ -1054,30 +1067,80 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
}
switch (hs_header->page_code) {
- case HS_PAGE_FIRM_CAP:
+ case HS_PAGE_FIRM_CAP: {
+ unsigned int new_max_io, new_ib_max_size, new_ob_max_size;
+ unsigned int new_list_num_io;
+ unsigned short new_max_target_id, new_capability;
+
hs_page1 = (struct mvumi_hs_page1 *) hs_header;
- mhba->max_io = hs_page1->max_io_support;
- mhba->list_num_io = hs_page1->cl_inout_list_depth;
- mhba->max_transfer_size = hs_page1->max_transfer_size;
- mhba->max_target_id = hs_page1->max_devices_support;
- mhba->hba_capability = hs_page1->capability;
- mhba->ib_max_size_setting = hs_page1->cl_in_max_entry_size;
- mhba->ib_max_size = (1 << hs_page1->cl_in_max_entry_size) << 2;
+ /* Firmware-supplied shift counts: (1U << n) << 2 must fit. */
+ if (hs_page1->cl_in_max_entry_size > 29 ||
+ hs_page1->cl_out_max_entry_size > 29) {
+ dev_err(&mhba->pdev->dev,
+ "invalid handshake entry size\n");
+ return -1;
+ }
- mhba->ob_max_size_setting = hs_page1->cl_out_max_entry_size;
- mhba->ob_max_size = (1 << hs_page1->cl_out_max_entry_size) << 2;
+ new_max_io = hs_page1->max_io_support;
+ new_ib_max_size = (1U << hs_page1->cl_in_max_entry_size) << 2;
+ new_ob_max_size = (1U << hs_page1->cl_out_max_entry_size) << 2;
+ new_max_target_id = hs_page1->max_devices_support;
+ new_capability = hs_page1->capability;
+
+ new_list_num_io = hs_page1->cl_inout_list_depth;
+ if (new_capability & HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF) {
+ if (hs_page1->cl_inout_list_depth >= 32) {
+ dev_err(&mhba->pdev->dev,
+ "invalid handshake list depth\n");
+ return -1;
+ }
+ new_list_num_io = 1U << hs_page1->cl_inout_list_depth;
+ }
+ /* The rings hold max_io entries but the index wraps on list_num_io. */
+ if (new_list_num_io == 0 || new_list_num_io > new_max_io) {
+ dev_err(&mhba->pdev->dev,
+ "invalid handshake list depth: %u entries for %u commands\n",
+ new_list_num_io, new_max_io);
+ return -1;
+ }
dev_dbg(&mhba->pdev->dev, "FW version:%d\n",
hs_page1->fw_ver.ver_build);
+ /* A host reset does not reallocate, so the sizes have to stay
+ * the ones the buffers were allocated with. Only check that
+ * the firmware is not asking for more than they hold.
+ */
+ if (mhba->fw_flag & MVUMI_FW_ALLOC) {
+ if (new_max_io > mhba->max_io ||
+ new_ib_max_size > mhba->ib_max_size ||
+ new_ob_max_size > mhba->ob_max_size ||
+ new_list_num_io > mhba->list_num_io ||
+ new_max_target_id > mhba->max_target_id) {
+ dev_err(&mhba->pdev->dev,
+ "handshake sizes grew across reset\n");
+ return -1;
+ }
+ break;
+ }
+
+ mhba->max_io = new_max_io;
+ mhba->list_num_io = new_list_num_io;
+ mhba->max_transfer_size = hs_page1->max_transfer_size;
+ mhba->max_target_id = new_max_target_id;
+ mhba->hba_capability = new_capability;
+ mhba->ib_max_size_setting = hs_page1->cl_in_max_entry_size;
+ mhba->ib_max_size = new_ib_max_size;
+ mhba->ob_max_size_setting = hs_page1->cl_out_max_entry_size;
+ mhba->ob_max_size = new_ob_max_size;
+
if (mhba->hba_capability & HS_CAPABILITY_SUPPORT_COMPACT_SG)
mhba->eot_flag = 22;
else
mhba->eot_flag = 27;
- if (mhba->hba_capability & HS_CAPABILITY_NEW_PAGE_IO_DEPTH_DEF)
- mhba->list_num_io = 1 << hs_page1->cl_inout_list_depth;
break;
+ }
default:
dev_err(&mhba->pdev->dev, "handshake: page code error\n");
return -1;
--
2.43.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 4/6] scsi: mvumi: reject handshake sizes that overflow the list allocations
2026-10-14 15:19 ` [PATCH v5 4/6] scsi: mvumi: reject handshake sizes that overflow the list allocations Yehyeong Lee
@ 2026-10-14 15:31 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-10-14 15:31 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mvumi: ring queue desync when firmware limits shrink across reset
--
commit af2767dc7ac6cb07639493635b81a3c6bd8a53fb
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
scsi: mvumi: reject handshake sizes that overflow the list allocations
This commit adds bounds checking for handshake page sizes to prevent
integer overflows and out-of-bounds writes during allocations. It also
validates that firmware limits do not grow across a host reset.
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 9e342db4b8383..e63b3ba7eaabd 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
[ ... ]
> @@ -1054,30 +1067,80 @@ static int mvumi_hs_process_page(struct mvumi_hba *mhba,
> }
>
> switch (hs_header->page_code) {
> - case HS_PAGE_FIRM_CAP:
> + case HS_PAGE_FIRM_CAP: {
[ ... ]
> + /* A host reset does not reallocate, so the sizes have to stay
> + * the ones the buffers were allocated with. Only check that
> + * the firmware is not asking for more than they hold.
> + */
> + if (mhba->fw_flag & MVUMI_FW_ALLOC) {
> + if (new_max_io > mhba->max_io ||
> + new_ib_max_size > mhba->ib_max_size ||
> + new_ob_max_size > mhba->ob_max_size ||
> + new_list_num_io > mhba->list_num_io ||
> + new_max_target_id > mhba->max_target_id) {
> + dev_err(&mhba->pdev->dev,
> + "handshake sizes grew across reset\n");
> + return -1;
> + }
> + break;
[Severity: High]
Does this break statement skip updating the driver's internal limits when the
firmware capacities shrink across a reset?
If a soft reset occurs and the firmware reports smaller capabilities for
list_num_io or max_io, this path is taken because the new values are not
larger than the old ones. However, by breaking out early, the driver misses
updating its internal state to match the smaller firmware limits.
Can this lead to desynchronization of the driver-firmware circular command
rings and overflow of firmware queues?
> + }
> +
> + mhba->max_io = new_max_io;
> + mhba->list_num_io = new_list_num_io;
> + mhba->max_transfer_size = hs_page1->max_transfer_size;
> + mhba->max_target_id = new_max_target_id;
> + mhba->hba_capability = new_capability;
> + mhba->ib_max_size_setting = hs_page1->cl_in_max_entry_size;
> + mhba->ib_max_size = new_ib_max_size;
> + mhba->ob_max_size_setting = hs_page1->cl_out_max_entry_size;
> + mhba->ob_max_size = new_ob_max_size;
> +
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261014151914.3632701-1-yhlee@isslab.korea.ac.kr?part=4
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 5/6] scsi: mvumi: validate the response frame tag before using it
2026-10-14 15:19 [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies Yehyeong Lee
` (3 preceding siblings ...)
2026-10-14 15:19 ` [PATCH v5 4/6] scsi: mvumi: reject handshake sizes that overflow the list allocations Yehyeong Lee
@ 2026-10-14 15:19 ` Yehyeong Lee
2026-10-14 15:33 ` sashiko-bot
2026-10-14 15:19 ` [PATCH v5 6/6] scsi: mvumi: widen the flush cache loop counter Yehyeong Lee
2026-10-14 16:28 ` [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies Yehyeong Lee
6 siblings, 1 reply; 14+ messages in thread
From: Yehyeong Lee @ 2026-10-14 15:19 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, linux-kernel, stable, yhlee
mvumi_handle_clob() indexes mhba->tag_cmd[] with the tag of a response
frame the firmware DMAs into the outbound list. The array holds
tag_pool.size entries, and the only check on that tag runs earlier, in
mvumi_receive_ob_list_entry() and mvumi_check_ob_frame(), against the frame
still sitting in the outbound list. mvumi_receive_ob_list_entry() then
copies the frame into an ob_data buffer and mvumi_handle_clob() reads the
tag out of that copy, so the value that is checked and the value that is
used are two separate fetches of firmware-writable memory. A firmware that
changes the tag after the check reaches tag_cmd[] with any u16, reads a
pointer past the array and dereferences it, writes NULL past the array and
pushes the tag onto the free stack for a later command to pick up. The
check is also off by one: tag_cmd[] and tag_pool.stack[] both hold
tag_pool.size entries, so a tag equal to tag_pool.size passes a > test and
lands one entry past both.
Check the tag that mvumi_handle_clob() actually uses, against the size of
the array it indexes, and check the command it finds for NULL before
dereferencing it: with two fetches the earlier NULL check no longer says
anything about this one. Make the two existing bounds checks >= so a tag
equal to the entry count is rejected there too, and read the tag into a
local in mvumi_receive_ob_list_entry() as well, so the check there and the
accesses beside it see one value instead of three fetches.
Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
drivers/scsi/mvumi.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index e63b3ba7eaabd..07cfac9276e7b 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -468,7 +468,7 @@ static char mvumi_check_ob_frame(struct mvumi_hba *mhba,
p_outb_frame = mhba->ob_list + cur_obf * mhba->ob_max_size;
request_id = p_outb_frame->request_id;
tag = p_outb_frame->tag;
- if (tag > mhba->tag_pool.size) {
+ if (tag >= mhba->tag_pool.size) {
dev_err(&mhba->pdev->dev, "ob frame data error\n");
return -1;
}
@@ -530,6 +530,7 @@ static void mvumi_receive_ob_list_entry(struct mvumi_hba *mhba)
struct mvumi_ob_data *ob_data;
struct mvumi_rsp_frame *p_outb_frame;
struct mvumi_hw_regs *regs = mhba->regs;
+ unsigned short tag;
if (mhba->instancet->check_ob_list(mhba, &cur_obf, &assign_obf_end))
return;
@@ -546,10 +547,11 @@ static void mvumi_receive_ob_list_entry(struct mvumi_hba *mhba)
/* Copy pointer may point to entry in outbound list
* before entry has valid data
*/
- if (unlikely(p_outb_frame->tag > mhba->tag_pool.size ||
- mhba->tag_cmd[p_outb_frame->tag] == NULL ||
- p_outb_frame->request_id !=
- mhba->tag_cmd[p_outb_frame->tag]->request_id))
+ tag = p_outb_frame->tag;
+ if (unlikely(tag >= mhba->tag_pool.size ||
+ !mhba->tag_cmd[tag] ||
+ mhba->tag_cmd[tag]->request_id !=
+ p_outb_frame->request_id))
if (mvumi_check_ob_frame(mhba, cur_obf, p_outb_frame))
continue;
@@ -1848,6 +1850,7 @@ static void mvumi_handle_clob(struct mvumi_hba *mhba)
struct mvumi_rsp_frame *ob_frame;
struct mvumi_cmd *cmd;
struct mvumi_ob_data *pool;
+ unsigned short tag;
while (!list_empty(&mhba->free_ob_list)) {
pool = list_first_entry(&mhba->free_ob_list,
@@ -1856,11 +1859,26 @@ static void mvumi_handle_clob(struct mvumi_hba *mhba)
list_add_tail(&pool->list, &mhba->ob_data_list);
ob_frame = (struct mvumi_rsp_frame *) &pool->data[0];
- cmd = mhba->tag_cmd[ob_frame->tag];
+ /*
+ * The tag was checked against the frame in the outbound list.
+ * This is the copy, so check the value that is used.
+ */
+ tag = ob_frame->tag;
+ if (tag >= mhba->tag_pool.size) {
+ dev_err(&mhba->pdev->dev,
+ "ob frame tag[0x%x] out of range\n", tag);
+ continue;
+ }
+ cmd = mhba->tag_cmd[tag];
+ if (!cmd) {
+ dev_err(&mhba->pdev->dev,
+ "ob frame tag[0x%x] with no command\n", tag);
+ continue;
+ }
atomic_dec(&mhba->fw_outstanding);
- mhba->tag_cmd[ob_frame->tag] = NULL;
- tag_release_one(mhba, &mhba->tag_pool, ob_frame->tag);
+ mhba->tag_cmd[tag] = NULL;
+ tag_release_one(mhba, &mhba->tag_pool, tag);
if (cmd->scmd)
mvumi_complete_cmd(mhba, cmd, ob_frame);
else
--
2.43.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 5/6] scsi: mvumi: validate the response frame tag before using it
2026-10-14 15:19 ` [PATCH v5 5/6] scsi: mvumi: validate the response frame tag before using it Yehyeong Lee
@ 2026-10-14 15:33 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-10-14 15:33 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: mvumi: missing READ_ONCE() on DMA tag allows TOCTOU OOB access
- [High] scsi: mvumi: missing endianness conversions in DMA shared structs
- [High] scsi: mvumi: missing dma_rmb() before reading DMA ring entry
--
commit 8741f5e18a328ba9b868f0711acd0a3e2b7e9ee4
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
Subject: scsi: mvumi: validate the response frame tag before using it
This patch adds bounds and NULL command checks on the response frame tag
used in mvumi_handle_clob, mvumi_receive_ob_list_entry, and
mvumi_check_ob_frame. It ensures the bounds checks include the tag pool
size, and explicitly checks the array element dereference.
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e63b3ba7eaabd..07cfac9276e7b 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -468,7 +468,7 @@ static char mvumi_check_ob_frame(struct mvumi_hba *mhba,
> p_outb_frame = mhba->ob_list + cur_obf * mhba->ob_max_size;
> request_id = p_outb_frame->request_id;
> tag = p_outb_frame->tag;
[Severity: High]
Does this read from DMA shared memory require a READ_ONCE to prevent a
TOCTOU double-fetch?
Without it, the compiler could potentially optimize this into multiple
fetches directly from memory. If a compromised or malicious device changes
the tag after the bounds check below, could it lead to an out-of-bounds
access on tag_cmd?
[Severity: High]
Does this function also need a dma_rmb before validating and returning?
The udelay(1) called earlier in this function works around the DMA visibility
race but does not provide the required memory barrier to prevent reordering of
subsequent payload reads.
> - if (tag > mhba->tag_pool.size) {
> + if (tag >= mhba->tag_pool.size) {
> dev_err(&mhba->pdev->dev, "ob frame data error\n");
> return -1;
> }
[ ... ]
> @@ -546,10 +547,11 @@ static void mvumi_receive_ob_list_entry(struct mvumi_hba *mhba)
> /* Copy pointer may point to entry in outbound list
> * before entry has valid data
> */
> - if (unlikely(p_outb_frame->tag > mhba->tag_pool.size ||
> - mhba->tag_cmd[p_outb_frame->tag] == NULL ||
> - p_outb_frame->request_id !=
> - mhba->tag_cmd[p_outb_frame->tag]->request_id))
> + tag = p_outb_frame->tag;
[Severity: High]
Like in mvumi_check_ob_frame, does this read from DMA shared memory also
require a READ_ONCE to prevent a TOCTOU double-fetch?
Without READ_ONCE, the compiler might optimize this into multiple fetches
directly from memory, which could lead to an out-of-bounds access on tag_cmd
if the tag is changed by the device after the bounds check.
[Severity: High]
Additionally, is a dma_rmb needed after observing a valid tag?
On weakly ordered architectures, the subsequent memcpy of the frame payload
(which happens further down in this function) could be speculatively
reordered before the tag is validated, potentially capturing stale data.
[Severity: High]
Also, tag is a 16-bit field in mvumi_rsp_frame. Since it is being read using
native host endianness, will this result in byteswapped values on big-endian
architectures? This could cause huge swapped tag values to falsely fail the
bounds check.
> + if (unlikely(tag >= mhba->tag_pool.size ||
> + !mhba->tag_cmd[tag] ||
> + mhba->tag_cmd[tag]->request_id !=
> + p_outb_frame->request_id))
> if (mvumi_check_ob_frame(mhba, cur_obf, p_outb_frame))
> continue;
[ ... ]
> @@ -1856,11 +1859,26 @@ static void mvumi_handle_clob(struct mvumi_hba *mhba)
> list_add_tail(&pool->list, &mhba->ob_data_list);
>
> ob_frame = (struct mvumi_rsp_frame *) &pool->data[0];
> - cmd = mhba->tag_cmd[ob_frame->tag];
> + /*
> + * The tag was checked against the frame in the outbound list.
> + * This is the copy, so check the value that is used.
> + */
> + tag = ob_frame->tag;
[Severity: High]
Will reading this 16-bit tag from the local copy of the frame using native
endianness cause false bounds check failures on big-endian hosts? It appears
an endianness conversion is missing here as well.
> + if (tag >= mhba->tag_pool.size) {
> + dev_err(&mhba->pdev->dev,
> + "ob frame tag[0x%x] out of range\n", tag);
> + continue;
> + }
> + cmd = mhba->tag_cmd[tag];
> + if (!cmd) {
> + dev_err(&mhba->pdev->dev,
> + "ob frame tag[0x%x] with no command\n", tag);
> + continue;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261014151914.3632701-1-yhlee@isslab.korea.ac.kr?part=5
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 6/6] scsi: mvumi: widen the flush cache loop counter
2026-10-14 15:19 [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies Yehyeong Lee
` (4 preceding siblings ...)
2026-10-14 15:19 ` [PATCH v5 5/6] scsi: mvumi: validate the response frame tag before using it Yehyeong Lee
@ 2026-10-14 15:19 ` Yehyeong Lee
2026-10-14 15:38 ` sashiko-bot
2026-10-14 16:28 ` [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies Yehyeong Lee
6 siblings, 1 reply; 14+ messages in thread
From: Yehyeong Lee @ 2026-10-14 15:19 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, linux-kernel, stable, yhlee
mvumi_flush_cache() walks the targets with an unsigned char counter while
the bound it compares against, mhba->max_target_id, comes from the u16
max_devices_support field of the firmware handshake page. A controller
reporting 256 or more devices makes the counter wrap back to zero before
it ever reaches the bound, so the loop never ends. mvumi_flush_cache()
runs from the shutdown, suspend and remove paths, so the firmware can
wedge any of them.
Give the counter the type of the bound. It is assigned to
frame->device_id, a u16, so nothing truncates it again, and target_map
holds round_up(max_target_id, 8) / 8 bytes, which covers every index the
wider counter produces. That last part needs the earlier patch in this
series: without the reset check it adds, max_target_id can grow past the
bitmap it was allocated for, and the wider counter would then walk further
past it than the narrow one could.
Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
drivers/scsi/mvumi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 07cfac9276e7b..090ae79ec8c3a 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -769,8 +769,9 @@ static unsigned char mvumi_flush_cache(struct mvumi_hba *mhba)
{
struct mvumi_cmd *cmd;
struct mvumi_msg_frame *frame;
- unsigned char device_id, retry = 0;
+ unsigned char retry = 0;
unsigned char bitcount = sizeof(unsigned char) * 8;
+ unsigned short device_id;
for (device_id = 0; device_id < mhba->max_target_id; device_id++) {
if (!(mhba->target_map[device_id / bitcount] &
--
2.43.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5 6/6] scsi: mvumi: widen the flush cache loop counter
2026-10-14 15:19 ` [PATCH v5 6/6] scsi: mvumi: widen the flush cache loop counter Yehyeong Lee
@ 2026-10-14 15:38 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-10-14 15:38 UTC (permalink / raw)
To: Yehyeong Lee; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] scsi: mvumi: target ID and LUN overlap causes command misrouting
--
commit 74a1c4d0f801df93a81b66efae4a7d7d5c3af7b9
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
scsi: mvumi: widen the flush cache loop counter
mvumi_flush_cache() walks the targets with an unsigned char counter while
the bound it compares against, mhba->max_target_id, comes from the u16
max_devices_support field of the firmware handshake page. A controller
reporting 256 or more devices makes the counter wrap back to zero before
it ever reaches the bound, so the loop never ends. This patch gives the
counter the type of the bound to prevent this wedge.
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 07cfac9276e7b..090ae79ec8c3a 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -769,8 +769,9 @@ static unsigned char mvumi_flush_cache(struct mvumi_hba *mhba)
> {
> struct mvumi_cmd *cmd;
> struct mvumi_msg_frame *frame;
> - unsigned char device_id, retry = 0;
> + unsigned char retry = 0;
> unsigned char bitcount = sizeof(unsigned char) * 8;
> + unsigned short device_id;
>
> for (device_id = 0; device_id < mhba->max_target_id; device_id++) {
> if (!(mhba->target_map[device_id / bitcount] &
[Severity: Critical]
By widening the device_id counter to unsigned short, this code now allows
target IDs of 256 and higher to be processed. Does this safely interact with
the I/O submission path?
Looking at mvumi_build_frame(), the target ID and LUN are packed together
into the device_id field using a bitwise OR:
drivers/scsi/mvumi.c:mvumi_build_frame() {
...
pframe->device_id = ((unsigned short) scmd->device->id) |
(((unsigned short) scmd->device->lun) << 8);
...
}
If a target ID is 256 or greater (e.g., 0x0100), its bits will bleed into the
LUN space. For example, an I/O to Target 256 LUN 0 would collide exactly with
an I/O to Target 0 LUN 1.
Could this lead to silent command misrouting and data corruption for
high-numbered targets newly unblocked by this patch?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261014151914.3632701-1-yhlee@isslab.korea.ac.kr?part=6
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies
2026-10-14 15:19 [PATCH v5 0/6] scsi: mvumi: bound the values the firmware supplies Yehyeong Lee
` (5 preceding siblings ...)
2026-10-14 15:19 ` [PATCH v5 6/6] scsi: mvumi: widen the flush cache loop counter Yehyeong Lee
@ 2026-10-14 16:28 ` Yehyeong Lee
6 siblings, 0 replies; 14+ messages in thread
From: Yehyeong Lee @ 2026-10-14 16:28 UTC (permalink / raw)
To: sashiko-bot, Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, linux-kernel, yhlee
Superseded by [PATCH v6 0/5],
<20261014162413.3649841-1-yhlee@isslab.korea.ac.kr>.
The real defects the reviews on this series found are fixed there: the
target/LUN alias from 6/6 is now a device-count bound in 4/5, the shift
counts 4/5 checks are read with READ_ONCE() so the checked value is the
shifted one, the hotplug bitmap is bounded by the transfer length rather
than the buffer, and the response frame tag is validated where it is used.
The rest I did not change, and the v6 cover letter says why: the driver has
no device-to-host endianness conversion anywhere, so converting one field
would be inconsistent; the bitmap alignment, the size >> 3 overlap and the
truncating shift in mvumi_delete_internal_cmd() are pre-existing and want
their own patches; keeping the sizes across a shrinking reset is correct
because the driver is what publishes them; and there is no ordering pair
for a dma_rmb() to protect.
^ permalink raw reply [flat|nested] 14+ messages in thread