[PATCH v4 0/2] nfc: nci: uart: fix write_work teardown UAF (+ nfcmrvl drv_data)

[PATCH v4 0/2] nfc: nci: uart: fix write_work teardown UAF (+ nfcmrvl drv_data)

[Shubham Antil]

This series fixes a use-after-free in the NFC NCI UART line-discipline
teardown and a NULL dereference in the Marvell NFC UART driver that the
same change surfaces.

nci_uart_tty_close() frees the tx skbs and purges the tx queue before it
cancels the write worker, while the NCI device is still registered.  The
worker can therefore be re-queued -- by a tty hangup, or by the NCI core
still sending through the driver -- and run against freed memory.  The
fix follows the Bluetooth hci_uart ldisc: gate the worker on a readiness
bit under a percpu_rwsem and drain it under the write lock on close.

Enabling the worker that way means it must be enabled before the device
is registered, since the Marvell driver may transmit (firmware download)
from within registration.  That exposes a pre-existing NULL dereference:
nfcmrvl publishes nu->drv_data only after nfcmrvl_nci_register_dev()
returns, so a transmit during registration reaches
nfcmrvl_nci_uart_tx_start() with a NULL nu->drv_data.

 1/2 nfcmrvl: publish nu->drv_data before nci_register_device(), so a
     transmit during registration does not hit a NULL nu->drv_data.
 2/2 nci: uart: the teardown use-after-free fix; NCI_UART_READY and the
     module reference are taken before ops.open(), with shared error
     labels and the tx_wakeup trylock comment aligned with hci_uart.

Both are runtime-tested together under KASAN: the unfixed tree crashes
within the first iterations (slab-use-after-free in nci_uart_write_work),
the series survives 56000+ register/hangup iterations cleanly.

Shubham Antil (2):
  nfc: nfcmrvl: set drv_data before registering the nci device
  nfc: nci: uart: fix use-after-free of write_work on ldisc teardown

 drivers/nfc/nfcmrvl/main.c | 17 ++++++++
 drivers/nfc/nfcmrvl/uart.c |  3 --
 include/net/nfc/nci_core.h |  2 +
 net/nfc/nci/uart.c         | 80 +++++++++++++++++++++++++++++++-------
 4 files changed, 85 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] nfc: nfcmrvl: set drv_data before registering the nci device

[Shubham Antil]

nfcmrvl_nci_uart_open() stores the per-ldisc back-pointer nu->drv_data
only after nfcmrvl_nci_register_dev() has returned.  That function
registers the nci device, and a transmit started during registration
reaches the UART write path

  nci_uart_write_work() -> nu->ops.tx_start()
  == nfcmrvl_nci_uart_tx_start()

which dereferences nu->drv_data:

  struct nfcmrvl_private *priv = (struct nfcmrvl_private *)nu->drv_data;

  if (priv->ndev->nfc_dev->fw_download_in_progress)

At that point nu->drv_data is still NULL:

  BUG: KASAN: null-ptr-deref in nfcmrvl_nci_uart_tx_start+0x2c/0xc0
  Workqueue: events nci_uart_write_work
  Call Trace:
   nfcmrvl_nci_uart_tx_start
   nci_uart_write_work
   worker_thread

Publish nu->drv_data (and nu->ndev) for the UART phy before
nci_register_device() is called, and clear them again if registration
fails, so that a transmit triggered during registration finds a valid
pointer.  The now-redundant assignment in nfcmrvl_nci_uart_open() is
removed.

