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

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

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

local_cleanup() uses timer_delete_sync() on link_timer and
sdreq_timer, followed by cancel_work_sync() on their associated
work items.  However, tx_work unconditionally re-arms link_timer
via mod_timer() when it runs, and sdreq_timeout_work conditionally
re-arms sdreq_timer.  If a work callback executes during
cancel_work_sync and re-arms the timer, the timer fires after
kfree(local), producing a crash via schedule_work on freed memory.

Use timer_shutdown_sync() instead, which prevents subsequent
mod_timer() calls from re-arming the timers.

Fixes: d646960f7986 ("NFC: Initial LLCP support")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 net/nfc/llcp_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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);
 	cancel_work_sync(&local->timeout_work);
 	kfree_skb(local->rx_pending);
 	local->rx_pending = NULL;
-	timer_delete_sync(&local->sdreq_timer);
+	timer_shutdown_sync(&local->sdreq_timer);
 	cancel_work_sync(&local->sdreq_timeout_work);
 	nfc_llcp_free_sdp_tlv_list(&local->pending_sdreqs);
 }
-- 
2.47.3

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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