Re: [PATCH] scsi: mpt3sas: validate variable event array spans

[sashiko-bot@kernel.org]

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

New issues:
- [High] TOCTOU vulnerability on shared DMA memory bypasses event validation

Pre-existing issues:
- [High] Missing bounds check for unhandled event types allows out-of-bounds read
- [High] Kernel panic in error path due to kfree() on a flexible array member
--

--- Patch [1]: [PATCH] scsi: mpt3sas: validate variable event array spans ---
commit 151ce14025a2487b9520990fe50767c45e6d12d5
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

scsi: mpt3sas: validate variable event array spans

This patch adds validation for SAS topology, PCIe topology, and IR configuration
events to ensure variable arrays fit within EventData before dispatch.
Malformed events are dropped after being acknowledged, while other event types
retain their existing behavior.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 79052f2accbd3..2a9ea81d1e2b0 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
[ ... ]
> @@ -1333,6 +1333,65 @@ _base_display_event_data(struct MPT3SAS_ADAPTER *ioc,
>  	ioc_info(ioc, "%s\n", desc);
>  }
>  
> +static bool
> +_base_variable_event_data_valid(struct MPT3SAS_ADAPTER *ioc,
> +				Mpi2EventNotificationReply_t *mpi_reply)
> +{
> +	const size_t event_offset = offsetof(Mpi2EventNotificationReply_t,
> +					     EventData);
> +	const void *event_data = mpi_reply->EventData;
> +	size_t event_data_len;
> +	size_t reply_len;
> +	u16 event;
> +
> +	event = le16_to_cpu(mpi_reply->Event);
> +	switch (event) {
> +	case MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST:
> +	case MPI2_EVENT_PCIE_TOPOLOGY_CHANGE_LIST:
> +	case MPI2_EVENT_IR_CONFIGURATION_CHANGE_LIST:
> +		break;
> +	default:
> +		return true;
> +	}

[Severity: High]
This is a pre-existing issue, but since we are adding validation here, does
returning true early leave other event types vulnerable to out-of-bounds
reads?

When mpt3sas_scsih_event_callback() processes unhandled events, it uses the
unvalidated EventDataLength to calculate the payload size:

    sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
    fw_event = alloc_fw_event_work(sz);
    ...
    memcpy(fw_event->event_data, mpi_reply->EventData, sz);

Could a compromised device supply a large EventDataLength (e.g. 0xFFFF) for
other events, causing memcpy() to read out-of-bounds from the ioc->reply
DMA pool?

> +
> +	reply_len = mpi_reply->MsgLength * 4;
> +	if (reply_len < event_offset || reply_len > ioc->reply_sz)
> +		return false;
> +
> +	event_data_len = le16_to_cpu(mpi_reply->EventDataLength) * 4;

[Severity: High]
Is there a Time-of-Check to Time-of-Use window here since mpi_reply is read
directly from shared DMA memory?

The validation function reads bounds from mpi_reply, but after validation
passes, mpt3sas_scsih_event_callback() re-reads fields like EventDataLength
and NumEntries directly from the same DMA-backed memory.

Could a malicious device mutate these fields in the window between validation
and use to bypass the checks introduced by this patch?

> +	if (event_data_len > reply_len - event_offset)
> +		return false;
> +
> +	switch (event) {
> +	case MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST: {
[ ... ]
> @@ -1521,6 +1586,8 @@ _base_async_event(struct MPT3SAS_ADAPTER *ioc, u8 msix_index, u32 reply)
>  	ioc->put_smid_default(ioc, smid);
>  
>   out:
> +	if (!event_data_valid)
> +		return 1;
>  
>  	/* scsih callback handler */
>  	mpt3sas_scsih_event_callback(ioc, msix_index, reply);

[Severity: High]
This isn't a bug introduced by this patch, but looking at
mpt3sas_scsih_event_callback(), is it safe to call kfree() on a flexible
array member in the error path?

If kzalloc() fails for fw_event->retries, the error handler does:

    ioc_err(ioc, "failure!\n");
    kfree(fw_event->event_data);
    fw_event_work_put(fw_event);
    return 1;

Since event_data is declared as a flexible array member at the end of
struct fw_event_work, doesn't passing this interior pointer to kfree()
trigger a kernel panic?  The subsequent fw_event_work_put() already
frees the whole structure.

>  
>  	/* ctl callback handler */
>  	mpt3sas_ctl_event_callback(ioc, msix_index, reply);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722041257.1557-1-pengpeng@iscas.ac.cn?part=1