Re: [PATCH net] nfc: llcp: use timer_shutdown_sync to prevent timer rearm

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete.

--

nfc: llcp: use timer_shutdown_sync to prevent timer rearm

This commit switches link_timer and sdreq_timer in local_cleanup() from
timer_delete_sync() to timer_shutdown_sync(). The goal is to stop the
associated work items from re-arming the timers while cancel_work_sync()
runs, so that no deferred work runs after kfree(local).

> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index 74bf817007cf2..a098d72a15ba8 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -173,14 +173,14 @@ static struct nfc_llcp_local *nfc_llcp_local_get(struct nfc_llcp_local *local)
>  static void local_cleanup(struct nfc_llcp_local *local)
>  {
>  	nfc_llcp_socket_release(local, false, ENXIO);
> -	timer_delete_sync(&local->link_timer);
> +	timer_shutdown_sync(&local->link_timer);
>  	skb_queue_purge(&local->tx_queue);
>  	cancel_work_sync(&local->tx_work);
>  	cancel_work_sync(&local->rx_work);

[Severity: High]
Does this fully close the use-after-free described in the commit message?
The timers can no longer be re-armed. But tx_work, rx_work, timeout_work
and sdreq_timeout_work still use cancel_work_sync(), and that does not
stop them from being queued again afterwards.

tx_work is cancelled before rx_work, and nfc_llcp_rx_work() always queues
tx_work:

net/nfc/llcp_core.c:nfc_llcp_rx_work() {
    ...
	nfc_llcp_rx_skb(local, skb);

	schedule_work(&local->tx_work);
    ...
}

Suppose rx_work is running when local_cleanup() reaches
cancel_work_sync(&local->rx_work). That cancel waits for it, and in the
meantime rx_work queues tx_work again. local_cleanup() then returns and
local_release_work() does:

	local_cleanup(local);
	kfree(local);

Can nfc_llcp_tx_work() then run on the freed local? It would touch
local->tx_queue, call nfc_data_exchange(local->dev, ...) and call
mod_timer(&local->link_timer). The pending tx_work work_struct inside the
freed object would also still be linked on system_wq.

Here is one way rx_work can be running during the final cleanup:

nfc_llcp_data_received()
    nfc_llcp_find_local()             /* takes a reference */
                                      nfc_llcp_unregister_device()
                                          nfc_llcp_remove_local()
                                          local_cleanup()
                                          nfc_llcp_local_put()
    __nfc_llcp_recv()
        schedule_work(&local->rx_work)    /* system_wq */
    nfc_llcp_local_put()  /* last ref, queues release_work on llcp_wq */

After this, rx_work and release_work run at the same time on different
workqueues.

Digital drivers can reach this. nfc_digital_unregister_device() calls
nfc_unregister_device() before cancel_work_sync(&ddev->cmd_complete_work).
That lets
digital_tg_recv_dep_req()->nfc_tm_data_received()->nfc_llcp_data_received()
overlap with LLCP teardown. nfc_tm_data_received() checks dep_link_up but
not shutting_down.

There is a related path. nfc_llcp_tx_work() passes the raw local pointer
to nfc_data_exchange() as the context for nfc_llcp_recv(), and it takes no
reference:

	ret = nfc_data_exchange(local->dev, local->target_idx,
				skb, nfc_llcp_recv, local);

For digital devices, this completion runs from cmd_complete_work, which is
only cancelled after nfc_unregister_device(). A late completion goes
through nfc_llcp_recv()->__nfc_llcp_recv(), which writes local->rx_pending
and calls schedule_work(&local->rx_work). The local it touches may already
have been through local_cleanup(), or may already be freed.

Would disable_work_sync() work for the LLCP work items, or at least
disabling rx_work before tx_work? That would match what
timer_shutdown_sync() does for the timers.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023195409.3532540-1-tristmd%40gmail.com