Re: [PATCH v2 05/17] mpi3mr: Fix buffer overflow when caching log data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unvalidated firmware input allows a small `reply_frame_size`, leading to `logdata_entry_sz == 0`, which causes a ZERO_SIZE_PTR dereference and Division by Zero.
- [High] Integer overflow in `sz` calculation in `mpi3mr_get_logdata` leads to silent data truncation.
- [High] Missing locking and memory barriers for `logdata_buf` between BSG ioctl handlers and interrupt context causes torn reads and potential uninitialized memory access.
- [High] MPI3_EVENT_DEVICE_INFO_CHANGED falls through to MPI3_EVENT_LOG_DATA, ignoring device info changes and polluting log data.
- [High] Copying into a 1-element trailing array will trigger a FORTIFY_SOURCE panic under -fstrict-flex-arrays=3.
--

commit 64e5ca8eae679f9947b56b20427a7ee194af1d35
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix buffer overflow when caching log data

This patch modifies mpi3mr_app_save_logdata_th to calculate the payload
room first and clamp the copy to it. It prevents a buffer overflow when
the entry size is derived from the controller reply size and is smaller
than the header.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 94b992acb2330..cd772b2cb98ae 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2947,7 +2947,8 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  void mpi3mr_app_save_logdata_th(struct mpi3mr_ioc *mrioc, char *event_data,
>  	u16 event_data_size)
>  {
> -	u32 index = mrioc->logdata_buf_idx, sz;
> +	u32 index = mrioc->logdata_buf_idx;
> +	size_t entry_payload_len, sz;
>  	struct mpi3mr_logdata_entry *entry;
>  
>  	if (!(mrioc->logdata_buf))
>  		return;

[Severity: High]
This is a pre-existing issue, but are there missing memory barriers or locking
when accessing mrioc->logdata_buf?

mpi3mr_enable_logdata() allocates and initializes mrioc->logdata_buf,
mrioc->logdata_entry_sz, and mrioc->logdata_buf_idx from a user-space
BSG ioctl context without any serialization against the interrupt handler.

If firmware events trigger mpi3mr_app_save_logdata_th() via the hardware
interrupt handler concurrently, the lack of memory barriers might allow the
interrupt handler to observe a non-NULL buffer but an uninitialized size/index,
leading to corrupt calculations or out-of-bounds writes.

Additionally, user space copies the log buffer without serialization:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    ...
    sg_copy_from_buffer(job->request_payload.sg_list,
                        job->request_payload.sg_cnt,
                        mrioc->logdata_buf, sz);
    ...
}

Could this lead to torn reads or corrupted logs if the interrupt handler
overwrites the ring buffer entries during the user-space copy operation?

>  
>  	entry = (struct mpi3mr_logdata_entry *)
>  		(mrioc->logdata_buf + (index * mrioc->logdata_entry_sz));
>  	entry->valid_entry = 1;

[Severity: High]
This is a pre-existing issue, but might this dereference a ZERO_SIZE_PTR and
cause a kernel panic?

If the device firmware sets reply_frame_size to 5 (or less),
mrioc->reply_sz becomes 20 (or less). In mpi3mr_enable_logdata():

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_enable_logdata() {
    ...
    mrioc->logdata_entry_sz =
        (mrioc->reply_sz - (sizeof(struct mpi3_event_notification_reply) - 4))
        + MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ;
    ...
}

Since the sizeof expression evaluates as a size_t, 20 - 24 underflows. The
result plus 4 wraps around to exactly 0 when assigned to the u16
logdata_entry_sz.

This causes kcalloc to return ZERO_SIZE_PTR (0x10). The check
if (!mrioc->logdata_buf) earlier in this function fails to catch this, leading
to a panic when writing to the unmapped address 0x10.

Additionally, could this cause a division by zero in mpi3mr_get_logdata()
when calculating num_entries?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    ...
    num_entries = job->request_payload.payload_len / entry_sz;
    ...
}

> -	sz = min(mrioc->logdata_entry_sz, event_data_size);
> +	if (mrioc->logdata_entry_sz > MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ)
> +		entry_payload_len = (size_t)mrioc->logdata_entry_sz -
> +		    MPI3MR_BSG_LOGDATA_ENTRY_HEADER_SZ;
> +	else
> +		entry_payload_len = 0;
> +	sz = min_t(size_t, entry_payload_len, event_data_size);
>  	memcpy(entry->data, event_data, sz);

[Severity: High]
This isn't a bug introduced by this patch, but will this memcpy trigger a
FORTIFY_SOURCE panic under -fstrict-flex-arrays=3?

The log data payload is copied into entry->data, which is defined as
__u8 data[1]; in the UAPI header struct mpi3mr_logdata_entry.

When compiled with -fstrict-flex-arrays=3, the compiler treats data strictly
as a 1-byte array. The fortified memcpy() will evaluate __member_size() as 1,
and since log events typically have a size > 1, this will deterministically
trigger a kernel panic upon receiving a log data event due to the detected
buffer overflow.

>  	mrioc->logdata_buf_idx =
>  		((++index) % MPI3MR_BSG_LOGDATA_MAX_ENTRIES);

[Severity: High]
This isn't a bug introduced by this patch, but could the size calculation in
mpi3mr_get_logdata() overflow and lead to silent data truncation?

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    u16 num_entries, sz, entry_sz = mrioc->logdata_entry_sz;
    ...
    sz = num_entries * entry_sz;
    ...
}

num_entries can be up to 400 (MPI3MR_BSG_LOGDATA_MAX_ENTRIES), and entry_sz is
typically 236 bytes. The product 400 * 236 equals 94400, which exceeds the
maximum value of a 16-bit unsigned integer (65535). 

Might this silently truncate/wrap around to 28864, causing
sg_copy_from_buffer() to return incomplete or corrupted log data back to
user space without generating an error?

[Severity: High]
This is a pre-existing issue, but is there an unintentional fall-through from
MPI3_EVENT_DEVICE_INFO_CHANGED to MPI3_EVENT_LOG_DATA?

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events() {
    ...
    case MPI3_EVENT_DEVICE_INFO_CHANGED:
    case MPI3_EVENT_LOG_DATA:

        if (mrioc->reply_sz > offsetof(struct mpi3_event_notification_reply,
                                       event_data))
            avail_reply_room = mrioc->reply_sz -
                offsetof(struct mpi3_event_notification_reply, event_data);
    ...
}

The switch case for MPI3_EVENT_DEVICE_INFO_CHANGED lacks the required
process_evt_bh = 1; assignment and a break; statement. 

Will this cause device information changes to be permanently ignored (since
they are never queued to the fwevt_worker_thread), while erroneously passing the
device page0 data payload to mpi3mr_app_save_logdata_th(), polluting the
log data ring buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825210411.301535-1-chandrakanth.patil@broadcom.com?part=5