Re: [PATCH 23/37] scsi: mpt3sas: Validate phy_number in mpt3sas_transport_update_links

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: nr_phys_allocated check breaks mpt3sas_transport_update_links()
--

commit 228d8378247f2aec196daebe40235a2011620814
Author: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>

scsi: mpt3sas: Validate phy_number in mpt3sas_transport_update_links

This commit adds a bounds check on phy_number in
mpt3sas_transport_update_links() before indexing sas_node->phy.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> index 32db07ed808a9..1041ee7591bfb 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
> @@ -1176,6 +1176,11 @@ mpt3sas_transport_update_links(struct MPT3SAS_ADAPTER *ioc,
>  		return;
>  	}
>  
> +	if (phy_number >= sas_node->nr_phys_allocated) {
> +		spin_unlock_irqrestore(&ioc->sas_node_lock, flags);
> +		return;
> +	}

[Severity: High]
Should this check against sas_node->num_phys (as described in the commit
message) instead of sas_node->nr_phys_allocated?

In mpt3sas_transport_update_links(), _transport_sas_node_find_by_sas_address()
returns either &ioc->sas_hba or an expander struct _sas_node:

drivers/scsi/mpt3sas/mpt3sas_transport.c:_transport_sas_node_find_by_sas_address()
	if (ioc->sas_hba.sas_address == sas_address)
		return &ioc->sas_hba;
	else
		return mpt3sas_scsih_expander_find_by_sas_address(ioc,
		    sas_address, port);

For expander nodes allocated in _scsih_expander_add(), the phy array is sized
using sas_expander->num_phys, while nr_phys_allocated is never initialized and
remains 0:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_expander_add()
	sas_expander->num_phys = expander_pg0.NumPhys;
	...
	sas_expander->phy = kzalloc_objs(struct _sas_phy,
					 sas_expander->num_phys);

Because phy_number is unsigned, phy_number >= sas_node->nr_phys_allocated is
always true for expanders, causing callers such as
_scsih_refresh_expander_links() and _scsih_sas_topology_change_event() to
return early without updating expander phy links or adding ports for attached
devices.

Additionally, for ioc->sas_hba, _scsih_sas_host_add() sets nr_phys_allocated
to at least MPT_MAX_HBA_NUM_PHYS (32) while only initializing num_phys
entries:

drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_sas_host_add()
	ioc->sas_hba.nr_phys_allocated = max_t(u8,
	    MPT_MAX_HBA_NUM_PHYS, num_phys);
	ioc->sas_hba.phy = kzalloc_objs(struct _sas_phy,
					ioc->sas_hba.nr_phys_allocated);
	...
	ioc->sas_hba.num_phys = num_phys;

Would a phy_number between ioc->sas_hba.num_phys and
ioc->sas_hba.nr_phys_allocated index an uninitialized _sas_phy entry where
mpt3sas_phy->phy is NULL, leading to a NULL pointer dereference in
mpt3sas_transport_add_phy_to_an_existing_port() -> _transport_add_phy() or in
the MPT_DEBUG_TRANSPORT dev_printk() call?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1793925772-1303-1-git-send-email-shivasharan.srikanteshwara@broadcom.com?part=23