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

[Alexey Gladkov <legion@kernel.org>]

On Tue, Jun 16, 2026 at 01:04:22PM +0000, sashiko-bot@kernel.org wrote:
> 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?

Indeed. We could create a wrapper to retrieve MPT_ADAPTER that checks for
array index out-of-bounds errors. But there is one little catch here,
since we use NumberOfPorts to iterate over different arrays. We need to
synchronize their sizes.

> >  		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
> 

-- 
Rgrds, legion