[PATCH net] nfc: pn533: Wait for out_urb's completion in pn533_usb_send_frame()

[PATCH net] nfc: pn533: Wait for out_urb's completion in pn533_usb_send_frame()

[Minsuk Kang]

Fix a use-after-free that occurs in hcd when in_urb sent from
pn533_usb_send_frame() is completed earlier than out_urb. Its callback
frees the skb data in pn533_send_async_complete() that is used as a
transfer buffer of out_urb. Wait before sending in_urb until the
callback of out_urb is called. To modify the callback of out_urb alone,
separate the complete function of out_urb and ack_urb.

Found by a modified version of syzkaller.

BUG: KASAN: use-after-free in dummy_timer
Call Trace:
 memcpy
 dummy_timer
 call_timer_fn
 run_timer_softirq
 __do_softirq
 irq_exit_rcu
 sysvec_apic_timer_interrupt

Fixes: c46ee38620a2 ("NFC: pn533: add NXP pn533 nfc device driver")
Signed-off-by: Minsuk Kang <linuxlovemin@yonsei.ac.kr>
---
 drivers/nfc/pn533/usb.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index 6f71ac72012e..325818fbaf3b 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -48,6 +48,8 @@ struct pn533_usb_phy {
 	struct usb_interface *interface;
 
 	struct urb *out_urb;
+	struct completion *out_done;
+
 	struct urb *in_urb;
 
 	struct urb *ack_urb;
@@ -157,6 +159,7 @@ static int pn533_usb_send_frame(struct pn533 *dev,
 				struct sk_buff *out)
 {
 	struct pn533_usb_phy *phy = dev->phy;
+	struct completion out_done;
 	int rc;
 
 	if (phy->priv == NULL)
@@ -168,10 +171,15 @@ static int pn533_usb_send_frame(struct pn533 *dev,
 	print_hex_dump_debug("PN533 TX: ", DUMP_PREFIX_NONE, 16, 1,
 			     out->data, out->len, false);
 
+	phy->out_done = &out_done;
+	init_completion(&out_done);
+
 	rc = usb_submit_urb(phy->out_urb, GFP_KERNEL);
 	if (rc)
 		return rc;
 
+	wait_for_completion(&out_done);
+
 	if (dev->protocol_type == PN533_PROTO_REQ_RESP) {
 		/* request for response for sent packet directly */
 		rc = pn533_submit_urb_for_response(phy, GFP_KERNEL);
@@ -408,7 +416,30 @@ static int pn533_acr122_poweron_rdr(struct pn533_usb_phy *phy)
 	return arg.rc;
 }
 
-static void pn533_send_complete(struct urb *urb)
+static void pn533_out_complete(struct urb *urb)
+{
+	struct pn533_usb_phy *phy = urb->context;
+
+	complete(phy->out_done);
+
+	switch (urb->status) {
+	case 0:
+		break; /* success */
+	case -ECONNRESET:
+	case -ENOENT:
+		dev_dbg(&phy->udev->dev,
+			"The urb has been stopped (status %d)\n",
+			urb->status);
+		break;
+	case -ESHUTDOWN:
+	default:
+		nfc_err(&phy->udev->dev,
+			"Urb failure (status %d)\n",
+			urb->status);
+	}
+}
+
+static void pn533_ack_complete(struct urb *urb)
 {
 	struct pn533_usb_phy *phy = urb->context;
 
@@ -496,10 +527,10 @@ static int pn533_usb_probe(struct usb_interface *interface,
 
 	usb_fill_bulk_urb(phy->out_urb, phy->udev,
 			  usb_sndbulkpipe(phy->udev, out_endpoint),
-			  NULL, 0, pn533_send_complete, phy);
+			  NULL, 0, pn533_out_complete, phy);
 	usb_fill_bulk_urb(phy->ack_urb, phy->udev,
 			  usb_sndbulkpipe(phy->udev, out_endpoint),
-			  NULL, 0, pn533_send_complete, phy);
+			  NULL, 0, pn533_ack_complete, phy);
 
 	switch (id->driver_info) {
 	case PN533_DEVICE_STD:
-- 
2.25.1

Re: [PATCH net] nfc: pn533: Wait for out_urb's completion in pn533_usb_send_frame()

[Jakub Kicinski]

On Wed,  4 Jan 2023 21:17:11 +0900 Minsuk Kang wrote:
> Fix a use-after-free that occurs in hcd when in_urb sent from
> pn533_usb_send_frame() is completed earlier than out_urb. Its callback
> frees the skb data in pn533_send_async_complete() that is used as a
> transfer buffer of out_urb. Wait before sending in_urb until the
> callback of out_urb is called. To modify the callback of out_urb alone,
> separate the complete function of out_urb and ack_urb.
> 
> Found by a modified version of syzkaller.
> 
> BUG: KASAN: use-after-free in dummy_timer
> Call Trace:
>  memcpy
>  dummy_timer
>  call_timer_fn
>  run_timer_softirq
>  __do_softirq
>  irq_exit_rcu
>  sysvec_apic_timer_interrupt

Could you add fine names and line numbers to the stack trace
(use scripts/decode_stacktrace.sh)?  dummy_timer is a very generic
name, it'd be useful to tie it to the gadget device via file name.

> Fixes: c46ee38620a2 ("NFC: pn533: add NXP pn533 nfc device driver")
> Signed-off-by: Minsuk Kang <linuxlovemin@yonsei.ac.kr>
> ---
>  drivers/nfc/pn533/usb.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
> index 6f71ac72012e..325818fbaf3b 100644
> --- a/drivers/nfc/pn533/usb.c
> +++ b/drivers/nfc/pn533/usb.c
> @@ -48,6 +48,8 @@ struct pn533_usb_phy {
>  	struct usb_interface *interface;
>  
>  	struct urb *out_urb;
> +	struct completion *out_done;

Why keep this pointer for the lifetime of the device instead of putting
it on the stack? For inspiration please take a look at 
struct pn533_acr122_poweron_rdr_arg