Re: [PATCH v4 01/10] mpi3mr: Skip device shutdown during unload per controller configuration

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:

Pre-existing issues:
- [High] The driver rounds down the trace buffer size internally when alloc=
ating segmented buffer entries, but passes the original unrounded size to t=
he firmware, which can cause the firmware to read past the end of the segme=
nt list and DMA write to arbitrary physical memory.
- [Medium] A trace buffer size smaller than 4KB results in a 0-byte DMA all=
ocation, triggering a WARN_ON_ONCE within the kernel DMA API.
--

--- Patch [1]: [PATCH v4 01/10] mpi3mr: Skip device shutdown during unload =
per controller configuration ---
commit 9ac255a070f5df8da5fb46e54b2787eacb0b7c7c
Author: Ranjan Kumar <[email protected]>

mpi3mr: Skip device shutdown during unload per controller configuration

This commit caches Driver Page 1 settings to suppress device shutdown durin=
g=20
driver unload. It adds endianness conversions for the diagnostic buffer siz=
es
read from Driver Page 1 and hardens the allocation retry loops against inva=
lid
decrement sizes provided by the firmware.

Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@=
broadcom.com?part=3D1
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar=
@broadcom.com?part=3D1
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar=
@broadcom.com?part=3D1

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3m=
r_app.c
> index 1353a8ff9c85d..fca5357a515d1 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
[ ... ]
> @@ -179,6 +181,12 @@ void mpi3mr_alloc_diag_bufs(struct mpi3mr_ioc *mrioc)
>  		    mpi3mr_alloc_trace_buffer(mrioc, trace_size)) {
> =20
>  			retry =3D true;
> +
> +			if (!trace_dec_size || trace_dec_size > trace_size) {
> +				retry =3D false;
> +				goto retry_fw;
> +			}
> +
>  			trace_size -=3D trace_dec_size;

[Severity: High]
This is a pre-existing issue, but does this lead to firmware reading
out-of-bounds and potentially corrupting physical memory?

The driver rounds down the trace buffer size internally when allocating
segmented buffer entries in mpi3mr_alloc_trace_buffer():

    seg_count =3D (trace_size) / MPI3MR_PAGE_SIZE_4K;
    trace_size =3D seg_count * MPI3MR_PAGE_SIZE_4K;

However, it does not update the caller's trace_size. Thus, the original
unrounded trace_size is stored in mpi3mr_alloc_diag_bufs():

    diag_buffer->size =3D trace_size;

When posting the buffer to firmware in mpi3mr_issue_diag_buf_post():

    diag_buf_post_req.length =3D le32_to_cpu(diag_buffer->size);

The firmware calculates the required number of segments using the unrounded
size (e.g., ceil(5120/4096) =3D 2), but only 1 segment was allocated (size 8
bytes for the segment list). The firmware will read past the end of the
segment list (16 bytes), leading to an out-of-bounds DMA read. It will then
use the arbitrary out-of-bounds value as a physical address to DMA-write
trace data, leading to memory corruption.

[Severity: Medium]
This is a pre-existing issue, but can this retry loop cause a 0-byte DMA
allocation warning?

If the firmware-provided host_diag_trace_max_size (or the size after retry
decrements) is less than 4KB, trace_size will be less than 4096. When passed
to mpi3mr_alloc_trace_buffer(), it calculates seg_count =3D trace_size / 40=
96
which evaluates to 0.

This results in a 0-byte allocation in mpi3mr_alloc_trace_buffer():

    diag_buffer_list =3D dma_alloc_coherent(&mrioc->pdev->dev,
                                          sizeof(u64) * seg_count,
                                          &diag_buffer_list_dma, GFP_KERNEL=
);

Calling the DMA API with a 0 size triggers a WARN_ON_ONCE(!size) splat in
dma_alloc_attrs(), which can crash systems with panic_on_warn enabled.

>  			dprint_init(mrioc, "trace diag buffer allocation failed\n"
>  			"retrying smaller size %dKB\n", trace_size / 1024);
>  			goto retry_trace;
>  		} else
>  			diag_buffer->size =3D trace_size;

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260805110634.3466=
[email protected]?part=3D1