Re: [PATCH] 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:
- [Critical] Incomplete fix: Out-of-bounds array access in fusion sub-drivers due to unclamped ioc->facts.NumberOfPorts
--

commit c993313579593bc432c039b88b1e748a26bc690c
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. It uses a fixed array for
them.

> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index 3a431ffd3e2eb..05bd556bb9383 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -3257,6 +3257,8 @@ GetPortFacts(MPT_ADAPTER *ioc, int portnum, int sleepFlag)
>  		return -4;
>  	}
>  
> +	BUG_ON(portnum < 0 || portnum >= ARRAY_SIZE(ioc->pfacts));
> +
>  	pfacts = &ioc->pfacts[portnum];

[ ... ]

> @@ -6701,6 +6703,7 @@ static int mpt_iocinfo_proc_show(struct seq_file *m, void *v)
>  	char		 expVer[32];
>  	int		 sz;
>  	int		 p;
> +	int		 numberOfPorts = MIN(ioc->facts.NumberOfPorts, ARRAY_SIZE(ioc->pfacts));
>  
>  	mpt_get_fw_exp_ver(expVer, ioc);
>  
> @@ -6755,7 +6758,7 @@ static int mpt_iocinfo_proc_show(struct seq_file *m, void *v)
>  	seq_printf(m, "  MaxBuses = %d\n", ioc->facts.MaxBuses);
>  
>  	/* per-port info */
> -	for (p=0; p < ioc->facts.NumberOfPorts; p++) {
> +	for (p = 0; p < numberOfPorts; p++) {

[Severity: Critical]
This is a pre-existing issue, but by only capping numberOfPorts locally here,
does this leave the rest of the fusion sub-drivers vulnerable to out-of-bounds
accesses?

The unclamped ioc->facts.NumberOfPorts is still used in several loops across
the driver, where the bounds of statically sized arrays (like pfacts or
fc_port_page1, both size 2) are exceeded if hardware reports more than 2
ports.

For instance, in drivers/message/fusion/mptfc.c:mptfc_remove():

	for (ii=0; ii<ioc->facts.NumberOfPorts; ii++) {
		if (ioc->fc_data.fc_port_page1[ii].data) {
			dma_free_coherent(&ioc->pcidev->dev,
					  ioc->fc_data.fc_port_page1[ii].pg_sz,
					  ioc->fc_data.fc_port_page1[ii].data,
					  ioc->fc_data.fc_port_page1[ii].dma);

Since fc_port_page1 is size 2, if NumberOfPorts is 8, could this read
adjacent struct members of MPT_ADAPTER and pass them to dma_free_coherent(),
leading to arbitrary invalid frees and a kernel panic?

Similar loops iterating over pfacts exist in mptfc_probe(), mptsas_probe(),
mptspi_probe(), and mptlan_probe().

Would it be safer to clamp ioc->facts.NumberOfPorts directly after retrieving
it from firmware, so all sub-drivers are protected?

>  		seq_printf(m, "  PortNumber = %d (of %d)\n",
>  				p+1,
>  				ioc->facts.NumberOfPorts);

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