[PATCH] nfc: pn533: add timeout to USB frame send completion wait

[PATCH] nfc: pn533: add timeout to USB frame send completion wait

[Zhao Feng]

pn533_usb_send_frame() waits indefinitely for the out URB to complete.
A malicious or malfunctioning device that never completes the transfer
blocks the caller in uninterruptible sleep forever -- e.g. during
device probe in the usb_hub_wq worker -- triggering hung task splats.

Bound the wait with a 1 second timeout.  On timeout, kill the URB and
fail with -ETIMEDOUT so that probing or the command in progress fails
gracefully instead of hanging the task.

Reported-by: syzbot+1c5368988052dc2d8201@syzkaller.appspotmail.com
Closes: https://syzbot.org/bug?extid=1c5368988052dc2d8201
Signed-off-by: Zhao Feng <zhaofeng2@cmss.chinamobile.com>
---
 drivers/nfc/pn533/usb.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index 0b7e3b118d..c2633b5da8 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -30,6 +30,9 @@
 #define ACS_VENDOR_ID 0x072f
 #define ACR122U_PRODUCT_ID 0x2200
 
+/* How long to wait for the out URB to complete */
+#define PN533_USB_SEND_TIMEOUT_MS	1000
+
 static const struct usb_device_id pn533_usb_table[] = {
 	{ USB_DEVICE(PN533_VENDOR_ID, PN533_PRODUCT_ID),
 	  .driver_info = PN533_DEVICE_STD },
@@ -184,7 +187,12 @@ static int pn533_usb_send_frame(struct pn533 *dev,
 	if (rc)
 		return rc;
 
-	wait_for_completion(&arg.done);
+	if (!wait_for_completion_timeout(&arg.done,
+					 msecs_to_jiffies(PN533_USB_SEND_TIMEOUT_MS))) {
+		usb_kill_urb(phy->out_urb);
+		phy->out_urb->context = cntx;
+		return -ETIMEDOUT;
+	}
 	phy->out_urb->context = cntx;
 
 	if (dev->protocol_type == PN533_PROTO_REQ_RESP) {
-- 
2.39.2

Re: [PATCH] nfc: pn533: add timeout to USB frame send completion wait

[netdev-bot+sashiko]

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

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

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 gives the device a new way to trigger a failure
  (-ETIMEDOUT from pn533_usb_send_frame() when OUT stalls for more than…
- [Medium] On timeout, pn533_usb_send_frame() calls
  usb_kill_urb(phy->out_urb) and returns -ETIMEDOUT unconditionally.
- [Low] Missing Fixes: tag (and Cc: stable).

--

nfc: pn533: add timeout to USB frame send completion wait

This patch replaces the unbounded wait for the out URB in
pn533_usb_send_frame() with a 1 second timeout. On timeout the URB is
killed and -ETIMEDOUT is returned, so a device that never completes the
transfer no longer hangs probe or the command in progress.

Closes: https://syzbot.org/bug?extid=1c5368988052dc2d8201

> Reported-by: syzbot+1c5368988052dc2d8201@syzkaller.appspotmail.com
> Closes: https://syzbot.org/bug?extid=1c5368988052dc2d8201
> Signed-off-by: Zhao Feng <zhaofeng2@cmss.chinamobile.com>

[Severity: Low]
Should this have a Fixes: tag and Cc: stable? The unbounded
wait_for_completion(&arg.done) and the context save/restore around it
came from:

Fixes: 9dab880d675b ("nfc: pn533: Wait for out_urb's completion in pn533_usb_send_frame()")

> diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
> index efb07f944fce2..60e80e818d2e6 100644
> --- a/drivers/nfc/pn533/usb.c
> +++ b/drivers/nfc/pn533/usb.c

[ ... ]

> @@ -184,7 +187,12 @@ static int pn533_usb_send_frame(struct pn533 *dev,
>  	if (rc)
>  		return rc;
>  
> -	wait_for_completion(&arg.done);
> +	if (!wait_for_completion_timeout(&arg.done,
> +					 msecs_to_jiffies(PN533_USB_SEND_TIMEOUT_MS))) {
> +		usb_kill_urb(phy->out_urb);
> +		phy->out_urb->context = cntx;
> +		return -ETIMEDOUT;
> +	}
>  	phy->out_urb->context = cntx;
>  
>  	if (dev->protocol_type == PN533_PROTO_REQ_RESP) {

[Severity: Medium]
Is it safe to return -ETIMEDOUT here without checking the final status
of out_urb?

The OUT transfer can complete between wait_for_completion_timeout()
returning 0 and the call to usb_kill_urb(). In that case usb_kill_urb()
returns at once because pn533_out_complete() has already run. The device
has the whole command, but the caller is told the send failed.
usb_start_wait_urb() looks at the URB status after killing it on timeout
for this reason.

A PN533 frame can also span several 64-byte bulk packets. The device may
accept part of the frame before it stalls, which leaves a partial frame
in its parser.

In both cases the early return skips pn533_submit_urb_for_ack() and
pn533_submit_urb_for_response(), so the device's ACK or response is
never read. __pn533_send_async() then does:

	rc = pn533_send_cmd_frame(dev, cmd);
	if (rc) {
		dev->cmd = NULL;
		goto error;
	}

This leaves cmd_pending at 0, so the next command is sent right away.
Its pn533_recv_ack() would see the stale ACK, and its in_urb would get
the abandoned command's response. pn533_recv_frame() only compares the
command code:

	} else if (!pn533_rx_frame_is_cmd_response(dev, skb->data)) {
		nfc_err(dev->dev, "It it not the response to the last command\n");
		dev->cmd->status = -EIO;
	}

When two commands in a row share a code, such as repeated InDataExchange
from pn533_transceive(), could the stale response be accepted as the
reply to the new request? The driver would then stay one response
behind. Commands with different codes would fail with -EIO instead.

The driver already has a protocol-level abort, pn533_usb_send_ack(),
which pn533_usb_abort_cmd() uses. The comment in pn533_usb_abort_cmd()
also says ACR122U sends broken responses when URBs are cancelled before
the chip responds.

Should the timeout path send the ACK abort or reset the device so that
the two sides stay in step?

[Severity: High]
Does the commit message claim that "the command in progress fails
gracefully instead of hanging the task" hold for queued commands?

When dev->cmd_pending is set, __pn533_send_async() adds the command to
dev->cmd_queue and returns 0. pn533_send_cmd_sync() then sleeps in:

	wait_for_completion(&arg.done);

Later pn533_wq_cmd() takes the command off the queue and sends it. If
pn533_usb_send_frame() now returns -ETIMEDOUT, the error path in
pn533_wq_cmd() is:

	rc = pn533_send_cmd_frame(dev, cmd);
	if (rc < 0) {
		dev->cmd = NULL;
		dev_kfree_skb(cmd->req);
		kfree(cmd);
		return;
	}

This path never calls cmd->complete_cb, never clears cmd_pending and
never re-queues cmd_work.

Could the pn533_send_cmd_sync() caller then sleep in
TASK_UNINTERRUPTIBLE forever? That is the same hung task, just moved to
the nfc netlink caller.

One way to reach it:

nfc_dev_down()
  pn533_stop_poll()   (abort_cmd is a no-op on ACR122U, poll stays pending)
  pn533_dev_down()
    pn533_rf_field()
      pn533_set_configuration()
        pn533_send_cmd_sync()   (queued behind the pending command)

Async callers are affected too:

- The pn533_data_exchange_arg allocated by pn533_transceive() would
  leak, and its data_exchange callback would never run. For rawsock this
  also leaks the sock_hold() reference.
- With cmd_pending stuck at 1, every later command is queued and never
  sent until unbind.
- pn53x_common_clean() then frees the queued cmds without freeing their
  cmd->req skbs.

The pn533_wq_cmd() error handling is older than this patch, but this
patch lets the device trigger it. Should pn533_wq_cmd() call
cmd->complete_cb with ERR_PTR(rc) and keep the queue moving as part of
this fix?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023025602.1746-1-zhaofeng2%40cmss.chinamobile.com

Re: [PATCH] nfc: pn533: add timeout to USB frame send completion wait

[Zhao Feng]

Thanks for the detailed review.  All three issues are legitimate, and
I'm respinning as v2.

[High] Confirmed against the code: pn533_wq_cmd() drops a queued
command on send failure without calling complete_cb, without clearing
cmd_pending and without rescheduling cmd_work, so a
pn533_send_cmd_sync() caller would indeed sleep forever and the queue
freezes.  v2 patch 1/2 now completes the failed command with the error
and reschedules cmd_work, so the queue drains with errors instead of
freezing.

[Medium] v2 patch 2/2 mirrors usb_start_wait_urb(): after
usb_kill_urb() it checks urb->status and proceeds normally if the
transfer completed between the timeout expiring and the kill, instead
of reporting a spurious -ETIMEDOUT.

Regarding a protocol-level ACK abort on timeout: pn533_usb_send_ack()
is a no-op on ACR122U, and after a genuine stall we cannot know how
much of the frame the device consumed, so I kept the timeout path
minimal.  The stale-response desync you describe remains possible
after a real timeout; a full device reset looks too invasive for this
fix, but I'm happy to revisit it if the maintainers prefer.

[Low] Fixes tag added with the commit you identified, and Cc: stable
on both patches.

pw-bot: cr