[PATCH] scsi: mpt3sas: fix invalid kfree of embedded event_data in event callback

[PATCH] scsi: mpt3sas: fix invalid kfree of embedded event_data in event callback

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

fw_event->event_data is a flexible array member embedded within the
fw_event_work structure (allocated as a single kzalloc). Calling
kfree(fw_event->event_data) on it is invalid and can corrupt the heap,
since it is not a separately allocated pointer.

The subsequent fw_event_work_put() will decrement the refcount to zero
and call fw_event_work_free(), which frees the entire fw_event_work
structure (including the embedded event_data). The extra kfree() is
therefore both wrong and redundant.

Remove the erroneous kfree(fw_event->event_data) call in the
MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST error path. The PCIE topology
change list handler already handles this correctly without the extra
kfree.

Fixes: ad59571931072e6f ("scsi: mpt3sas: Add firmware event requeue support for busy devices")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 12caffe..ca01a49 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -12267,7 +12267,6 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
 		if (!fw_event->retries) {
 
 			ioc_err(ioc, "failure at %s:%d/%s()!\n",  __FILE__, __LINE__, __func__);
-			kfree(fw_event->event_data);
 			fw_event_work_put(fw_event);
 			return 1;
 		}
-- 
2.9.4

Re: [PATCH] scsi: mpt3sas: fix invalid kfree of embedded event_data in event callback

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver queues delayed works for firmware events but fails to cancel them properly during module unload and host removal, leading to a use-after-free and workqueue corruption when the delayed timer eventually fires.
- [High] Integer overflow in firmware event size calculation leads to out-of-bounds read on the kernel heap.
- [Medium] Missing bounds check on EventDataLength allows out-of-bounds read from the DMA pool, leaking data to userspace.
--

commit b3e94f572f1eaa0696e3b47cd0fd8d836c959627
Author: Li RongQing <lirongqing@baidu.com>

scsi: mpt3sas: fix invalid kfree of embedded event_data in event callback

This commit removes an erroneous kfree(fw_event->event_data) call in the
MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST error path. Since event_data is a
flexible array member embedded within the fw_event_work structure, freeing
it separately is invalid and can corrupt the heap. The extra kfree is also
redundant because the subsequent fw_event_work_put() frees the entire
structure.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 12caffeed3a0d..ca01a496f14c7 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -12267,7 +12267,6 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
>  		if (!fw_event->retries) {
>  
>  			ioc_err(ioc, "failure at %s:%d/%s()!\n",  __FILE__, __LINE__, __func__);
> -			kfree(fw_event->event_data);
>  			fw_event_work_put(fw_event);
>  			return 1;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this function
I noticed the size calculation can silently overflow:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:mpt3sas_scsih_event_callback() {
    ...
	u16 sz;
    ...
	sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
	fw_event = alloc_fw_event_work(sz);
    ...
}

If a device provides a large EventDataLength (e.g., 16385), le16_to_cpu() * 4
will overflow the 16-bit sz integer, wrapping it to a small value like 4.
alloc_fw_event_work() will allocate memory using this truncated size.

Later, could _scsih_sas_topology_change_event() cast this small buffer and
read fields from uninitialized adjacent heap memory, potentially causing
unexpected device removals or a denial of service?

[Severity: High]
This is a pre-existing issue, but does this code properly cancel the timer?

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_fw_event_cleanup_queue() {
    ...
		if (cancel_work_sync(&fw_event->work))
			fw_event_work_put(fw_event);
    ...
}

The driver queues delayed works for firmware events but calls
cancel_work_sync(&fw_event->work) instead of
cancel_delayed_work_sync(&fw_event->delayed_work) during module unload and
host removal.

Could this leave the timer active, leading to a use-after-free on the ioc
structure and workqueue corruption when the delayed timer eventually fires
after the ioc has been freed?

[Severity: Medium]
This is a pre-existing issue, but does this memcpy read out-of-bounds from
the DMA pool?

drivers/scsi/mpt3sas/mpt3sas_ctl.c:mpt3sas_ctl_add_to_event_log() {
    ...
		event_data_sz = le16_to_cpu(mpi_reply->EventDataLength)*4;
		sz = min_t(u32, event_data_sz, MPT3_EVENT_DATA_SIZE);
		memset(event_log[i].data, 0, MPT3_EVENT_DATA_SIZE);
		memcpy(event_log[i].data, mpi_reply->EventData, sz);
    ...
}

The function calculates event_data_sz from the untrusted EventDataLength
field and clamps it to MPT3_EVENT_DATA_SIZE (192 bytes), but fails to check
if this exceeds the actual hardware reply frame size (ioc->reply_sz,
typically 128 bytes).

Could the memcpy read past the end of the reply frame, pulling in data from
adjacent reply frames in the DMA pool and leaking it to userspace via the
MPT3EVENTREPORT ioctl?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706060840.2287-1-lirongqing@baidu.com?part=1