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 allocating segmented buffer entries, but passes the original unrounded size to the firmware, which can cause the firmware to read past the end of the segment list and DMA write to arbitrary physical memory.
- [Medium] A trace buffer size smaller than 4KB results in a 0-byte DMA allocation, 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 during 
driver unload. It adds endianness conversions for the diagnostic buffer sizes
read from Driver Page 1 and hardens the allocation retry loops against invalid
decrement sizes provided by the firmware.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=1
Closes: https://sashiko.dev/#/patchset/[email protected]?part=1
Closes: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_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)) {
>  
>  			retry = true;
> +
> +			if (!trace_dec_size || trace_dec_size > trace_size) {
> +				retry = false;
> +				goto retry_fw;
> +			}
> +
>  			trace_size -= 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 = (trace_size) / MPI3MR_PAGE_SIZE_4K;
    trace_size = 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 = trace_size;

When posting the buffer to firmware in mpi3mr_issue_diag_buf_post():

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

The firmware calculates the required number of segments using the unrounded
size (e.g., ceil(5120/4096) = 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 = trace_size / 4096
which evaluates to 0.

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

    diag_buffer_list = 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 = trace_size;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1