Fixes: e097dc624f78 ("NFC: nfcmrvl: add UART driver")
Assisted-by: LLM Claude
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 drivers/nfc/nfcmrvl/main.c | 17 +++++++++++++++++
 drivers/nfc/nfcmrvl/uart.c |  3 ---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 6efa83219..8186e86d7 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -96,6 +96,7 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 				const struct nfcmrvl_platform_data *pdata)
 {
 	struct nfcmrvl_private *priv;
+	struct nci_uart *nu = NULL;
 	int rc;
 	int headroom;
 	int tailroom;
@@ -110,6 +111,9 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 	priv->dev = dev;
 	priv->phy = phy;
 
+	if (phy == NFCMRVL_PHY_UART)
+		nu = drv_data;
+
 	memcpy(&priv->config, pdata, sizeof(*pdata));
 
 	if (!priv->config.reset_gpio) {
@@ -154,9 +158,22 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 
 	nci_set_drvdata(priv->ndev, priv);
 
+	/* For the UART phy the transmit path reaches the driver through
+	 * nu->drv_data; publish it before nci_register_device() so a transmit
+	 * triggered during registration does not dereference a NULL pointer.
+	 */
+	if (nu) {
+		nu->drv_data = priv;
+		nu->ndev = priv->ndev;
+	}
+
 	rc = nci_register_device(priv->ndev);
 	if (rc) {
 		nfc_err(dev, "nci_register_device failed %d\n", rc);
+		if (nu) {
+			nu->drv_data = NULL;
+			nu->ndev = NULL;
+		}
 		goto error_fw_dnld_deinit;
 	}
 
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 9aedd1687..421e6dc99 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -138,9 +138,6 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
 
 	priv->support_fw_dnld = true;
 
-	nu->drv_data = priv;
-	nu->ndev = priv->ndev;
-
 	return 0;
 }
 
-- 
2.43.0

[PATCH v4 2/2] nfc: nci: uart: fix use-after-free of write_work on ldisc teardown

[Shubham Antil]

nci_uart_tty_close() frees nu->tx_skb and purges nu->tx_q before it
cancels nu->write_work, and the NCI device is still registered at that
point.  Two paths can therefore (re)queue the write worker after the
skbs are freed and run it against freed memory:

 - a tty hangup invokes the ldisc ->write_wakeup() (nci_uart_tty_wakeup
   -> nci_uart_tx_wakeup -> schedule_work), and
 - the NCI core keeps sending via the driver (nci_uart_send() ->
   nci_uart_tx_wakeup -> schedule_work) until the device is unregistered
   by nu->ops.close().

nci_uart_write_work() then dereferences the freed nu->tx_skb -- a
use-after-free.  nu->ops.close() also frees driver state that the worker
dereferences via nu->ops.tx_start()/tx_done(), so the worker has to be
stopped before ops.close() runs.

Fix this the way the Bluetooth hci_uart ldisc does: gate
nci_uart_tx_wakeup() on a new NCI_UART_READY bit taken under a
per-connection rwsem.  nci_uart_tty_close() clears NCI_UART_READY under
the write lock -- draining any in-flight nci_uart_tx_wakeup() -- so that
neither the tty nor the internal send path can requeue write_work once it
is cancelled; only then is the device closed and the skbs freed.

NCI_UART_READY is set, and the module reference taken, before
nu->ops.open() registers the device: the driver may transmit (e.g.
download firmware) from within registration and user space can use the
interface as soon as it is registered, so gating those transmits off
would drop or leak them.  The open path uses shared error labels and, on
a failed open, drains the worker the same way the close path does.

Runtime-tested under KASAN with a line-discipline hangup reproducer: the
unfixed ldisc reports

  BUG: KASAN: slab-use-after-free in nci_uart_write_work

within the first iterations, while the fixed ldisc runs the same race for
56000+ register/hangup cycles without any KASAN report.

Fixes: 9961127d4bce ("NFC: nci: add generic uart support")
Assisted-by: LLM Claude
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 include/net/nfc/nci_core.h |  2 +
 net/nfc/nci/uart.c         | 80 +++++++++++++++++++++++++++++++-------
 2 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/include/net/nfc/nci_core.h b/include/net/nfc/nci_core.h
index 664d5058e..c71648ab5 100644
--- a/include/net/nfc/nci_core.h
+++ b/include/net/nfc/nci_core.h
@@ -18,6 +18,7 @@
 #define __NCI_CORE_H
 
 #include <linux/interrupt.h>
+#include <linux/percpu-rwsem.h>
 #include <linux/skbuff.h>
 #include <linux/tty.h>
 
@@ -455,6 +456,7 @@ struct nci_uart {
 	struct work_struct	write_work;
 	struct tty_struct	*tty;
 	unsigned long		tx_state;
+	struct percpu_rw_semaphore tx_lock;
 	struct sk_buff_head	tx_q;
 	struct sk_buff		*tx_skb;
 	struct sk_buff		*rx_skb;
diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
index aa20e8603..a971fb3b1 100644
--- a/net/nfc/nci/uart.c
+++ b/net/nfc/nci/uart.c
@@ -33,6 +33,7 @@
 /* TX states  */
 #define NCI_UART_SENDING	1
 #define NCI_UART_TX_WAKEUP	2
+#define NCI_UART_READY		3
 
 static struct nci_uart *nci_uart_drivers[NCI_UART_DRIVER_MAX];
 
@@ -58,13 +59,28 @@ static inline int nci_uart_queue_empty(struct nci_uart *nu)
 
 static int nci_uart_tx_wakeup(struct nci_uart *nu)
 {
+	/* This may be called in an IRQ context, so we can't sleep.  Therefore
+	 * we try to acquire the read lock only, and if that fails we assume
+	 * the tty is being closed, because that is the only time the write
+	 * lock is taken (nci_uart_tty_close()).  If the write lock is ever
+	 * taken elsewhere, this must be revisited.
+	 */
+	if (!percpu_down_read_trylock(&nu->tx_lock))
+		return 0;
+
+	if (!test_bit(NCI_UART_READY, &nu->tx_state))
+		goto out;
+
 	if (test_and_set_bit(NCI_UART_SENDING, &nu->tx_state)) {
 		set_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
-		return 0;
+		goto out;
 	}
 
 	schedule_work(&nu->write_work);
 
+out:
+	percpu_up_read(&nu->tx_lock);
+
 	return 0;
 }
 
@@ -123,18 +139,46 @@ static int nci_uart_set_driver(struct tty_struct *tty, unsigned int driver)
 	INIT_WORK(&nu->write_work, nci_uart_write_work);
 	spin_lock_init(&nu->rx_lock);
 
-	ret = nu->ops.open(nu);
-	if (ret) {
-		kfree(nu);
-		return ret;
-	} else if (!try_module_get(nu->owner)) {
-		nu->ops.close(nu);
-		kfree(nu);
-		return -ENOENT;
+	ret = percpu_init_rwsem(&nu->tx_lock);
+	if (ret)
+		goto err_free;
+
+	/* Take the module reference and enable the write worker before the
+	 * device is registered: ops.open() may already transmit (e.g. download
+	 * firmware), and user space can use the interface as soon as it is
+	 * registered.
+	 */
+	if (!try_module_get(nu->owner)) {
+		ret = -ENOENT;
+		goto err_rwsem;
 	}
+
+	set_bit(NCI_UART_READY, &nu->tx_state);
+
+	ret = nu->ops.open(nu);
+	if (ret)
+		goto err_ready;
+
 	tty->disc_data = nu;
 
 	return 0;
+
+err_ready:
+	/* ops.open() may already have scheduled write_work; stop it before
+	 * freeing, the same way nci_uart_tty_close() does.
+	 */
+	percpu_down_write(&nu->tx_lock);
+	clear_bit(NCI_UART_READY, &nu->tx_state);
+	percpu_up_write(&nu->tx_lock);
+	cancel_work_sync(&nu->write_work);
+	kfree_skb(nu->tx_skb);
+	skb_queue_purge(&nu->tx_q);
+	module_put(nu->owner);
+err_rwsem:
+	percpu_free_rwsem(&nu->tx_lock);
+err_free:
+	kfree(nu);
+	return ret;
 }
 
 /* ------ LDISC part ------ */
@@ -180,16 +224,24 @@ static void nci_uart_tty_close(struct tty_struct *tty)
 	if (!nu)
 		return;
 
-	kfree_skb(nu->tx_skb);
-	kfree_skb(nu->rx_skb);
+	/* Drain in-flight tx_wakeups and block new ones, so write_work cannot
+	 * be requeued once it is cancelled below.
+	 */
+	percpu_down_write(&nu->tx_lock);
+	clear_bit(NCI_UART_READY, &nu->tx_state);
+	percpu_up_write(&nu->tx_lock);
 
-	skb_queue_purge(&nu->tx_q);
+	cancel_work_sync(&nu->write_work);
 
 	nu->ops.close(nu);
 	nu->tty = NULL;
-	module_put(nu->owner);
 
-	cancel_work_sync(&nu->write_work);
+	kfree_skb(nu->tx_skb);
+	kfree_skb(nu->rx_skb);
+	skb_queue_purge(&nu->tx_q);
+
+	module_put(nu->owner);
+	percpu_free_rwsem(&nu->tx_lock);
 
 	kfree(nu);
 }
-- 
2.43.0

Re: [PATCH v4 2/2] nfc: nci: uart: fix use-after-free of write_work on ldisc teardown

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 (2) · Low: 2 (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).

- [Medium] Regression: CORE_RESET sent while the device is torn down
  never reaches the NFCC.
- [Low] The commit message describes the tty-hangup race wrongly.
- [Low] The new comment in nci_uart_tx_wakeup() is wrong as soon as the
  patch lands.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but two
  NCIUARTSETDRIVER ioctls can race on one tty (TOCTOU), orphaning an…
- [Medium] This problem wasn't introduced by this patch, but the TX state
  machine can lose a wakeup.
- [Medium] This problem wasn't introduced by this patch, but the driver-
  template registry has a lifetime race.
- [Low] This problem wasn't introduced by this patch, but tty->disc_data
  is published only after nu->ops.open() has registered the NCI device…

--

nfc: nci: uart: fix use-after-free of write_work on ldisc teardown

This patch gates nci_uart_tx_wakeup() on a new NCI_UART_READY bit taken
under a percpu rwsem. It reorders nci_uart_tty_close() so write_work is
cancelled before ops.close() runs and before the tx skbs are freed. It
also takes the module reference and sets READY before ops.open() in
nci_uart_set_driver(), and adds shared error labels to that path.

> Two paths can therefore (re)queue the write worker after the
> skbs are freed and run it against freed memory:
>
>  - a tty hangup invokes the ldisc ->write_wakeup() (nci_uart_tty_wakeup
>    -> nci_uart_tx_wakeup -> schedule_work), and

[Severity: Low]
Is this description of the hangup path accurate?

In tty_ldisc_hangup(), ->write_wakeup() is called under a tty_ldisc_ref()
reference. That reference is dropped before tty_ldisc_lock() is taken and
before ->close() runs:

drivers/tty/tty_ldisc.c:tty_ldisc_hangup() {
    ld = tty_ldisc_ref(tty);
    ...
        ld->ops->write_wakeup(tty);
    ...
    tty_ldisc_deref(ld);
    ...
    tty_ldisc_lock(tty, MAX_SCHEDULE_TIMEOUT);
}

Other wakeups go through tty_wakeup()->tty_ldisc_ref(). That call fails
while the ldisc semaphore is write-held during close. So it looks like no
tty-originated wakeup can run inside nci_uart_tty_close().

The hangup use-after-free seems to have a different cause. Work queued
just before close is still pending or running when the old code frees
nu->tx_skb and calls nu->ops.close().

For that path, moving cancel_work_sync() ahead of the frees is what closes
the window. The NCI_UART_READY gate only matters for the internal
nci_uart_send() path.

The changelog later says:

> neither the tty nor the internal send path can requeue write_work once it
> is cancelled

Could the changelog describe which reordering closes which window?

> diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
> index aa20e8603f327..a971fb3b1bbb6 100644
> --- a/net/nfc/nci/uart.c
> +++ b/net/nfc/nci/uart.c

[ ... ]

> @@ -58,13 +59,28 @@ static inline int nci_uart_queue_empty(struct nci_uart *nu)
>  
>  static int nci_uart_tx_wakeup(struct nci_uart *nu)
>  {
> +	/* This may be called in an IRQ context, so we can't sleep.  Therefore
> +	 * we try to acquire the read lock only, and if that fails we assume
> +	 * the tty is being closed, because that is the only time the write
> +	 * lock is taken (nci_uart_tty_close()).  If the write lock is ever
> +	 * taken elsewhere, this must be revisited.
> +	 */

[Severity: Low]
This isn't a bug, but is this comment accurate? The same patch also takes
the write lock in the err_ready path of nci_uart_set_driver():

err_ready:
	...
	percpu_down_write(&nu->tx_lock);
	clear_bit(NCI_UART_READY, &nu->tx_state);
	percpu_up_write(&nu->tx_lock);

Dropping the wakeup there is harmless because that path is also teardown.
Still, the comment says nci_uart_tty_close() is the only place the write
lock is taken.

> +	if (!percpu_down_read_trylock(&nu->tx_lock))
> +		return 0;
> +
> +	if (!test_bit(NCI_UART_READY, &nu->tx_state))
> +		goto out;
> +
>  	if (test_and_set_bit(NCI_UART_SENDING, &nu->tx_state)) {
>  		set_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
> -		return 0;
> +		goto out;
>  	}

[Severity: Medium]
This is a pre-existing issue, but can a wakeup be lost here?
nci_uart_write_work() checks NCI_UART_TX_WAKEUP before it clears
NCI_UART_SENDING:

net/nfc/nci/uart.c:nci_uart_write_work() {
    ...
	if (test_bit(NCI_UART_TX_WAKEUP, &nu->tx_state))
		goto restart;

	if (nu->ops.tx_done && nci_uart_queue_empty(nu))
		nu->ops.tx_done(nu);

	clear_bit(NCI_UART_SENDING, &nu->tx_state);
}

Consider this interleaving:

CPU1 (nci_uart_write_work)
  dequeue loop ends with tx_q empty
  test_bit(NCI_UART_TX_WAKEUP) -> false

CPU2 (nci_uart_send)
  skb_queue_tail(&nu->tx_q, skb)
  nci_uart_tx_wakeup()
    test_and_set_bit(NCI_UART_SENDING) -> 1
    set_bit(NCI_UART_TX_WAKEUP)

CPU1
  tx_done skipped, queue not empty
  clear_bit(NCI_UART_SENDING)

The new skb then stays on nu->tx_q with no work scheduled until an
unrelated wakeup arrives, so the pending NCI command can time out.

nci_uart_tty_wakeup() also clears TTY_DO_WRITE_WAKEUP. A tty wakeup that
lands while SENDING is set therefore doesn't reliably rescue the frame.

hci_uart_tx_wakeup() and hci_uart_write_work() avoid this race. They set
TX_WAKEUP before test_and_set_bit(SENDING), and clear SENDING before
rechecking TX_WAKEUP.

Since this function is being rewritten to follow hci_uart, could it use
that ordering too?

> @@ -123,18 +139,46 @@ static int nci_uart_set_driver(struct tty_struct *tty, unsigned int driver)
>  	INIT_WORK(&nu->write_work, nci_uart_write_work);
>  	spin_lock_init(&nu->rx_lock);
>  
> -	ret = nu->ops.open(nu);
> -	if (ret) {
> -		kfree(nu);
> -		return ret;
> -	} else if (!try_module_get(nu->owner)) {
> -		nu->ops.close(nu);
> -		kfree(nu);
> -		return -ENOENT;
> +	ret = percpu_init_rwsem(&nu->tx_lock);
> +	if (ret)
> +		goto err_free;
> +
> +	/* Take the module reference and enable the write worker before the
> +	 * device is registered: ops.open() may already transmit (e.g. download
> +	 * firmware), and user space can use the interface as soon as it is
> +	 * registered.
> +	 */
> +	if (!try_module_get(nu->owner)) {
> +		ret = -ENOENT;
> +		goto err_rwsem;
>  	}

[Severity: Medium]
This is a pre-existing issue, but is the module reference still taken too
late? Earlier in nci_uart_set_driver(), the template is read from the
registry with no lock and no module reference held:

	if (!nci_uart_drivers[driver])
		return -ENOENT;
	...
	memcpy(nu, nci_uart_drivers[driver], sizeof(struct nci_uart));

The template lives in the nfcmrvl_uart module image.
nci_uart_unregister() clears the slot without any synchronization:

	nci_uart_drivers[nu->driver] = NULL;

Suppose an rmmod of nfcmrvl_uart completes between the slot check and
try_module_get(). Can the memcpy() then read freed module memory, or
try_module_get() run on a freed struct module?

Moving try_module_get() ahead of ops.open() narrows this, but the window
between lookup and reference remains. Triggering it needs CAP_SYS_MODULE
racing the ioctl.

> +
> +	set_bit(NCI_UART_READY, &nu->tx_state);
> +
> +	ret = nu->ops.open(nu);
> +	if (ret)
> +		goto err_ready;
> +
>  	tty->disc_data = nu;

[Severity: High]
This isn't a bug introduced by this patch, but can two concurrent
NCIUARTSETDRIVER ioctls on the same tty both get here?

tty_ioctl() takes only a shared ldisc reference before calling
ld->ops->ioctl. nci_uart_tty_ioctl() then checks tty->disc_data without
any serialization:

	case NCIUARTSETDRIVER:
		if (!nu)
			return nci_uart_set_driver(tty, (unsigned int)arg);
		else
			return -EBUSY;

tty->disc_data is stored only after nu->ops.open() returns, so the window
covers all of device registration. Both callers can allocate their own nu,
set NCI_UART_READY and register a separate NFC device. The second store
then overwrites tty->disc_data.

nci_uart_tty_close() tears down only the instance in tty->disc_data. The
first nu is never unregistered or freed, its write_work is never
cancelled, and it keeps its module reference.

Setting N_NCI and issuing this ioctl need no capability, so an
unprivileged pty user can repeat the race.

Once the tty has been freed, bringing the orphaned NFC device up looks
like it would use the freed nu->tty:

NFC_CMD_DEV_UP
  nci_open_device()
    nci_uart_send()
      nci_uart_tx_wakeup()          <- NCI_UART_READY is still set
        nci_uart_write_work()
          set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
          tty->ops->write(tty, skb->data, skb->len);

That last step needs NFC_CMD_DEV_UP, which is GENL_ADMIN_PERM.

hci_ldisc guards HCIUARTSETPROTO with test_and_set_bit(HCI_UART_PROTO_SET,
...). Would a similar guard work here?

[Severity: Low]
This is a pre-existing issue, but tty->disc_data is published only after
nu->ops.open() has registered the NCI device. In that window,
nci_uart_tty_wakeup() and nci_uart_tty_receive() return early because
disc_data is still NULL:

	struct nci_uart *nu = tty->disc_data;

	if (!nu)
		return;

A write_wakeup after a partial write, or response bytes arriving in this
window, are therefore dropped.

TX does not stall for good, because nci_uart_write_work() always clears
NCI_UART_SENDING at the end. The worst case seems to be one NCI command
timing out, for example a dev_up issued right after
nci_register_device(). This patch does not change that behaviour.

[ ... ]

> @@ -180,16 +224,24 @@ static void nci_uart_tty_close(struct tty_struct *tty)
>  	if (!nu)
>  		return;
>  
> -	kfree_skb(nu->tx_skb);
> -	kfree_skb(nu->rx_skb);
> +	/* Drain in-flight tx_wakeups and block new ones, so write_work cannot
> +	 * be requeued once it is cancelled below.
> +	 */
> +	percpu_down_write(&nu->tx_lock);
> +	clear_bit(NCI_UART_READY, &nu->tx_state);
> +	percpu_up_write(&nu->tx_lock);
>  
> -	skb_queue_purge(&nu->tx_q);
> +	cancel_work_sync(&nu->write_work);
>  
>  	nu->ops.close(nu);

[Severity: Medium]
Does this prevent the CORE_RESET sent during unregistration from reaching
the controller?

For nfcmrvl_uart, ops.close() ends up in nci_close_device(), which sends
CORE_RESET when NCI_UP is set:

nfcmrvl_nci_uart_close()
  nfcmrvl_nci_unregister_dev()
    nci_unregister_device()
      nci_close_device()
        __nci_request(ndev, nci_reset_req, ...)
          nfcmrvl_nci_send()
            nfcmrvl_uart_nci_send()
              nci_uart_send()
                skb_queue_tail(&nu->tx_q, skb)
                nci_uart_tx_wakeup()

NCI_UART_READY is already clear at this point. nci_uart_tx_wakeup() now
returns without scheduling write_work:

	if (!test_bit(NCI_UART_READY, &nu->tx_state))
		goto out;

The frame is never written to the tty. skb_queue_purge(&nu->tx_q) frees
it later.

Nothing else on this path quiesces the NFCC. nfcmrvl_uart_nci_close() is
a no-op, and nfcmrvl_nci_unregister_dev() neither calls
nfcmrvl_chip_halt() nor touches the reset GPIO.

Before this patch, write_work could still run during ops.close(). The
reset therefore reached the UART whenever the tty was still working at
ldisc close. Examples are a TIOCSETD switch, or a hangup where
tty_ldisc_hangup() runs before tty->ops->hangup().

If the ldisc is detached while the device is up and polling, can the NFCC
keep its RF and firmware state until the next power cycle, GPIO reset or
dev_up?

hci_ldisc makes the same choice by clearing HCI_UART_PROTO_READY before
hci_unregister_dev(). The commit message doesn't mention this behaviour
change for NCI UART, though.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006115532.72100-1-shubham%40octane.security

Re: [PATCH v4 0/2] nfc: nci: uart: fix write_work teardown UAF (+ nfcmrvl drv_data)

[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.

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.