Re: [PATCH] scsi: myrb: unregister the SCSI host on remove

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] scsi: myrb: UAF of cb and active monitor_work in myrb_remove() teardown
--

commit b7fd5614b253ea1ce6d98ee7d37937a94b3e4183
Author: Wentao Liang <vulab@iscas.ac.cn>

scsi: myrb: unregister the SCSI host on remove

This commit adds a call to scsi_remove_host() in myrb_remove() so that the
SCSI host is unregistered and its device registration references are dropped
when the PCI device is removed.

> diff --git a/drivers/scsi/myrb.c b/drivers/scsi/myrb.c
> index 95df6e5345f7a..e43d86f139c12 100644
> --- a/drivers/scsi/myrb.c
> +++ b/drivers/scsi/myrb.c
> @@ -3496,6 +3496,7 @@ static void myrb_remove(struct pci_dev *pdev)
>  {
>  	struct myrb_hba *cb = pci_get_drvdata(pdev);
>  
> +	scsi_remove_host(cb->host);
>  	shost_printk(KERN_NOTICE, cb->host, "Flushing Cache...");
>  	myrb_exec_type3(cb, MYRB_CMD_FLUSH, 0);
>  	myrb_cleanup(cb);

[Severity: Critical]
Does calling scsi_remove_host(cb->host) here trigger a use-after-free of cb
when myrb_remove() subsequently calls myrb_destroy_mempools(cb)?

In drivers/scsi/myrb.c:myrb_detect(), struct myrb_hba *cb is allocated inline
within struct Scsi_Host via scsi_host_alloc() and obtained with
shost_priv(shost):

drivers/scsi/myrb.c:myrb_detect() {
	shost = scsi_host_alloc(&myrb_template, sizeof(struct myrb_hba));
	if (!shost) {
		dev_err(&pdev->dev, "Unable to allocate Controller\n");
		return NULL;
	}
	shost->max_cmd_len = 12;
	shost->max_lun = 256;
	cb = shost_priv(shost);
}

Now that scsi_remove_host(cb->host) drops the device registration references
taken by scsi_add_host(), the scsi_host_put(cb->host) call at the end of
myrb_cleanup(cb) drops the final reference on shost->shost_gendev and frees
both shost and cb via scsi_host_dev_release():

drivers/scsi/myrb.c:myrb_cleanup() {
	...
	pci_set_drvdata(pdev, NULL);
	pci_disable_device(pdev);
	scsi_host_put(cb->host);
}

Immediately after myrb_cleanup(cb) returns, myrb_remove() calls
myrb_destroy_mempools(cb), which dereferences the freed cb pointer:

drivers/scsi/myrb.c:myrb_remove() {
	struct myrb_hba *cb = pci_get_drvdata(pdev);

	scsi_remove_host(cb->host);
	shost_printk(KERN_NOTICE, cb->host, "Flushing Cache...");
	myrb_exec_type3(cb, MYRB_CMD_FLUSH, 0);
	myrb_cleanup(cb);
	myrb_destroy_mempools(cb);
}

drivers/scsi/myrb.c:myrb_destroy_mempools() {
	cancel_delayed_work_sync(&cb->monitor_work);
	destroy_workqueue(cb->work_q);

	dma_pool_destroy(cb->sg_pool);
	dma_pool_destroy(cb->dcdb_pool);
}

In addition, because cb->monitor_work and cb->work_q are only stopped in
myrb_destroy_mempools(), running myrb_cleanup() first leaves myrb_monitor()
able to run concurrently while myrb_cleanup() -> myrb_unmap() frees the DMA
buffers and mailboxes (cb->enquiry, cb->err_table, cb->ldev_info_buf,
cb->first_stat_mbox, cb->first_cmd_mbox), unmaps cb->mmio_base, and frees the
IRQ.

Should myrb_destroy_mempools(cb) be called before myrb_cleanup(cb), matching
the order used in the myrb_probe() error path and in myrs_remove()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104021419.1468133-1-vulab@iscas.ac.cn?part=1