[PATCH] nfc: pn533: bound the OUT URB wait in pn533_usb_send_frame()

[PATCH] nfc: pn533: bound the OUT URB wait in pn533_usb_send_frame()

[Nguyen Ngoc Thang]

pn533_usb_send_frame() submits phy->out_urb and then calls
wait_for_completion() with no timeout. During probe this is reached
via pn533_finalize_setup() -> pn533_get_firmware_version(), with the
USB device lock held by hub_event(). If the device never completes the
bulk OUT transfer, the wait never returns: the hub kworker hangs inside
probe() and every other task needing that device lock (e.g.
usbdev_open()) hangs behind it.

Bound the wait with wait_for_completion_timeout(), using the same 5000ms
that usb_bulk_msg() gets in pn533_acr122_poweron_rdr(). On timeout, kill
the URB (usb_kill_urb() waits for the handler, so the on-stack
completion can't be touched after return) and fail with -ETIMEDOUT,
which __pn533_send_async() already unwinds.

Fixes: 9dab880d675b ("nfc: pn533: Wait for out_urb's completion in pn533_usb_send_frame()")
Reported-by: syzbot+1c5368988052dc2d8201@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1c5368988052dc2d8201
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
 drivers/nfc/pn533/usb.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index efb07f944fce..569b1915afc9 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -184,7 +184,12 @@ static int pn533_usb_send_frame(struct pn533 *dev,
 	if (rc)
 		return rc;
 
-	wait_for_completion(&arg.done);
+	/* device may never ack the OUT transfer: don't hang probe() */
+	if (!wait_for_completion_timeout(&arg.done, msecs_to_jiffies(5000))) {
+		usb_kill_urb(phy->out_urb); /* waits for the handler, so &arg can't outlive us */
+		phy->out_urb->context = cntx;
+		return -ETIMEDOUT;
+	}
 	phy->out_urb->context = cntx;
 
 	if (dev->protocol_type == PN533_PROTO_REQ_RESP) {
-- 
2.43.0

Re: [PATCH] nfc: pn533: bound the OUT URB wait in pn533_usb_send_frame()

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 1 · 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 patch only bounds the bulk OUT stage, so probe() can still
  hang forever under the USB device lock.
- [High] The new -ETIMEDOUT return from pn533_usb_send_frame()
  (usb.c:188-191) is not unwound correctly when the command was queued.
- [Medium] The timeout path in pn533_usb_send_frame() (usb.c:188-191)
  returns -ETIMEDOUT without checking what actually happened to the OUT…

--

nfc: pn533: bound the OUT URB wait in pn533_usb_send_frame()

This patch replaces the unbounded wait_for_completion() on the bulk OUT
URB in pn533_usb_send_frame() with a 5000ms wait_for_completion_timeout().
When the timeout expires, it kills the URB and returns -ETIMEDOUT. The
goal is to stop probe() from hanging with the USB device lock held when
the device never completes the OUT transfer.

Closes: https://syzkaller.appspot.com/bug?extid=1c5368988052dc2d8201

> diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
> index efb07f944fce..569b1915afc9 100644
> --- a/drivers/nfc/pn533/usb.c
> +++ b/drivers/nfc/pn533/usb.c
> @@ -184,7 +184,12 @@ static int pn533_usb_send_frame(struct pn533 *dev,
>  	if (rc)
>  		return rc;
>  
> -	wait_for_completion(&arg.done);
> +	/* device may never ack the OUT transfer: don't hang probe() */
> +	if (!wait_for_completion_timeout(&arg.done, msecs_to_jiffies(5000))) {
> +		usb_kill_urb(phy->out_urb); /* waits for the handler, so &arg can't outlive us */

[Severity: Medium]
Can this report a failure for a transfer that actually succeeded? The HCD
can give back out_urb with status 0 after wait_for_completion_timeout()
returns 0 but before usb_kill_urb() runs. In that case usb_kill_urb() does
nothing, and the function still returns -ETIMEDOUT.

__pn533_send_async() or pn533_wq_cmd() then frees the cmd and clears
dev->cmd, and no in_urb gets submitted. The chip still sends an ACK and a
response for the command it received. Those would then be read as the
reply to the next command. That command would fail with -EIO, or get
stale data if its command code is the same.

The commit message cites usb_bulk_msg() as precedent. Its helper,
usb_start_wait_urb(), checks the real completion status after the kill:

drivers/usb/core/message.c:usb_start_wait_urb() {
    ...
	if (rc <= 0) {
		usb_kill_urb(urb);
		if (ctx.status != -ENOENT)
			retval = ctx.status;
		else if (rc == 0)
			retval = -ETIMEDOUT;
    ...
}

Could the same be done here? For example, pn533_out_complete() could
record the URB status in struct pn533_out_arg.

The kill could also land after some max-packet chunks of a multi-packet
frame have already been accepted. That would leave the chip holding a
partial frame, or an ACR122 CCID message shorter than dwLength. Should the
timeout path resync the chip? pn533_usb_abort_cmd() and
pn533_usb_send_ack() already send an ACK abort frame for non-ACR122
devices.

> +		phy->out_urb->context = cntx;
> +		return -ETIMEDOUT;

[Severity: High]
The commit message says -ETIMEDOUT is something "__pn533_send_async()
already unwinds". Is that accurate for every caller? It holds for the
direct send path. A queued command, though, is sent from pn533_wq_cmd(),
and its error branch looks like this:

drivers/nfc/pn533/pn533.c:pn533_wq_cmd() {
    ...
	rc = pn533_send_cmd_frame(dev, cmd);
	if (rc < 0) {
		dev->cmd = NULL;
		dev_kfree_skb(cmd->req);
		kfree(cmd);
		return;
	}
}

That branch never calls cmd->complete_cb and never clears
dev->cmd_pending.

Queuing can happen during probe. pn533_wq_cmd_complete() wakes the sync
waiter before cmd_work runs:

	rc = pn533_send_async_complete(dev);
	if (rc != -EINPROGRESS)
		queue_work(dev->wq, &dev->cmd_work);

dev->cmd_pending is only cleared in pn533_wq_cmd(), and only when the
queue is empty. So the probe thread can issue its next sync command from
pn533_setup() (for example via pn533_set_configuration()) while
cmd_pending is still 1. __pn533_send_async() then queues that command
instead of sending it.

If that command's OUT URB then hits this new timeout, can the probe
thread get stuck forever in wait_for_completion(&arg.done) inside
pn533_send_cmd_sync()?

Outside of probe, would async callers leak their complete_cb_context?
And would cmd_pending stay at 1, so every later command is queued and
never sent?

Before this patch, the pn533_wq_cmd() error branch was only reachable
when usb_submit_urb() failed.

> +	}
>  	phy->out_urb->context = cntx;
>  
>  	if (dev->protocol_type == PN533_PROTO_REQ_RESP) {

[Severity: High]
Does this fully stop probe() from hanging? pn533_out_complete() calls
complete() for any URB status. Once out_urb completes, nothing here checks
out_urb->status. The function submits in_urb for the ACK or the response
and returns 0.

The probe thread then waits here:

drivers/nfc/pn533/pn533.c:pn533_send_cmd_sync() {
    ...
	rc = pn533_send_cmd_async(dev, cmd_code, req,
				  pn533_send_sync_complete, &arg);
    ...
	wait_for_completion(&arg.done);
    ...
}

That completion is only signalled from cmd_complete_work. cmd_complete_work
is only queued when in_urb completes, via pn533_recv_ack() or
pn533_recv_frame(). The USB transport has no command timeout, and
abort_cmd is only called from the poll and listen paths.

So a device that accepts (or stalls) the bulk OUT transfer but never sends
anything on bulk IN could still hang this chain:

pn533_usb_probe()
  pn533_finalize_setup()
    pn533_get_firmware_version()
      pn533_send_cmd_sync()
        wait_for_completion(&arg.done)

That would happen inside hub_event() with the USB device lock held. Isn't
that the same hang the commit message describes? Would the wait need to be
bounded, or the command aborted, at the pn533_send_cmd_sync() level
instead?

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