Re: [PATCH net v2 1/1] nfc: digital: fix use-after-free in unregister

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107141910.549465-1-xuyuqiabc@gmail.com?part=1
---
nfc: digital: fix use-after-free in unregister

This patch aborts the in-flight driver command in
nfc_digital_unregister_device() before freeing the command queue. It
also blocks new submissions once shutting_down is set and frees any
leftover req/resp skbs. Per-driver changes in port100, nfcsim,
trf7970a and st95hf try to make sure no completion callback can run
after abort_cmd() returns.

> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
> index ddfc58c299649..87c833abb36af 100644
> --- a/drivers/nfc/trf7970a.c
> +++ b/drivers/nfc/trf7970a.c
> @@ -1903,6 +1903,7 @@ static void trf7970a_abort_cmd(struct nfc_digital_dev *ddev)
>  		trf->aborting = true;
>  		break;
>  	case TRF7970A_ST_LISTENING:
> +	case TRF7970A_ST_LISTENING_MD:
>  		trf->ignore_timeout = !cancel_delayed_work(&trf->timeout_work);
>  		trf7970a_send_err_upstream(trf, -ECANCELED);
>  		dev_dbg(trf->dev, "Abort process complete\n");

[Severity: Low]

The commit message presents this as part of the unregister fix ("handle
its MD listening state in abort and shutdown"). Does this abort_cmd case
take effect on the unregister path?

trf7970a_remove() calls trf7970a_shutdown() first. With this patch,
every active state, LISTENING_MD included, goes through
send_err_upstream()->switch_rf_off()->power_down() and ends in PWR_OFF.
When nfc_digital_unregister_device() later calls digital_abort_cmd(),
trf7970a_abort_cmd() sees PWR_OFF and takes the default: break case.

This hunk does change behaviour on the normal digital_stop_poll(),
digital_dep_link_down() and digital_deactivate_target() paths. An abort
during MD listening now cancels the timeout and sends -ECANCELED
upstream right away. Before, the listen ran on until -ETIMEDOUT or until
a target arrived.

The same applies to suspend, because trf7970a_suspend() calls
trf7970a_shutdown(). Before this patch, LISTENING_MD hit the default
case there, power_down() returned -EBUSY, and the chip stayed powered.

Could the commit message describe these behaviour changes? A stable
backporter would otherwise assume all of them are needed for the
use-after-free fix.

> @@ -2017,6 +2018,8 @@ static void trf7970a_shutdown(struct trf7970a *trf)
>  	case TRF7970A_ST_WAIT_FOR_RX_DATA_CONT:
>  	case TRF7970A_ST_WAIT_TO_ISSUE_EOF:
>  	case TRF7970A_ST_LISTENING:
> +	case TRF7970A_ST_LISTENING_MD:
> +		trf->ignore_timeout = !cancel_delayed_work(&trf->timeout_work);

[Severity: High]

Can this assignment overwrite an ignore_timeout = true that the IRQ
thread set for a timeout handler that is already running?

Say the state is WAIT_FOR_TX_FIFO or WAIT_FOR_RX_DATA. The timeout
expires and trf7970a_timeout_work_handler() (H1) starts running, but it
has not taken trf->lock yet.

trf7970a_irq() holds trf->lock and runs:

	case TRF7970A_ST_WAIT_FOR_TX_FIFO:
		if (status & TRF7970A_IRQ_STATUS_TX) {
			trf->ignore_timeout =
			    !cancel_delayed_work(&trf->timeout_work);
			trf7970a_fill_fifo(trf);

H1 is running, so the cancel fails and ignore_timeout becomes true. Then
fill_fifo()->trf7970a_transmit() re-arms timeout_work. The SRX branch
does the same via trf7970a_drain_fifo().

Now trf7970a_remove() or trf7970a_suspend() takes trf->lock before H1
does. This new line cancels the re-armed work, the cancel returns true,
and ignore_timeout becomes false. send_err_upstream(-ECANCELED)
completes the command, and the state ends in PWR_OFF.

H1 then takes the lock, sees ignore_timeout == false, and falls through
to:

	else
		trf7970a_send_err_upstream(trf, -ETIMEDOUT);

That leads to:

trf7970a_send_upstream()
  trf->cb(trf->ddev, trf->cb_arg, trf->rx_skb)
    digital_send_cmd_complete()
      cmd->resp = resp;
      schedule_work(&ddev->cmd_complete_work);

trf->cb_arg is still the old one. Doesn't this write cmd->resp into a
digital_cmd that was already freed, either by digital_wq_cmd_complete()
or by the new cleanup loop in nfc_digital_unregister_device()?

It also re-queues ddev->cmd_complete_work after unregister's final
cancel_work_sync(). On remove, the new cancel_delayed_work_sync() only
waits for H1 after the fact, and nfc_digital_free_device() then frees
ddev while cmd_complete_work is still queued. On suspend, the second
completion either hits the freed cmd or completes an unrelated queued
command with resp == NULL.

Before this patch, shutdown did not touch ignore_timeout in these
states. In the same interleaving, send_err_upstream() cancelled the
re-armed work, ignore_timeout stayed true, and H1 did nothing.

H1 also moves the state from PWR_OFF to IDLE here. Doesn't that
contradict the new comment in trf7970a_remove() that no command path
can arm the timeout again?

>  		trf7970a_send_err_upstream(trf, -ECANCELED);
>  		fallthrough;
>  	case TRF7970A_ST_IDLE:
> @@ -2230,6 +2233,14 @@ static void trf7970a_remove(struct spi_device *spi)
>  	mutex_unlock(&trf->lock);
>  
>  	nfc_digital_unregister_device(trf->ddev);
> +
> +	/*
> +	 * shutdown() leaves the chip powered off, so no IRQ or command path
> +	 * can arm the timeout again and unregister has cancelled cmd_work.
> +	 * Wait for a handler that was already running before freeing trf.
> +	 */
> +	cancel_delayed_work_sync(&trf->timeout_work);

[Severity: High]

Can a timeout handler that is already running still deliver a
completion into a freed digital_cmd before this point?

The IRQ thread's error paths call trf7970a_send_err_upstream() without
setting ignore_timeout. For example, in trf7970a_irq():

		} else {
			trf7970a_send_err_upstream(trf, -EIO);
		}

The WAIT_TO_ISSUE_EOF case and the LISTENING/LISTENING_MD !NFC_RF cases
do the same. send_err_upstream() only calls the non-sync
cancel_delayed_work(), which cannot stop a handler that has already
started.

Suppose the timeout handler H1 is running when such an IRQ completes the
command. The state becomes IDLE and ignore_timeout stays false.
trf7970a_shutdown() in IDLE only calls switch_rf_off() and power_down(),
ending in PWR_OFF, and does not touch ignore_timeout.

H1 then sees PWR_OFF with ignore_timeout == false and runs:

trf7970a_timeout_work_handler()
  trf7970a_send_err_upstream(trf, -ETIMEDOUT)
    trf7970a_send_upstream()
      trf->cb(trf->ddev, trf->cb_arg, trf->rx_skb)
        digital_send_cmd_complete()
          cmd->resp = resp;
          schedule_work(&ddev->cmd_complete_work);

trf->cb_arg is never cleared, so this cmd has already been freed, either
by digital_wq_cmd_complete() or by the cleanup loop in
nfc_digital_unregister_device(). cmd_complete_work is also re-queued
after unregister's final cancel_work_sync().

This cancel_delayed_work_sync() only waits for H1 to finish, after that
has already happened. nfc_digital_free_device() then frees ddev with
cmd_complete_work still queued.

The race between the IRQ and timeout completions is older than this
patch. However, the commit message says trf7970a's timeout work and IRQ
completions are handled for unregister, and the new comment says remove
waits for a handler that was already running. Is the trf7970a part of
the fix complete?

> +
>  	nfc_digital_free_device(trf->ddev);
>  
>  	regulator_disable(trf->vddio_regulator);
> diff --git a/include/net/nfc/digital.h b/include/net/nfc/digital.h
> index bb3e8fdc06929..d5ae45da5a337 100644
> --- a/include/net/nfc/digital.h
> +++ b/include/net/nfc/digital.h
> @@ -131,7 +131,11 @@ typedef void (*nfc_digital_cmd_complete_t)(struct nfc_digital_dev *ddev,
>   * @switch_rf: Turns device radio on or off. The stack does not call explicitly
>   *	switch_rf to turn the radio on. A call to in|tg_configure_hw must turn
>   *	the device radio on.
> - * @abort_cmd: Discard the last sent command.
> + * @abort_cmd: Discard the last sent command. When called from
> + *	nfc_digital_unregister_device(), the command is freed after this
> + *	returns, so the driver must ensure that its completion callback
> + *	cannot run afterwards. Completions delivered from work items, IRQ
> + *	handlers or timers must be cancelled or flushed before returning.

[Severity: Low]

Do the in-tree implementations follow this new contract?

st95hf_abort_cmd() is still an empty stub:

/* TODO st95hf_abort_cmd */
static void st95hf_abort_cmd(struct nfc_digital_dev *ddev)
{
}

trf7970a_abort_cmd() in the WAIT_FOR_TX_FIFO, WAIT_FOR_RX_DATA,
WAIT_FOR_RX_DATA_CONT and WAIT_TO_ISSUE_EOF states only sets
trf->aborting = true. The completion is left for the IRQ thread or the
timeout work to deliver later.

st95hf is safe only because st95hf_remove() holds rm_lock across
unregister and free, and sets nfcdev_free. trf7970a relies on calling
shutdown() before unregister.

Should the documentation allow driver-side serialization in remove() as
an alternative? Or should these abort_cmd() implementations actually
stop or drain their IRQ- and timer-driven completions?

>   *
>   * Notes: Asynchronous functions have a timeout parameter. It is the driver
>   *	responsibility to call the digital stack back through the

[ ... ]