[PATCH net] nfc: pn533: prevent division by zero in the listen mode timer

[Yinhao Hu]

The listen-mode timer handler advances the polling state machine through
pn533_poll_next_mod(), which computes:

dev->poll_mod_curr = (dev->poll_mod_curr + 1) % dev->poll_mod_count;

pn533_poll_reset_mod_list() clears dev->poll_mod_count without first
stopping that timer: pn533_dep_link_down() deletes no timer at all, and
pn533_stop_poll() uses timer_delete(), which does not wait for a handler
already running on another CPU. When the handler runs after the count
has been zeroed, it divides by zero:

Oops: divide error: 0000 [#1] SMP
RIP: 0010:pn533_listen_mode_timer+0x9b/0x110

Delete the timer synchronously in pn533_poll_reset_mod_list(), the single
place that clears the list, so the handler can no longer run past a reset.
Also return early when poll_mod_count is already zero, covering the window
where pn533_wq_poll() re-arms the timer just before a reset.

Fixes: 6fbbdc16be38 ("NFC: Implement pn533 polling loop")
Signed-off-by: Yinhao Hu <ddd...@hust.edu.cn>
---
drivers/nfc/pn533/pn533.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index d7bdbc82e2ba..88df99001b4a 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -951,6 +951,7 @@ static inline void pn533_poll_next_mod(struct pn533 *dev)

static void pn533_poll_reset_mod_list(struct pn533 *dev)
{
+ timer_delete_sync(&dev->listen_timer);
dev->poll_mod_count = 0;
}

@@ -1235,6 +1236,10 @@ static void pn533_listen_mode_timer(struct timer_list *t)
{
struct pn533 *dev = timer_container_of(dev, t, listen_timer);

+ /* Polling may have been stopped while the timer was pending. */
+ if (!dev->poll_mod_count)
+ return;
+
dev->cancel_listen = 1;

pn533_poll_next_mod(dev);
--
2.43.0

[Simon Horman]

I am concerned that access to poll_mod_count is not synchronised and thus
this may not work as intended.

[Yinhao Hu]

Thanks for the review.

The guard's read is lockless, but it isn't what makes the fix safe;
timer_delete_sync() is. pn533_poll_reset_mod_list() is the only place
that clears the count, and it now syncs the timer before the store, so
the handler can't be mid-pn533_poll_next_mod() when the count is zeroed.

The early return is just a backstop for a timer re-armed after that
reset, and it mirrors the existing lockless guard in
pn533_poll_complete() ahead of the other pn533_poll_next_mod() call, the
same pattern already in-tree.

If you'd still prefer the lockless accesses marked explicitly, I can add
READ_ONCE()/WRITE_ONCE() in v2.