Re: [PATCH 03/37] scsi: mpt3sas: Pin Scsi_Host while an ioctl is using its ioc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: NULL ioc->shost dereference in _ctl_verify_adapter()
--

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

scsi: mpt3sas: Pin Scsi_Host while an ioctl is using its ioc

This commit acquires a Scsi_Host reference in _ctl_verify_adapter() and
releases it at the end of _ctl_ioctl_main() to prevent the adapter from
being torn down while an ioctl is in flight.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 8bb947004885b..f254e9984da80 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -447,6 +447,11 @@ _ctl_verify_adapter(int ioc_number, struct MPT3SAS_ADAPTER **iocpp,
>  				continue;
>  		}
>  out:
> +		if (!scsi_host_get(ioc->shost)) {
> +			spin_unlock(&gioc_lock);
> +			*iocpp = NULL;
> +			return -1;
> +		}

[Severity: High]
Can this dereference a NULL ioc->shost pointer if _ctl_ioctl_main() runs
concurrently with _scsih_probe()?

In _mpt3sas_init(), mpt3sas_ctl_init() registers the /dev/mpt3ctl and
/dev/mpt2ctl misc devices before pci_register_driver() probes adapters, and
the device nodes also remain accessible during subsequent PCI hotplug or
driver bind operations.

In drivers/scsi/mpt3sas/mpt3sas_scsih.c:_scsih_probe(), ioc is added to
mpt3sas_ioc_list under gioc_lock before ioc->shost is assigned:

	INIT_LIST_HEAD(&ioc->list);
	spin_lock(&gioc_lock);
	list_add_tail(&ioc->list, &mpt3sas_ioc_list);
	spin_unlock(&gioc_lock);
	ioc->shost = shost;

If _ctl_verify_adapter() finds ioc on mpt3sas_ioc_list before
ioc->shost = shost runs, it passes NULL to scsi_host_get():

struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
{
	if (scsi_get_host_state(shost) == SHOST_DEL ||
		!get_device(&shost->shost_gendev))
		return NULL;
	return shost;
}

which immediately dereferences shost while holding gioc_lock.

Even right after ioc->shost = shost is assigned in _scsih_probe(), the host
is in SHOST_CREATED state (not SHOST_DEL), so scsi_host_get(ioc->shost)
succeeds before _scsih_probe() initializes ioc->pci_access_mutex, sets
ioc->is_driver_loading = 1, or initializes ioc->ctl_cmds.mutex in
mpt3sas_base_attach().

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