Re: [PATCH v2] scsi: mptfusion: Fix array out of bounds error

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch attempts to fix an array out-of-bounds error by clamping `ioc->facts.NumberOfPorts` to `ARRAY_SIZE(ioc->pfacts)` inside a newly introduced `mpt_get_adapter()` wrapper. However, this fix is incomplete and easily bypassed. The underlying hardware facts are dynamically refreshed during background hardware fault recovery by `GetIocFacts()`, which directly overwrites `ioc->facts` with raw firmware data using `memset` and hardware replies. Because `mpt_get_adapter()` is only called during device probing and suspend/resume, background recovery resets `NumberOfPorts` to the unbounded hardware value (e.g., 8). Subsequently, unprivileged users can read the S_IRUGO `/proc/mpt/iocX/info` file, triggering `mpt_iocinfo_proc_show()`. This function loops over the unbounded `NumberOfPorts`, causing out-of-bounds reads on the `pfacts` and `fc_port_page0` arrays (both sized to 2), leaking adjacent `MPT_ADAPTER` memory to user-space.
--

commit 09bc8d96f319385694c9b6744f49e39d39f37f63
Author: Alexey Gladkov <legion@kernel.org>

scsi: mptfusion: Fix array out of bounds error

The driver retrieves the number of ports from the hardware. However, the
driver can handle no more than two such ports and uses a fixed array.
This patch introduces mpt_get_adapter() to clamp the number of ports
to the array size to prevent out-of-bounds errors.

Link: https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/mptsas.h?ref_type=heads#L7
Link: https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/mptsas.c?ref_type=heads#L619

> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index 3a431ffd3e2eb..9e738d0bb8e35 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c

[ ... ]

> @@ -1741,6 +1741,21 @@ mpt_mapresources(MPT_ADAPTER *ioc)
>  	return r;
>  }
>  
> +MPT_ADAPTER *
> +mpt_get_adapter(struct pci_dev *pdev)
> +{
> +	MPT_ADAPTER *ioc = pci_get_drvdata(pdev);
> +
> +	if (ioc && ioc->facts.NumberOfPorts >= ARRAY_SIZE(ioc->pfacts)) {
> +		ioc->facts.NumberOfPorts = ARRAY_SIZE(ioc->pfacts);
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this fix completely prevent the
out-of-bounds access?

While this wrapper clamps the value during device probing and suspend/resume,
background recovery operations bypass it. Specifically, GetIocFacts() writes
raw firmware data directly into ioc->facts:

GetIocFacts() {
    facts = &ioc->facts;
    reply_sz = sizeof(*facts);
    memset(facts, 0, reply_sz);
    ... 
    r = mpt_handshake_req_reply_wait(ioc, req_sz, (u32*)&get_facts,
                                     reply_sz, (u16*)facts, ...);
}

If the hardware (or QEMU) provides a NumberOfPorts greater than 2 during
background recovery, the clamp is lost. Later, an unprivileged user reading
the S_IRUGO file /proc/mpt/iocX/info triggers mpt_iocinfo_proc_show(), which
uses the unclamped value:

mpt_iocinfo_proc_show() {
    for (p=0; p < ioc->facts.NumberOfPorts; p++) {
        ...
        ioc->fc_port_page0[p].WWNN.High,
}

Could this still lead to an out-of-bounds array read that leaks kernel
memory to user-space? Would it be safer to enforce this clamp directly
inside GetIocFacts() where the hardware data is received?

> +	BUILD_BUG_ON(ARRAY_SIZE(ioc->pfacts) != ARRAY_SIZE(ioc->fc_port_page0));
> +	BUILD_BUG_ON(ARRAY_SIZE(ioc->pfacts) != ARRAY_SIZE(ioc->fc_data.fc_port_page1));
> +
> +	return ioc;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616152908.363621-1-legion@kernel.org?part=1