Re: [PATCH] scsi: myrb: quiesce the SCSI host before controller teardown

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect teardown sequence in `myrb_cleanup()` causes use-after-free and page fault panics.
- [High] Data race and waitqueue corruption due to concurrent accesses to `cb->mcmd_blk` without locking.
- [High] Hardware command execution uses an unbounded `wait_for_completion()`, allowing unprivileged denial of service on hardware failure.
--

--- Patch [1]: [PATCH] scsi: myrb: quiesce the SCSI host before controller teardown ---
commit f8a31e3f5c2db4d7eb1518b32495ee728296ad52
Author: Hongyan Xu <[email protected]>

scsi: myrb: quiesce the SCSI host before controller teardown

This commit addresses teardown issues in the myrb driver by removing the
SCSI host before releasing controller resources. It stops the monitor and
destroys pools before unmapping hardware and dropping the host reference
to ensure callbacks do not run after teardown.

> diff --git a/drivers/scsi/myrb.c b/drivers/scsi/myrb.c
> index 3678b66310ed0..adb2fadf9146e 100644
> --- a/drivers/scsi/myrb.c
> +++ b/drivers/scsi/myrb.c
> @@ -3496,9 +3496,10 @@ static void myrb_remove(struct pci_dev *pdev)
>  	struct myrb_hba *cb = pci_get_drvdata(pdev);
>  
>  	shost_printk(KERN_NOTICE, cb->host, "Flushing Cache...");
> +	scsi_remove_host(cb->host);
>  	myrb_exec_type3(cb, MYRB_CMD_FLUSH, 0);

[Severity: High]
This isn't a bug introduced by this patch, but does hardware command execution
use an unbounded wait_for_completion()?

In drivers/scsi/myrb.c:myrb_exec_cmd() (called by myrb_exec_type3()):

	spin_lock_irqsave(&cb->queue_lock, flags);
	cb->qcmd(cb, cmd_blk);
	spin_unlock_irqrestore(&cb->queue_lock, flags);

	wait_for_completion(&cmpl);
	return cmd_blk->status;

myrb_exec_cmd() submits a command to the controller and blocks indefinitely
waiting for a hardware interrupt. There is no timeout or fallback wake
condition.

If the controller firmware crashes, drops an interrupt, or the device is
abruptly removed, the completion is never signaled. Since unprivileged local
users can trigger hardware commands through world-readable sysfs attributes
(like reading rebuild), could this allow them to intentionally hang a
kernel task if they can induce or encounter a hardware error state?

> -	myrb_cleanup(cb);
>  	myrb_destroy_mempools(cb);

[Severity: High]
This is a pre-existing issue, but does myrb_get_rbld_progress() have a data
race on cb->mcmd_blk?

In drivers/scsi/myrb.c:myrb_get_rbld_progress():

	struct myrb_cmdblk *cmd_blk = &cb->mcmd_blk;
	union myrb_cmd_mbox *mbox = &cmd_blk->mbox;
...
	myrb_reset_cmd(cmd_blk);
	mbox->type3.id = MYRB_MCMD_TAG;
	mbox->type3.opcode = MYRB_CMD_GET_REBUILD_PROGRESS;
	mbox->type3.addr = rbld_addr;
	status = myrb_exec_cmd(cb, cmd_blk);

myrb_get_rbld_progress() accesses and resets the shared cb->mcmd_blk and its
waitqueue completion without any serialization.

myrb_reset_cmd(cmd_blk) re-initializes cmd_blk->completion. If an
unprivileged user concurrently reads the rebuild sysfs attribute while the
background monitor work myrb_update_rbld_progress() is running, both will
modify cb->mcmd_blk at the same time.

One thread could re-initialize the completion while the other is blocked in
wait_for_completion(), corrupting the waitqueue list.

Could this corrupt the waitqueue list and lead to deadlocks or kernel crashes?

> +	myrb_cleanup(cb);
>  }

[Severity: High]
This is a pre-existing issue, but I noticed an incorrect teardown sequence in
myrb_cleanup() that could lead to a use-after-free and page fault panics.

In drivers/scsi/myrb.c:myrb_cleanup():

	/* Free the memory mailbox, status, and related structures */
	myrb_unmap(cb);

	if (cb->mmio_base) {
		if (cb->disable_intr)
			cb->disable_intr(cb->io_base);
		iounmap(cb->mmio_base);
	}
	if (cb->irq)
		free_irq(cb->irq, cb);

The teardown logic frees DMA memory (cb->first_stat_mbox and
cb->first_cmd_mbox via myrb_unmap) and unmaps the MMIO base before freeing
the shared IRQ line.

If an interrupt fires on the shared IRQ line during this window, the
interrupt handler (e.g., DAC960_LA_intr_handler) accesses the freed DMA
memory and the unmapped MMIO base without knowing they have been released:

drivers/scsi/myrb.c:DAC960_LA_intr_handler():

	DAC960_LA_ack_intr(base);
	next_stat_mbox = cb->next_stat_mbox;
	while (next_stat_mbox->valid) {

Could this cause a kernel panic if the shared IRQ fires before free_irq() is
called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1