Re: [PATCH] scsi: ppa: cancel delayed work before freeing on detach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] scsi: ppa: premature cancel_delayed_work_sync() in ppa_detach()
--

commit 917c5d0559eb52b27c70170022ca6e35b2cc8798
Author: Tristan Madani <tristan@talencesecurity.com>

scsi: ppa: cancel delayed work before freeing on detach

This commit adds a cancel_delayed_work_sync() call on dev->ppa_tq in
ppa_detach() to prevent ppa_interrupt() from accessing freed memory after
dev is freed.

> diff --git a/drivers/scsi/ppa.c b/drivers/scsi/ppa.c
> index 8a4e910d5758f..7f4b2a4157725 100644
> --- a/drivers/scsi/ppa.c
> +++ b/drivers/scsi/ppa.c
> @@ -1136,6 +1136,7 @@ static void ppa_detach(struct parport *pb)
>  	ppa_struct *dev;
>  	list_for_each_entry(dev, &ppa_hosts, list) {
>  		if (dev->dev->port == pb) {
> +			cancel_delayed_work_sync(&dev->ppa_tq);
>  			list_del_init(&dev->list);
>  			scsi_remove_host(dev->host);
>  			scsi_host_put(dev->host);

[Severity: Critical]
Is calling cancel_delayed_work_sync() before scsi_remove_host() in
drivers/scsi/ppa.c:ppa_detach() safe against new commands being queued or
in-flight commands draining during host removal?

Unlike disable_delayed_work_sync() (which is what drivers/scsi/imm.c uses in
imm_detach(), rather than cancel_delayed_work_sync() as mentioned in the
commit description), cancel_delayed_work_sync() does not prevent subsequent
schedule_delayed_work() calls.

When cancel_delayed_work_sync() returns here, dev->host has not been removed
yet and the SCSI device request queues are still active. Concurrent I/O or
shutdown commands issued during scsi_remove_host() -> scsi_forget_host() ->
__scsi_remove_device() -> device_del() -> sd_remove() -> sd_shutdown() can
still invoke ppa_queuecommand_lck() before blk_mq_destroy_queue() freezes the
queue:

drivers/scsi/ppa.c:ppa_queuecommand_lck() {
	...
	dev->cur_cmd = cmd;
	cmd->result = DID_ERROR << 16;	/* default return code */
	ppa_scsi_pointer(cmd)->phase = 0;	/* bus free */

	schedule_delayed_work(&dev->ppa_tq, 0);

	ppa_pb_claim(dev);
	...
}

If dev->ppa_tq is re-armed after cancel_delayed_work_sync() has already run,
and that command times out and is aborted by ppa_abort() or ppa_reset() (which
sets dev->cur_cmd = NULL without cancelling dev->ppa_tq), or if
ppa_interrupt() calls scsi_done(cmd) allowing scsi_remove_host() to finish
while ppa_interrupt() is still executing or re-arming itself, ppa_detach() can
proceed to kfree(dev) while dev->ppa_tq is still pending or running, leaving
the use-after-free in place.

Conversely, because this driver has no hardware interrupt and relies solely on
ppa_interrupt() polling via dev->ppa_tq to drive ppa_engine(), release the
parport claim, and complete commands:

drivers/scsi/ppa.c:ppa_interrupt() {
	...
	if (ppa_engine(dev, cmd)) {
		schedule_delayed_work(&dev->ppa_tq, 1);
		return;
	}
	...
	if (ppa_scsi_pointer(cmd)->phase > 1)
		ppa_disconnect(dev);

	ppa_pb_dismiss(dev);

	dev->cur_cmd = NULL;

	scsi_done(cmd);
}

cancelling dev->ppa_tq before scsi_remove_host() stops polling for any
already in-flight command (dev->cur_cmd != NULL). When scsi_remove_host() ->
__scsi_remove_device() -> blk_mq_destroy_queue() -> blk_mq_freeze_queue_wait()
waits for in-flight requests to complete, wouldn't it stall until the SCSI
command timeout fires, and also skip ppa_pb_dismiss(dev) before
parport_unregister_device(dev->dev)?

Should dev->ppa_tq be cancelled (or disabled with disable_delayed_work_sync())
after scsi_remove_host(dev->host) instead, so that in-flight and shutdown
commands can finish draining and ppa_queuecommand_lck() can no longer be
called to re-schedule the work?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019163821.659714-1-tristmd@gmail.com?part=1