[PATCH net v6 1/6] nfc: nci: skip controller reset while unregistering

[PATCH net v6 1/6] nfc: nci: skip controller reset while unregistering

[Shubham Antil]

nci_unregister_device() marks a device as unregistering before closing it,
but nci_close_device() still sends CORE_RESET and waits for a response.  A
transport may already be detaching at this point.  In particular, line
discipline close holds the TTY line discipline lock for writing, so receive
callbacks cannot deliver the response and close waits for the full timeout.

Skip the reset request when NCI_UNREG is set.  Keep the reset for a normal
device-down operation, where the transport remains attached and can deliver
the response.

Found during a source audit assisted by OpenAI Codex, gpt-5.6-sol, and
reproduced by closing a pty-backed NCI UART while its NCI device was up.

Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation")
Cc: stable@vger.kernel.org
Assisted-by: LLM KASAN
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 net/nfc/nci/core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 73e3a96470ac..8ee33505e93a 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -582,13 +582,15 @@ static int nci_close_device(struct nci_dev *ndev)
 	/* Flush TX wq, RX wq flush can't be under the lock */
 	flush_workqueue(ndev->tx_wq);
 
-	/* Reset device */
+	/* Reset device unless it is being unregistered */
 	skb_queue_purge(&ndev->cmd_q);
-	atomic_set(&ndev->cmd_cnt, 1);
 
-	set_bit(NCI_INIT, &ndev->flags);
-	__nci_request(ndev, nci_reset_req, (void *)0,
-		      msecs_to_jiffies(NCI_RESET_TIMEOUT));
+	if (!test_bit(NCI_UNREG, &ndev->flags)) {
+		atomic_set(&ndev->cmd_cnt, 1);
+		set_bit(NCI_INIT, &ndev->flags);
+		__nci_request(ndev, nci_reset_req, (void *)0,
+			      msecs_to_jiffies(NCI_RESET_TIMEOUT));
+	}
 
 	/* After this point our queues are empty
 	 * rx work may be running but will see that NCI_UP was cleared
-- 
2.43.0

Re: [PATCH net v6 1/6] nfc: nci: skip controller reset while unregistering

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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).

- [Medium] The patch skips CORE_RESET_CMD for every caller of
  nci_unregister_device(), not only for transports that are detaching.
- [Medium] The NCI_UNREG check only removes the CORE_RESET that close
  sends itself.
- [Medium] The new NCI_UNREG gate cannot affect a reset (or init) request
  that is already in flight when line discipline close starts.
- [Low] On the unregister path, nci_close_device() no longer sets
  NCI_INIT, but it still calls ndev->ops->close().

--

nfc: nci: skip controller reset while unregistering

This patch makes nci_close_device() skip sending CORE_RESET (and skip
setting NCI_INIT) when NCI_UNREG is set. The motivation is that on line
discipline close the transport can no longer deliver the response, so close
waits for the full reset timeout.

> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> index 73e3a96470ace..8ee33505e93ad 100644
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -582,13 +582,15 @@ static int nci_close_device(struct nci_dev *ndev)
>  	/* Flush TX wq, RX wq flush can't be under the lock */
>  	flush_workqueue(ndev->tx_wq);
>  
> -	/* Reset device */
> +	/* Reset device unless it is being unregistered */
>  	skb_queue_purge(&ndev->cmd_q);
> -	atomic_set(&ndev->cmd_cnt, 1);
>  
> -	set_bit(NCI_INIT, &ndev->flags);
> -	__nci_request(ndev, nci_reset_req, (void *)0,
> -		      msecs_to_jiffies(NCI_RESET_TIMEOUT));
> +	if (!test_bit(NCI_UNREG, &ndev->flags)) {
> +		atomic_set(&ndev->cmd_cnt, 1);
> +		set_bit(NCI_INIT, &ndev->flags);
> +		__nci_request(ndev, nci_reset_req, (void *)0,
> +			      msecs_to_jiffies(NCI_RESET_TIMEOUT));
> +	}

[Severity: Medium]
This skips the reset for every caller of nci_unregister_device(), not only
for transports that are detaching. Is that intended for drivers whose bus
still works during remove() and whose ops->close() does nothing?

For nfcmrvl over I2C/SPI, nfcmrvl_nci_close() only calls the per-bus close
function, and that function is empty:

drivers/nfc/nfcmrvl/i2c.c:nfcmrvl_i2c_nci_close() {
	return 0;
}

FDP is the same:

drivers/nfc/fdp/fdp.c:fdp_nci_close() {
	return 0;
}

nfcmrvl_nci_unregister_dev() also never calls nfcmrvl_chip_halt(). The
I2C/SPI IRQs are devm-managed, so they are still live during remove().

Before this patch, the CORE_RESET went out over the live bus with NCI_INIT
set. nci_recv_frame() accepted the response, and the NFCC went back to
reset.

With this change, suppose the device is up when nfcmrvl_i2c_remove(),
nfcmrvl_spi_remove() or fdp_nci_remove() runs. Can the controller then stay
in its last RF state (polling with the field on, listen mode, or an active
interface) after the kernel has cleared all of its state?

The commit message says:

  Keep the reset for a normal device-down operation, where the transport
  remains attached and can deliver the response.

For these drivers, the transport is also still attached during unregister.

Later patches in the series don't seem to change this:

- nci_close_device() stays the same.
- nfcmrvl_fw_dnld_abort() returns early when no download is active.
- The new nci_quiesce hook exists only for UART.

[Severity: Low]
On the unregister path, test_and_clear_bit() at the top of
nci_close_device() has already cleared NCI_UP. Now NCI_INIT is not set
either. But ndev->ops->close(ndev) still runs right after this block, and
for ST NCI that callback sends a request:

st_nci_close()
  ndlc_close()
    nci_prop_cmd(ndlc->ndev, ST_NCI_CORE_PROP, ...)
      __nci_request(..., msecs_to_jiffies(NCI_CMD_TIMEOUT))

The reply comes in through llt_ndlc_rcv_queue() -> nci_recv_frame(), which
drops it:

	if (!ndev || (!test_bit(NCI_UP, &ndev->flags) &&
	    !test_bit(NCI_INIT, &ndev->flags))) {

Will the mode-set command in ndlc_close() now always time out after 5
seconds and log a pr_err during unregister?

The impact is limited. st_nci_remove() already calls ndlc_close() directly
before nci_unregister_device(), and that call disables the IRQ, so the old
CORE_RESET also timed out. The stall moves from the reset to the mode-set
command, and total unbind time stays about the same.

[Severity: Medium]
Does this cover a reset or init request that is already running when line
discipline close starts?

nfc_dev_down() takes device_lock and calls nci_dev_down() ->
nci_close_device(). NCI_UNREG isn't set yet, so __nci_request(nci_reset_req)
runs and waits for up to NCI_RESET_TIMEOUT. nfc_dev_up() ->
nci_open_device() does the same with its reset and init requests.

Meanwhile, with the ldisc write lock held:

nci_uart_tty_close()
  tty->disc_data = NULL;
  nu->ops.close(nu)
    nfcmrvl_nci_unregister_dev()
      nci_unregister_device()
        nfc_unregister_rfkill()
          device_lock(&dev->dev);   <- blocks behind nfc_dev_down()

This block happens before set_bit(NCI_UNREG) and before nci_req_cancel() in
nci_close_device().

The pending response can't arrive, because disc_data is NULL and the ldisc
lock is held for writing. So the waiter runs out the full 5000 ms timeout,
and ldisc close waits along with it.

This is the "close waits for the full timeout" symptom from the commit
message, reached through a race. The rest of the series doesn't seem to
change it either. The later UART quiesce patch stops rx/tx before ops.close,
which only makes sure the response never arrives.

Would it work better to mark the device as unregistering and cancel pending
requests before serializing on device_lock? Another option is to make
in-flight requests abort when the transport detaches.

[Severity: Medium]
A related question about requests already in flight. nci_req_cancel() runs
only once, at the top of nci_close_device(). It only completes a request if
one is pending at that exact moment:

net/nfc/nci/core.c:nci_req_cancel() {
	if (ndev->req_status == NCI_REQ_PEND) {
	...
}

nci_open_device() checks NCI_UNREG only on entry:

	if (test_bit(NCI_UNREG, &ndev->flags)) {

After that check, its __nci_request() calls (reset/init, 5000 ms timeouts)
wait out their full timeouts while device_lock is held.

Say NFC_CMD_DEV_UP is waiting there when nci_uart_tty_close() starts.
Unregister then blocks in device_lock() in nfc_unregister_rfkill() until
the timeout expires.

Doesn't that still leave ldisc close stalled for NCI_RESET_TIMEOUT or
NCI_INIT_TIMEOUT, even with the new NCI_UNREG check?

This overlaps with the device_lock ordering question above. Other nfc core
ops take device_lock and check shutting_down, so no new request should be
able to start after nfc_unregister_rfkill().

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

[PATCH net v6 2/6] nfc: nci: uart: serialize driver selection

[Shubham Antil]

NCIUARTSETDRIVER assumes that tty->disc_data remains NULL until a
transport has been selected.  Two concurrent callers can therefore both
allocate and register an NCI device for the same line discipline.

The transport registry can also be changed while a caller is copying a
template from it, before the transport module has been pinned.

Allocate one UART context when the line discipline is opened and claim it
atomically during driver selection.  Protect the transport registry with a
mutex, take the module reference while holding that mutex, and copy only
the immutable template fields.  Keep TTY callbacks disabled until the
transport has finished opening, and unwind the context so a failed
selection can be retried.  Use acquire/release bit operations for the
selection claim so a retry observes the completed unwind.  Treat the driver
number as unsigned before using it to index the registry.

Found during a source audit assisted by OpenAI Codex, gpt-5.6-sol, and
reproduced with two concurrent NCIUARTSETDRIVER ioctls on a pty-backed NCI
UART.

Fixes: 9961127d4bce ("NFC: nci: add generic uart support")
Cc: stable@vger.kernel.org
Assisted-by: LLM KASAN
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 include/net/nfc/nci_core.h |   1 +
 net/nfc/nci/uart.c         | 133 +++++++++++++++++++++++++------------
 2 files changed, 93 insertions(+), 41 deletions(-)

diff --git a/include/net/nfc/nci_core.h b/include/net/nfc/nci_core.h
index 664d5058e66e..2f542f5c9496 100644
--- a/include/net/nfc/nci_core.h
+++ b/include/net/nfc/nci_core.h
@@ -454,6 +454,7 @@ struct nci_uart {
 	spinlock_t		rx_lock;
 	struct work_struct	write_work;
 	struct tty_struct	*tty;
+	unsigned long		flags;
 	unsigned long		tx_state;
 	struct sk_buff_head	tx_q;
 	struct sk_buff		*tx_skb;
diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
index aa20e8603f32..fb9ed281f62c 100644
--- a/net/nfc/nci/uart.c
+++ b/net/nfc/nci/uart.c
@@ -13,6 +13,7 @@
 
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/mutex.h>
 #include <linux/types.h>
 #include <linux/fcntl.h>
 #include <linux/interrupt.h>
@@ -34,7 +35,12 @@
 #define NCI_UART_SENDING	1
 #define NCI_UART_TX_WAKEUP	2
 
+/* UART flags */
+#define NCI_UART_DRIVER_SET	0
+#define NCI_UART_READY		1
+
 static struct nci_uart *nci_uart_drivers[NCI_UART_DRIVER_MAX];
+static DEFINE_MUTEX(nci_uart_drivers_lock);
 
 static inline struct sk_buff *nci_uart_dequeue(struct nci_uart *nu)
 {
@@ -102,39 +108,57 @@ restart:
 	clear_bit(NCI_UART_SENDING, &nu->tx_state);
 }
 
-static int nci_uart_set_driver(struct tty_struct *tty, unsigned int driver)
+static int nci_uart_set_driver(struct nci_uart *nu, unsigned int driver)
 {
-	struct nci_uart *nu = NULL;
+	struct nci_uart *template;
 	int ret;
 
 	if (driver >= NCI_UART_DRIVER_MAX)
 		return -EINVAL;
 
-	if (!nci_uart_drivers[driver])
-		return -ENOENT;
+	if (test_and_set_bit_lock(NCI_UART_DRIVER_SET, &nu->flags))
+		return -EBUSY;
 
-	nu = kzalloc_obj(*nu);
-	if (!nu)
-		return -ENOMEM;
+	mutex_lock(&nci_uart_drivers_lock);
+	template = nci_uart_drivers[driver];
+	if (!template || !try_module_get(template->owner)) {
+		ret = -ENOENT;
+		goto err_unlock;
+	}
 
-	memcpy(nu, nci_uart_drivers[driver], sizeof(struct nci_uart));
-	nu->tty = tty;
-	skb_queue_head_init(&nu->tx_q);
-	INIT_WORK(&nu->write_work, nci_uart_write_work);
-	spin_lock_init(&nu->rx_lock);
+	nu->owner = template->owner;
+	nu->ops = template->ops;
+	nu->name = template->name;
+	nu->driver = template->driver;
+	mutex_unlock(&nci_uart_drivers_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;
-	}
-	tty->disc_data = nu;
+	if (ret)
+		goto err_module;
+
+	/* Publish transport fields before callbacks observe READY. */
+	smp_mb__before_atomic();
+	set_bit(NCI_UART_READY, &nu->flags);
 
 	return 0;
+
+err_module:
+	cancel_work_sync(&nu->write_work);
+	kfree_skb(nu->tx_skb);
+	nu->tx_skb = NULL;
+	kfree_skb(nu->rx_skb);
+	nu->rx_skb = NULL;
+	skb_queue_purge(&nu->tx_q);
+	clear_bit(NCI_UART_SENDING, &nu->tx_state);
+	clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+	module_put(nu->owner);
+	nu->owner = NULL;
+	goto err_clear;
+err_unlock:
+	mutex_unlock(&nci_uart_drivers_lock);
+err_clear:
+	clear_bit_unlock(NCI_UART_DRIVER_SET, &nu->flags);
+	return ret;
 }
 
 /* ------ LDISC part ------ */
@@ -156,7 +180,17 @@ static int nci_uart_tty_open(struct tty_struct *tty)
 	if (!tty->ops->write)
 		return -EOPNOTSUPP;
 
-	tty->disc_data = NULL;
+	struct nci_uart *nu = kzalloc_obj(*nu);
+
+	if (!nu)
+		return -ENOMEM;
+
+	nu->tty = tty;
+	skb_queue_head_init(&nu->tx_q);
+	INIT_WORK(&nu->write_work, nci_uart_write_work);
+	spin_lock_init(&nu->rx_lock);
+
+	tty->disc_data = nu;
 	tty->receive_room = 65536;
 
 	/* Flush any pending characters in the driver */
@@ -180,14 +214,17 @@ static void nci_uart_tty_close(struct tty_struct *tty)
 	if (!nu)
 		return;
 
-	kfree_skb(nu->tx_skb);
-	kfree_skb(nu->rx_skb);
+	if (test_bit(NCI_UART_DRIVER_SET, &nu->flags)) {
+		kfree_skb(nu->tx_skb);
+		kfree_skb(nu->rx_skb);
 
-	skb_queue_purge(&nu->tx_q);
+		skb_queue_purge(&nu->tx_q);
+
+		nu->ops.close(nu);
+		module_put(nu->owner);
+	}
 
-	nu->ops.close(nu);
 	nu->tty = NULL;
-	module_put(nu->owner);
 
 	cancel_work_sync(&nu->write_work);
 
@@ -211,7 +248,8 @@ static void nci_uart_tty_wakeup(struct tty_struct *tty)
 
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 
-	if (tty != nu->tty)
+	if (tty != nu->tty ||
+	    !test_bit_acquire(NCI_UART_READY, &nu->flags))
 		return;
 
 	nci_uart_tx_wakeup(nu);
@@ -300,7 +338,8 @@ static void nci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
 {
 	struct nci_uart *nu = tty->disc_data;
 
-	if (!nu || tty != nu->tty)
+	if (!nu || tty != nu->tty ||
+	    !test_bit_acquire(NCI_UART_READY, &nu->flags))
 		return;
 
 	spin_lock(&nu->rx_lock);
@@ -328,13 +367,12 @@ static int nci_uart_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
 	struct nci_uart *nu = tty->disc_data;
 	int err = 0;
 
+	if (!nu)
+		return -EBADF;
+
 	switch (cmd) {
 	case NCIUARTSETDRIVER:
-		if (!nu)
-			return nci_uart_set_driver(tty, (unsigned int)arg);
-		else
-			return -EBUSY;
-		break;
+		return nci_uart_set_driver(nu, (unsigned int)arg);
 	default:
 		err = n_tty_ioctl_helper(tty, cmd, arg);
 		break;
@@ -370,33 +408,46 @@ static int nci_uart_send(struct nci_uart *nu, struct sk_buff *skb)
 
 int nci_uart_register(struct nci_uart *nu)
 {
+	int ret = 0;
+
 	if (!nu || !nu->ops.open ||
-	    !nu->ops.recv || !nu->ops.close)
+	    !nu->ops.recv || !nu->ops.close ||
+	    (unsigned int)nu->driver >= NCI_UART_DRIVER_MAX)
 		return -EINVAL;
 
-	/* Set the send callback */
-	nu->ops.send = nci_uart_send;
-
 	/* Add this driver in the driver list */
+	mutex_lock(&nci_uart_drivers_lock);
 	if (nci_uart_drivers[nu->driver]) {
 		pr_err("driver %d is already registered\n", nu->driver);
-		return -EBUSY;
+		ret = -EBUSY;
+		goto unlock;
 	}
+
+	/* Set the send callback */
+	nu->ops.send = nci_uart_send;
 	nci_uart_drivers[nu->driver] = nu;
 
 	pr_info("NCI uart driver '%s [%d]' registered\n", nu->name, nu->driver);
 
-	return 0;
+unlock:
+	mutex_unlock(&nci_uart_drivers_lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(nci_uart_register);
 
 void nci_uart_unregister(struct nci_uart *nu)
 {
+	mutex_lock(&nci_uart_drivers_lock);
+
 	pr_info("NCI uart driver '%s [%d]' unregistered\n", nu->name,
 		nu->driver);
 
 	/* Remove this driver from the driver list */
-	nci_uart_drivers[nu->driver] = NULL;
+	if ((unsigned int)nu->driver < NCI_UART_DRIVER_MAX &&
+	    nci_uart_drivers[nu->driver] == nu)
+		nci_uart_drivers[nu->driver] = NULL;
+
+	mutex_unlock(&nci_uart_drivers_lock);
 }
 EXPORT_SYMBOL_GPL(nci_uart_unregister);
 
-- 
2.43.0

[PATCH net v6 3/6] nfc: nci: uart: quiesce callbacks during teardown

[Shubham Antil]

nci_uart_tty_close() releases transport state before ensuring that the
UART write worker and receive callback have stopped using it.  A write
wakeup can also race the worker's final TX_WAKEUP test and SENDING handoff.

Serialize work scheduling and the worker's final state transition with a
transmit lock.  Once quiescing starts, block new scheduling and cancel the
worker synchronously.  Clear the TTY write-wakeup request after the
cancellation.  Keep a locked TX_WAKEUP check before tx_done() and another
after it so the original Marvell BREAK timing is preserved while the
lost-wakeup window remains closed.

Stop generic receive and transmit callbacks before invoking the transport
close operation.  The preceding change skips the unregister-time controller
reset because the TTY core cannot deliver its response after line
discipline close starts.  Detach the UART context from the TTY before
quiescing, and let the Marvell transport teardown clear its back-pointers
before releasing private state.  Apply the same barriers to the failed-open
path.

Found during a source audit assisted by OpenAI Codex, gpt-5.6-sol, and
reproduced with repeated pty-backed NCI UART registration and hangup cycles
under KASAN.

Fixes: 9961127d4bce ("NFC: nci: add generic uart support")
Cc: stable@vger.kernel.org
Assisted-by: LLM KASAN
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 drivers/nfc/nfcmrvl/main.c    |   5 ++
 drivers/nfc/nfcmrvl/nfcmrvl.h |   1 +
 drivers/nfc/nfcmrvl/uart.c    |  11 ++++
 include/net/nfc/nci_core.h    |   5 ++
 net/nfc/nci/uart.c            | 109 ++++++++++++++++++++++++++++------
 5 files changed, 113 insertions(+), 18 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 6efa83219117..baf4c781ee6b 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -167,6 +167,8 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 	return priv;
 
 error_fw_dnld_deinit:
+	if (priv->if_ops->nci_quiesce)
+		priv->if_ops->nci_quiesce(priv);
 	nfcmrvl_fw_dnld_deinit(priv);
 error_free_dev:
 	nci_free_device(priv->ndev);
@@ -181,6 +183,9 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
 	struct nci_dev *ndev = priv->ndev;
 
 	nci_unregister_device(ndev);
+	if (priv->if_ops->nci_quiesce)
+		priv->if_ops->nci_quiesce(priv);
+
 	if (priv->ndev->nfc_dev->fw_download_in_progress)
 		nfcmrvl_fw_dnld_abort(priv);
 
diff --git a/drivers/nfc/nfcmrvl/nfcmrvl.h b/drivers/nfc/nfcmrvl/nfcmrvl.h
index 62fa77d587b2..61b3f4a4f096 100644
--- a/drivers/nfc/nfcmrvl/nfcmrvl.h
+++ b/drivers/nfc/nfcmrvl/nfcmrvl.h
@@ -112,6 +112,7 @@ struct nfcmrvl_if_ops {
 	int (*nci_open) (struct nfcmrvl_private *priv);
 	int (*nci_close) (struct nfcmrvl_private *priv);
 	int (*nci_send) (struct nfcmrvl_private *priv, struct sk_buff *skb);
+	void (*nci_quiesce)(struct nfcmrvl_private *priv);
 	void (*nci_update_config)(struct nfcmrvl_private *priv,
 				  const void *param);
 };
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 9aedd168759c..66803f1c6fd3 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -44,6 +44,16 @@ static int nfcmrvl_uart_nci_send(struct nfcmrvl_private *priv,
 	return nu->ops.send(nu, skb);
 }
 
+static void nfcmrvl_uart_nci_quiesce(struct nfcmrvl_private *priv)
+{
+	struct nci_uart *nu = priv->drv_data;
+
+	nci_uart_rx_stop(nu);
+	nci_uart_tx_stop(nu);
+	nu->drv_data = NULL;
+	nu->ndev = NULL;
+}
+
 static void nfcmrvl_uart_nci_update_config(struct nfcmrvl_private *priv,
 					   const void *param)
 {
@@ -58,6 +68,7 @@ static const struct nfcmrvl_if_ops uart_ops = {
 	.nci_open = nfcmrvl_uart_nci_open,
 	.nci_close = nfcmrvl_uart_nci_close,
 	.nci_send = nfcmrvl_uart_nci_send,
+	.nci_quiesce = nfcmrvl_uart_nci_quiesce,
 	.nci_update_config = nfcmrvl_uart_nci_update_config
 };
 
diff --git a/include/net/nfc/nci_core.h b/include/net/nfc/nci_core.h
index 2f542f5c9496..99797872c2e0 100644
--- a/include/net/nfc/nci_core.h
+++ b/include/net/nfc/nci_core.h
@@ -436,6 +436,7 @@ struct nci_uart;
 
 struct nci_uart_ops {
 	int (*open)(struct nci_uart *nci_uart);
+	/* Release transport-private state after generic callbacks stop. */
 	void (*close)(struct nci_uart *nci_uart);
 	int (*recv)(struct nci_uart *nci_uart, struct sk_buff *skb);
 	int (*send)(struct nci_uart *nci_uart, struct sk_buff *skb);
@@ -452,6 +453,7 @@ struct nci_uart {
 	/* Dynamic data */
 	struct nci_dev		*ndev;
 	spinlock_t		rx_lock;
+	spinlock_t		tx_lock; /* protects tx_state */
 	struct work_struct	write_work;
 	struct tty_struct	*tty;
 	unsigned long		flags;
@@ -466,5 +468,8 @@ struct nci_uart {
 int nci_uart_register(struct nci_uart *nu);
 void nci_uart_unregister(struct nci_uart *nu);
 void nci_uart_set_config(struct nci_uart *nu, int baudrate, int flow_ctrl);
+void nci_uart_set_ready(struct nci_uart *nu);
+void nci_uart_rx_stop(struct nci_uart *nu);
+void nci_uart_tx_stop(struct nci_uart *nu);
 
 #endif /* __NCI_CORE_H */
diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c
index fb9ed281f62c..aa24e1f572d9 100644
--- a/net/nfc/nci/uart.c
+++ b/net/nfc/nci/uart.c
@@ -34,6 +34,7 @@
 /* TX states  */
 #define NCI_UART_SENDING	1
 #define NCI_UART_TX_WAKEUP	2
+#define NCI_UART_CLOSING	3
 
 /* UART flags */
 #define NCI_UART_DRIVER_SET	0
@@ -64,24 +65,61 @@ static inline int nci_uart_queue_empty(struct nci_uart *nu)
 
 static int nci_uart_tx_wakeup(struct nci_uart *nu)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	if (test_bit(NCI_UART_CLOSING, &nu->tx_state) ||
+	    !test_bit_acquire(NCI_UART_READY, &nu->flags))
+		goto unlock;
+
 	if (test_and_set_bit(NCI_UART_SENDING, &nu->tx_state)) {
 		set_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
-		return 0;
+		goto unlock;
 	}
 
 	schedule_work(&nu->write_work);
 
+unlock:
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
+
 	return 0;
 }
 
+void nci_uart_tx_stop(struct nci_uart *nu)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	set_bit(NCI_UART_CLOSING, &nu->tx_state);
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
+
+	cancel_work_sync(&nu->write_work);
+	clear_bit(TTY_DO_WRITE_WAKEUP, &nu->tty->flags);
+
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	clear_bit(NCI_UART_SENDING, &nu->tx_state);
+	clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
+}
+EXPORT_SYMBOL_GPL(nci_uart_tx_stop);
+
 static void nci_uart_write_work(struct work_struct *work)
 {
 	struct nci_uart *nu = container_of(work, struct nci_uart, write_work);
 	struct tty_struct *tty = nu->tty;
 	struct sk_buff *skb;
+	unsigned long flags;
 
 restart:
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	if (test_bit(NCI_UART_CLOSING, &nu->tx_state)) {
+		clear_bit(NCI_UART_SENDING, &nu->tx_state);
+		clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+		spin_unlock_irqrestore(&nu->tx_lock, flags);
+		return;
+	}
 	clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
 
 	if (nu->ops.tx_start)
 		nu->ops.tx_start(nu);
@@ -99,14 +137,50 @@ restart:
 		kfree_skb(skb);
 	}
 
-	if (test_bit(NCI_UART_TX_WAKEUP, &nu->tx_state))
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	if (test_bit(NCI_UART_CLOSING, &nu->tx_state)) {
+		clear_bit(NCI_UART_SENDING, &nu->tx_state);
+		clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+		spin_unlock_irqrestore(&nu->tx_lock, flags);
+		return;
+	}
+	if (test_and_clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state)) {
+		spin_unlock_irqrestore(&nu->tx_lock, flags);
 		goto restart;
+	}
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
 
 	if (nu->ops.tx_done && nci_uart_queue_empty(nu))
 		nu->ops.tx_done(nu);
 
+	spin_lock_irqsave(&nu->tx_lock, flags);
+	if (!test_bit(NCI_UART_CLOSING, &nu->tx_state) &&
+	    test_and_clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state)) {
+		spin_unlock_irqrestore(&nu->tx_lock, flags);
+		goto restart;
+	}
 	clear_bit(NCI_UART_SENDING, &nu->tx_state);
+	clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+	spin_unlock_irqrestore(&nu->tx_lock, flags);
+}
+
+void nci_uart_set_ready(struct nci_uart *nu)
+{
+	/* Publish transport fields before callbacks observe READY. */
+	smp_mb__before_atomic();
+	set_bit(NCI_UART_READY, &nu->flags);
 }
+EXPORT_SYMBOL_GPL(nci_uart_set_ready);
+
+void nci_uart_rx_stop(struct nci_uart *nu)
+{
+	spin_lock(&nu->rx_lock);
+	clear_bit(NCI_UART_READY, &nu->flags);
+	kfree_skb(nu->rx_skb);
+	nu->rx_skb = NULL;
+	spin_unlock(&nu->rx_lock);
+}
+EXPORT_SYMBOL_GPL(nci_uart_rx_stop);
 
 static int nci_uart_set_driver(struct nci_uart *nu, unsigned int driver)
 {
@@ -136,21 +210,17 @@ static int nci_uart_set_driver(struct nci_uart *nu, unsigned int driver)
 	if (ret)
 		goto err_module;
 
-	/* Publish transport fields before callbacks observe READY. */
-	smp_mb__before_atomic();
-	set_bit(NCI_UART_READY, &nu->flags);
+	nci_uart_set_ready(nu);
 
 	return 0;
 
 err_module:
-	cancel_work_sync(&nu->write_work);
+	nci_uart_rx_stop(nu);
+	nci_uart_tx_stop(nu);
 	kfree_skb(nu->tx_skb);
 	nu->tx_skb = NULL;
-	kfree_skb(nu->rx_skb);
-	nu->rx_skb = NULL;
 	skb_queue_purge(&nu->tx_q);
-	clear_bit(NCI_UART_SENDING, &nu->tx_state);
-	clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
+	clear_bit(NCI_UART_CLOSING, &nu->tx_state);
 	module_put(nu->owner);
 	nu->owner = NULL;
 	goto err_clear;
@@ -188,6 +258,7 @@ static int nci_uart_tty_open(struct tty_struct *tty)
 	nu->tty = tty;
 	skb_queue_head_init(&nu->tx_q);
 	INIT_WORK(&nu->write_work, nci_uart_write_work);
+	spin_lock_init(&nu->tx_lock);
 	spin_lock_init(&nu->rx_lock);
 
 	tty->disc_data = nu;
@@ -208,26 +279,24 @@ static void nci_uart_tty_close(struct tty_struct *tty)
 {
 	struct nci_uart *nu = tty->disc_data;
 
-	/* Detach from the tty */
-	tty->disc_data = NULL;
-
 	if (!nu)
 		return;
 
+	tty->disc_data = NULL;
+
 	if (test_bit(NCI_UART_DRIVER_SET, &nu->flags)) {
-		kfree_skb(nu->tx_skb);
-		kfree_skb(nu->rx_skb);
+		nci_uart_rx_stop(nu);
+		nci_uart_tx_stop(nu);
+		nu->ops.close(nu);
 
+		kfree_skb(nu->tx_skb);
 		skb_queue_purge(&nu->tx_q);
 
-		nu->ops.close(nu);
 		module_put(nu->owner);
 	}
 
 	nu->tty = NULL;
 
-	cancel_work_sync(&nu->write_work);
-
 	kfree(nu);
 }
 
@@ -343,6 +412,10 @@ static void nci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
 		return;
 
 	spin_lock(&nu->rx_lock);
+	if (!test_bit(NCI_UART_READY, &nu->flags)) {
+		spin_unlock(&nu->rx_lock);
+		return;
+	}
 	nci_uart_default_recv_buf(nu, data, count);
 	spin_unlock(&nu->rx_lock);
 
-- 
2.43.0

Re: [PATCH net v6 3/6] nfc: nci: uart: quiesce callbacks during teardown

[netdev-bot+sashiko]

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

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

- [Medium] When nci_uart_tty_close() tears down an NCI UART whose NCI
  device is up, the NFC controller (NFCC) is left running in whatever RF…
- [Medium] Baseline decision: fixed later in the series.
- [Low] Baseline decision: not a bug in practice.

Pre-existing issues:
- [High] Pre-existing problem, not introduced by this patch: during a
  Marvell firmware download, the UART peer (NFCC) controls a 16-bit…
- [High] Pre-existing problem, not introduced by this patch: the Marvell
  firmware download copies from the firmware image at a peer-advanced…
- [High] Baseline decision: pre-existing and fixed later in the series.

--

nfc: nci: uart: quiesce callbacks during teardown

This patch adds a tx_lock and a CLOSING transmit state. Together they
serialize nci_uart_tx_wakeup() and nci_uart_write_work() against teardown.
It also adds nci_uart_rx_stop() and nci_uart_tx_stop(), which stop the
generic receive and transmit callbacks before the transport close operation
runs. The Marvell UART transport gets a new nci_quiesce hook that stops
these callbacks and clears its back-pointers before private state is
released.

> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index 6efa832191172..baf4c781ee6b6 100644
> --- a/drivers/nfc/nfcmrvl/main.c
> +++ b/drivers/nfc/nfcmrvl/main.c

[ ... ]

> @@ -181,6 +183,9 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
>  	struct nci_dev *ndev = priv->ndev;
>  
>  	nci_unregister_device(ndev);
> +	if (priv->if_ops->nci_quiesce)
> +		priv->if_ops->nci_quiesce(priv);
> +
>  	if (priv->ndev->nfc_dev->fw_download_in_progress)
>  		nfcmrvl_fw_dnld_abort(priv);
>  

[Severity: High]
This isn't a bug introduced by this patch, but can this teardown race with
a firmware download timeout callback that is already running?

fw_dnld_over() only waits for the timer if it is still pending:

drivers/nfc/nfcmrvl/fw_dnld.c:fw_dnld_over() {
    ...
	if (timer_pending(&priv->fw_dnld.timer))
		timer_delete_sync(&priv->fw_dnld.timer);
    ...
}

If fw_dnld_timeout() is already executing, timer_pending() returns false
and nothing waits for it. nfcmrvl_nci_unregister_dev() then continues to:

	nci_free_device(ndev);
	kfree(priv);

The callback may still be using priv and priv->ndev at that point.

A later patch in this series, "nfc: nfcmrvl: stop firmware work before
unregistering", appears to fix this. With it, fw_dnld_timeout() only queues
timeout_work, and nfcmrvl_fw_dnld_abort() calls timer_shutdown_sync() and
cancel_work_sync(&timeout_work) before nci_unregister_device().

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. While
looking at the firmware download teardown here: can a UART peer make
process_state_fw_dnld() overflow out_skb?

In SUBSTATE_WAIT_COMMAND, any length is accepted as long as its complement
matches (for example len = 0xFFFF with comp = 0x0000). It is then stored
with no upper bound:

drivers/nfc/nfcmrvl/fw_dnld.c:process_state_fw_dnld() {
    ...
		priv->fw_dnld.chunk_len = len;
    ...
}

In SUBSTATE_WAIT_ACK_CREDIT, the skb comes from alloc_lc_skb(), which takes
a uint8_t plen, so it holds at most 255 payload bytes. skb_put_data() then
uses the full chunk_len:

			out_skb = alloc_lc_skb(priv, priv->fw_dnld.chunk_len);
			if (!out_skb)
				return -ENOMEM;
			skb_put_data(out_skb,
				     ((uint8_t *)priv->fw_dnld.fw->data) + priv->fw_dnld.offset,
				     priv->fw_dnld.chunk_len);

With chunk_len = 65535, would skb_put() reach skb_over_panic() and BUG()?

This is reachable once a CAP_NET_ADMIN user starts NFC_CMD_FW_DOWNLOAD. The
lengths come from the device or from a pty-backed peer.

[Severity: High]
This is also a pre-existing issue, not one introduced by this patch. Does
anything in process_state_fw_dnld() bound priv->fw_dnld.offset against
priv->fw_dnld.fw->size?

SUBSTATE_WAIT_DATA_CREDIT advances the offset by the length the peer
chose:

		priv->fw_dnld.offset += priv->fw_dnld.chunk_len;

SUBSTATE_WAIT_ACK_CREDIT then copies from that offset using the
skb_put_data() call quoted above. Neither step checks offset + chunk_len
against fw->size.

Suppose a peer keeps asking for chunks of 255 bytes or less and sends the
expected credit notifications. Can offset then move past the end of the
firmware image? If so, the kernel would read memory beyond fw->data and
send it to the peer in NCI data frames, or fault on an unmapped page.

[ ... ]

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

[ ... ]

> @@ -64,24 +65,61 @@ static inline int nci_uart_queue_empty(struct nci_uart *nu)
>  
>  static int nci_uart_tx_wakeup(struct nci_uart *nu)
>  {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&nu->tx_lock, flags);
> +	if (test_bit(NCI_UART_CLOSING, &nu->tx_state) ||
> +	    !test_bit_acquire(NCI_UART_READY, &nu->flags))
> +		goto unlock;

[Severity: Medium]
Can this READY check leave a frame stuck in nu->tx_q?

When READY is not set yet, the kick is dropped without setting
NCI_UART_TX_WAKEUP or recording that work is pending. nci_uart_set_ready()
also does not check tx_q when it publishes READY.

For Marvell, nci_register_device() runs from inside ops.open(), before
READY is set:

nci_uart_set_driver()
    nu->ops.open(nu)
        nfcmrvl_nci_uart_open()
            nfcmrvl_nci_register_dev()
                nci_register_device()
    nci_uart_set_ready(nu)

If NFC_CMD_DEV_UP arrives in that window, nci_uart_send() queues
CORE_RESET into nu->tx_q, but write_work is never scheduled. Would
nci_open_device() then time out, and would the stale CORE_RESET go out
ahead of the next command that kicks the worker?

Before this patch, the same window scheduled write_work and could
dereference a NULL nu->drv_data in nfcmrvl_nci_uart_tx_start(), so this is
still better than that crash.

A later patch in this series, "nfc: nfcmrvl: publish UART state before
registration", calls nci_uart_set_ready() before nci_register_device(),
which closes this window.

[ ... ]

> @@ -99,14 +137,50 @@ static void nci_uart_write_work(struct work_struct *work)
>  		kfree_skb(skb);
>  	}
>  
> -	if (test_bit(NCI_UART_TX_WAKEUP, &nu->tx_state))
> +	spin_lock_irqsave(&nu->tx_lock, flags);
> +	if (test_bit(NCI_UART_CLOSING, &nu->tx_state)) {
> +		clear_bit(NCI_UART_SENDING, &nu->tx_state);
> +		clear_bit(NCI_UART_TX_WAKEUP, &nu->tx_state);
> +		spin_unlock_irqrestore(&nu->tx_lock, flags);
> +		return;
> +	}

[Severity: Low]
This might not matter in practice. This early return skips ops.tx_done()
even though ops.tx_start() already ran in this pass.

With Marvell break_control, nfcmrvl_nci_uart_tx_start() calls
break_ctl(tty, 0) and nfcmrvl_nci_uart_tx_done() calls break_ctl(tty, -1).
If teardown hits the worker mid-burst, the line is left with BREAK
de-asserted. If it hits between bursts, BREAK stays asserted. Is that
difference intended?

The ldisc is being detached here, and a full tty close clears break during
serial shutdown, so the practical effect may be limited.

[ ... ]

> @@ -208,26 +279,24 @@ static void nci_uart_tty_close(struct tty_struct *tty)
>  {
>  	struct nci_uart *nu = tty->disc_data;
>  
> -	/* Detach from the tty */
> -	tty->disc_data = NULL;
> -
>  	if (!nu)
>  		return;
>  
> +	tty->disc_data = NULL;
> +
>  	if (test_bit(NCI_UART_DRIVER_SET, &nu->flags)) {
> -		kfree_skb(nu->tx_skb);
> -		kfree_skb(nu->rx_skb);
> +		nci_uart_rx_stop(nu);
> +		nci_uart_tx_stop(nu);
> +		nu->ops.close(nu);

[Severity: Medium]
Does this leave the NFC controller running in its last RF state after
teardown?

nci_uart_tx_stop() sets NCI_UART_CLOSING and cancels write_work before
nu->ops.close() runs. As a result, nci_uart_tx_wakeup() refuses every frame
sent during:

nu->ops.close()
    nfcmrvl_nci_unregister_dev()
        nci_unregister_device()
            nci_close_device()

This patch depends on the earlier series patch "nfc: nci: skip controller
reset while unregistering", which already skips the reset in
nci_close_device():

	if (!test_bit(NCI_UNREG, &ndev->flags)) {
		atomic_set(&ndev->cmd_cnt, 1);
		set_bit(NCI_INIT, &ndev->flags);
		__nci_request(ndev, nci_reset_req, (void *)0,
			      msecs_to_jiffies(NCI_RESET_TIMEOUT));

Before the series, write_work was only cancelled at the end of
nci_uart_tty_close(), so the CORE_RESET was still written to the UART and
only the response was lost. Now no command reaches the controller during
unregister.

The Marvell path does not halt the controller some other way either:

  - nfcmrvl_nci_close() only calls if_ops->nci_close, which does nothing
    for UART.
  - nfcmrvl_uart_nci_quiesce() only clears software state.
  - nfcmrvl_chip_halt() is only called from fw_dnld_over() on error, which
    only happens while a firmware download is in progress, and it also
    needs a reset GPIO.

So an NFCC that was polling with the RF field on, or in listen/card
emulation mode, would stay that way with no host driver until it is
re-attached (nci_open_device() sends CORE_RESET then) or power-cycled.

The commit message says:

    The preceding change skips the unregister-time controller
    reset because the TTY core cannot deliver its response after line
    discipline close starts.

That explains why the response is lost. Could the commit message also
explain why it is acceptable that the reset command itself is no longer
sent, or could the controller be quiesced some other way before TX is
shut off?

[ ... ]

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

Re: [PATCH net v6 3/6] nfc: nci: uart: quiesce callbacks during teardown

[Simon Horman]

On Sun, Oct 25, 2026 at 04:03:57AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (3) · Medium: 2 · 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).

Hi Shubham,

The text above requests a response to this email, and similar ones sent
for other patches of this patch-set. And I would appreciate it if you could
provide such responses.

FWIIW, the Medium issue immediately below is of most concern to me.

> 
> - [Medium] When nci_uart_tty_close() tears down an NCI UART whose NCI
>   device is up, the NFC controller (NFCC) is left running in whatever RF…
> - [Medium] Baseline decision: fixed later in the series.
> - [Low] Baseline decision: not a bug in practice.
> 
> Pre-existing issues:
> - [High] Pre-existing problem, not introduced by this patch: during a
>   Marvell firmware download, the UART peer (NFCC) controls a 16-bit…
> - [High] Pre-existing problem, not introduced by this patch: the Marvell
>   firmware download copies from the firmware image at a peer-advanced…
> - [High] Baseline decision: pre-existing and fixed later in the series.
> 
> --

...

Re: [PATCH net v6 3/6] nfc: nci: uart: quiesce callbacks during teardown

[Shubham Antil]

Hi Simon,

I am still looking into these issues and will keep you updated.

Regards,
Shubham Antil

On Wed, Oct 28, 2026 at 10:13 PM Simon Horman <horms@kernel.org> wrote:
>
> On Sun, Oct 25, 2026 at 04:03:57AM +0000, netdev-bot+sashiko@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 3 potential
> > issue(s) to consider.
> >
> > Critical: 0 · High: 0 (3) · Medium: 2 · 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).
>
> Hi Shubham,
>
> The text above requests a response to this email, and similar ones sent
> for other patches of this patch-set. And I would appreciate it if you could
> provide such responses.
>
> FWIIW, the Medium issue immediately below is of most concern to me.
>
> >
> > - [Medium] When nci_uart_tty_close() tears down an NCI UART whose NCI
> >   device is up, the NFC controller (NFCC) is left running in whatever RF…
> > - [Medium] Baseline decision: fixed later in the series.
> > - [Low] Baseline decision: not a bug in practice.
> >
> > Pre-existing issues:
> > - [High] Pre-existing problem, not introduced by this patch: during a
> >   Marvell firmware download, the UART peer (NFCC) controls a 16-bit…
> > - [High] Pre-existing problem, not introduced by this patch: the Marvell
> >   firmware download copies from the firmware image at a peer-advanced…
> > - [High] Baseline decision: pre-existing and fixed later in the series.
> >
> > --
>
> ...

[PATCH net v6 4/6] nfc: nfcmrvl: publish UART state before registration

[Shubham Antil]

nfcmrvl_nci_uart_open() does not publish the UART back-pointers until
nfcmrvl_nci_register_dev() returns.  The registration call exposes the NCI
device first, so an early open can reach the transmit path while those
pointers are unset.  Its response is also dropped because the TTY receive
path is not enabled yet.

Add a transport callback which publishes the Marvell UART state and
enables its TTY callbacks before nci_register_device().  Halt the
controller before that publication.  The existing quiesce callback rolls
the state back if registration fails and withdraws it during normal
teardown.

The generic UART layer still marks the transport ready after ->open()
returns.  For Marvell, repeating the publication is idempotent.  It also
retains the fallback for transports without an early publication hook.

Found during a source audit assisted by OpenAI Codex, gpt-5.6-sol, and
reproduced with a pty-backed controller that returned CORE_RESET_RSP during
device registration.

Fixes: e097dc624f78 ("NFC: nfcmrvl: add UART driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM KASAN
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 drivers/nfc/nfcmrvl/main.c    |  9 ++++++---
 drivers/nfc/nfcmrvl/nfcmrvl.h |  1 +
 drivers/nfc/nfcmrvl/uart.c    | 16 +++++++++++-----
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index baf4c781ee6b..b6f47cce0f1f 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -154,15 +154,18 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
 
 	nci_set_drvdata(priv->ndev, priv);
 
+	/* Ensure that the controller is powered off before exposing it. */
+	nfcmrvl_chip_halt(priv);
+
+	if (priv->if_ops->nci_ready)
+		priv->if_ops->nci_ready(priv);
+
 	rc = nci_register_device(priv->ndev);
 	if (rc) {
 		nfc_err(dev, "nci_register_device failed %d\n", rc);
 		goto error_fw_dnld_deinit;
 	}
 
-	/* Ensure that controller is powered off */
-	nfcmrvl_chip_halt(priv);
-
 	nfc_info(dev, "registered with nci successfully\n");
 	return priv;
 
diff --git a/drivers/nfc/nfcmrvl/nfcmrvl.h b/drivers/nfc/nfcmrvl/nfcmrvl.h
index 61b3f4a4f096..5c2a5382c280 100644
--- a/drivers/nfc/nfcmrvl/nfcmrvl.h
+++ b/drivers/nfc/nfcmrvl/nfcmrvl.h
@@ -112,6 +112,7 @@ struct nfcmrvl_if_ops {
 	int (*nci_open) (struct nfcmrvl_private *priv);
 	int (*nci_close) (struct nfcmrvl_private *priv);
 	int (*nci_send) (struct nfcmrvl_private *priv, struct sk_buff *skb);
+	void (*nci_ready)(struct nfcmrvl_private *priv);
 	void (*nci_quiesce)(struct nfcmrvl_private *priv);
 	void (*nci_update_config)(struct nfcmrvl_private *priv,
 				  const void *param);
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 66803f1c6fd3..95223c20d50f 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -44,6 +44,16 @@ static int nfcmrvl_uart_nci_send(struct nfcmrvl_private *priv,
 	return nu->ops.send(nu, skb);
 }
 
+static void nfcmrvl_uart_nci_ready(struct nfcmrvl_private *priv)
+{
+	struct nci_uart *nu = priv->drv_data;
+
+	priv->support_fw_dnld = true;
+	nu->drv_data = priv;
+	nu->ndev = priv->ndev;
+	nci_uart_set_ready(nu);
+}
+
 static void nfcmrvl_uart_nci_quiesce(struct nfcmrvl_private *priv)
 {
 	struct nci_uart *nu = priv->drv_data;
@@ -68,6 +78,7 @@ static const struct nfcmrvl_if_ops uart_ops = {
 	.nci_open = nfcmrvl_uart_nci_open,
 	.nci_close = nfcmrvl_uart_nci_close,
 	.nci_send = nfcmrvl_uart_nci_send,
+	.nci_ready = nfcmrvl_uart_nci_ready,
 	.nci_quiesce = nfcmrvl_uart_nci_quiesce,
 	.nci_update_config = nfcmrvl_uart_nci_update_config
 };
@@ -147,11 +158,6 @@ static int nfcmrvl_nci_uart_open(struct nci_uart *nu)
 	if (IS_ERR(priv))
 		return PTR_ERR(priv);
 
-	priv->support_fw_dnld = true;
-
-	nu->drv_data = priv;
-	nu->ndev = priv->ndev;
-
 	return 0;
 }
 
-- 
2.43.0

[PATCH net v6 5/6] nfc: serialize firmware download state

[Shubham Antil]

nfc_fw_download() marks a firmware download in progress but does not reject
a second request.  An overlapping request can replace driver-owned download
state, and an error from that request clears the shared in-progress flag
while the first download is still running.

Make the in-progress state atomic and claim it before invoking the driver.
An overlapping request then leaves the first download's state unchanged.
Copy the firmware name before releasing that claim, then send the
completion notification after the release so userspace can immediately
start its next operation.  Use release/acquire ordering so the next
operation observes the completed download cleanup.

OpenAI Codex, gpt-5.6-sol assisted a source audit of NFC firmware teardown.

Fixes: 9674da8759df ("NFC: Add firmware upload netlink command")
Cc: stable@vger.kernel.org
Assisted-by: LLM KASAN
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 drivers/nfc/nfcmrvl/main.c       |  4 ++--
 drivers/nfc/nfcmrvl/uart.c       |  4 ++--
 drivers/nfc/st-nci/vendor_cmds.c |  8 ++++----
 include/net/nfc/nfc.h            |  3 ++-
 net/nfc/core.c                   | 19 ++++++++++++++-----
 5 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index b6f47cce0f1f..27fadd64155e 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -189,7 +189,7 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
 	if (priv->if_ops->nci_quiesce)
 		priv->if_ops->nci_quiesce(priv);
 
-	if (priv->ndev->nfc_dev->fw_download_in_progress)
+	if (atomic_read(&priv->ndev->nfc_dev->fw_download_in_progress))
 		nfcmrvl_fw_dnld_abort(priv);
 
 	nfcmrvl_fw_dnld_deinit(priv);
@@ -213,7 +213,7 @@ int nfcmrvl_nci_recv_frame(struct nfcmrvl_private *priv, struct sk_buff *skb)
 		}
 	}
 
-	if (priv->ndev->nfc_dev->fw_download_in_progress) {
+	if (atomic_read(&priv->ndev->nfc_dev->fw_download_in_progress)) {
 		nfcmrvl_fw_dnld_recv_frame(priv, skb);
 		return 0;
 	}
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 95223c20d50f..d87f6497baf2 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -176,7 +176,7 @@ static void nfcmrvl_nci_uart_tx_start(struct nci_uart *nu)
 {
 	struct nfcmrvl_private *priv = (struct nfcmrvl_private *)nu->drv_data;
 
-	if (priv->ndev->nfc_dev->fw_download_in_progress)
+	if (atomic_read(&priv->ndev->nfc_dev->fw_download_in_progress))
 		return;
 
 	/* Remove BREAK to wake up the NFCC */
@@ -190,7 +190,7 @@ static void nfcmrvl_nci_uart_tx_done(struct nci_uart *nu)
 {
 	struct nfcmrvl_private *priv = (struct nfcmrvl_private *)nu->drv_data;
 
-	if (priv->ndev->nfc_dev->fw_download_in_progress)
+	if (atomic_read(&priv->ndev->nfc_dev->fw_download_in_progress))
 		return;
 
 	/*
diff --git a/drivers/nfc/st-nci/vendor_cmds.c b/drivers/nfc/st-nci/vendor_cmds.c
index 6335d7afca24..ec577d497500 100644
--- a/drivers/nfc/st-nci/vendor_cmds.c
+++ b/drivers/nfc/st-nci/vendor_cmds.c
@@ -158,11 +158,12 @@ static int st_nci_hci_dm_fwupd_start(struct nfc_dev *dev, void *data,
 	int r;
 	struct nci_dev *ndev = nfc_get_drvdata(dev);
 
-	dev->fw_download_in_progress = true;
+	if (atomic_cmpxchg_acquire(&dev->fw_download_in_progress, 0, 1))
+		return -EBUSY;
 	r = nci_hci_send_cmd(ndev, ST_NCI_DEVICE_MGNT_GATE,
 			ST_NCI_HCI_DM_FWUPD_START, data, data_len, NULL);
 	if (r)
-		dev->fw_download_in_progress = false;
+		atomic_set_release(&dev->fw_download_in_progress, 0);
 
 	return r;
 }
@@ -181,8 +182,7 @@ static int st_nci_hci_dm_direct_load(struct nfc_dev *dev, void *data,
 {
 	struct nci_dev *ndev = nfc_get_drvdata(dev);
 
-	if (dev->fw_download_in_progress) {
-		dev->fw_download_in_progress = false;
+	if (atomic_xchg(&dev->fw_download_in_progress, 0)) {
 		return nci_hci_send_cmd(ndev, ST_NCI_DEVICE_MGNT_GATE,
 				ST_NCI_HCI_DM_LOAD, data, data_len, NULL);
 	}
diff --git a/include/net/nfc/nfc.h b/include/net/nfc/nfc.h
index bcafab5c53e5..f6cb17f46ca7 100644
--- a/include/net/nfc/nfc.h
+++ b/include/net/nfc/nfc.h
@@ -11,6 +11,7 @@
 #ifndef __NET_NFC_H
 #define __NET_NFC_H
 
+#include <linux/atomic.h>
 #include <linux/nfc.h>
 #include <linux/device.h>
 #include <linux/skbuff.h>
@@ -172,7 +173,7 @@ struct nfc_dev {
 	int targets_generation;
 	struct device dev;
 	bool dev_up;
-	bool fw_download_in_progress;
+	atomic_t fw_download_in_progress;
 	u8 rf_mode;
 	bool polling;
 	struct nfc_target *active_target;
diff --git a/net/nfc/core.c b/net/nfc/core.c
index f521669293f0..1f335fa7f6f0 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/rfkill.h>
 #include <linux/nfc.h>
+#include <linux/string.h>
 
 #include <net/genetlink.h>
 
@@ -53,10 +54,14 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
 		goto error;
 	}
 
-	dev->fw_download_in_progress = true;
+	if (atomic_cmpxchg_acquire(&dev->fw_download_in_progress, 0, 1)) {
+		rc = -EBUSY;
+		goto error;
+	}
+
 	rc = dev->ops->fw_download(dev, firmware_name);
 	if (rc)
-		dev->fw_download_in_progress = false;
+		atomic_set_release(&dev->fw_download_in_progress, 0);
 
 error:
 	device_unlock(&dev->dev);
@@ -73,9 +78,12 @@ error:
 int nfc_fw_download_done(struct nfc_dev *dev, const char *firmware_name,
 			 u32 result)
 {
-	dev->fw_download_in_progress = false;
+	char name[NFC_FIRMWARE_NAME_MAXSIZE + 1];
+
+	strscpy(name, firmware_name, sizeof(name));
+	atomic_set_release(&dev->fw_download_in_progress, 0);
 
-	return nfc_genl_fw_download_done(dev, firmware_name, result);
+	return nfc_genl_fw_download_done(dev, name, result);
 }
 EXPORT_SYMBOL(nfc_fw_download_done);
 
@@ -104,7 +112,7 @@ int nfc_dev_up(struct nfc_dev *dev)
 		goto error;
 	}
 
-	if (dev->fw_download_in_progress) {
+	if (atomic_read_acquire(&dev->fw_download_in_progress)) {
 		rc = -EBUSY;
 		goto error;
 	}
@@ -1073,6 +1081,7 @@ struct nfc_dev *nfc_allocate_device(const struct nfc_ops *ops,
 	dev->dev.class = &nfc_class;
 	dev_set_name(&dev->dev, "nfc%d", dev->idx);
 	device_initialize(&dev->dev);
+	atomic_set(&dev->fw_download_in_progress, 0);
 
 	dev->ops = ops;
 	dev->supported_protocols = supported_protocols;
-- 
2.43.0

Re: [PATCH net v6 5/6] nfc: serialize firmware download state

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · 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] At this commit nfc_fw_download_done() releases the shared
  fw_download_in_progress claim unconditionally, with no owner or…
- [Medium] The patch changes how the st-nci vendor command
  ST_NCI_HCI_DM_FWUPD_START behaves, and the commit message does not…
- [Medium] nfc_fw_download() now claims fw_download_in_progress with
  atomic_cmpxchg_acquire(0->1) and rejects every later request with…

--

nfc: serialize firmware download state

This patch turns nfc_dev->fw_download_in_progress into an atomic_t. Both
nfc_fw_download() and the st-nci FWUPD_START vendor command now claim it
with cmpxchg and return -EBUSY if a download is already in progress.
nfc_fw_download_done() copies the firmware name, then releases the claim
before sending the netlink completion notification.

> diff --git a/drivers/nfc/st-nci/vendor_cmds.c b/drivers/nfc/st-nci/vendor_cmds.c
> index 6335d7afca24f..ec577d497500c 100644
> --- a/drivers/nfc/st-nci/vendor_cmds.c
> +++ b/drivers/nfc/st-nci/vendor_cmds.c
> @@ -158,11 +158,12 @@ static int st_nci_hci_dm_fwupd_start(struct nfc_dev *dev, void *data,
>  	int r;
>  	struct nci_dev *ndev = nfc_get_drvdata(dev);
>  
> -	dev->fw_download_in_progress = true;
> +	if (atomic_cmpxchg_acquire(&dev->fw_download_in_progress, 0, 1))
> +		return -EBUSY;
>  	r = nci_hci_send_cmd(ndev, ST_NCI_DEVICE_MGNT_GATE,
>  			ST_NCI_HCI_DM_FWUPD_START, data, data_len, NULL);
>  	if (r)
> -		dev->fw_download_in_progress = false;
> +		atomic_set_release(&dev->fw_download_in_progress, 0);
>  
>  	return r;
>  }

[Severity: Medium]
This changes what the ST_NCI_HCI_DM_FWUPD_START vendor command does from
userspace, and the commit message doesn't mention it. Is that intended?

In st-nci the flag is set only by FWUPD_START. The only place it gets
cleared is st_nci_hci_dm_direct_load(). FWUPD_STOP leaves it alone:

drivers/nfc/st-nci/vendor_cmds.c:st_nci_hci_dm_fwupd_end() {
	...
	return nci_hci_send_cmd(ndev, ST_NCI_DEVICE_MGNT_GATE,
			ST_NCI_HCI_DM_FWUPD_STOP, data, data_len, NULL);
}

st_nci_hci_dm_reset() and the close/teardown paths don't clear it either.

Suppose a session is abandoned after a successful FWUPD_START: the tool
crashes, or it sends START then STOP, or START then RESET. Every later
FWUPD_START then returns -EBUSY. Before this patch, START was simply sent
again.

The only ways out seem to be sending DIRECT_LOAD, which sends
ST_NCI_HCI_DM_LOAD with user data to a chip that may already have left
update mode, or rebinding the driver.

There is a second change userspace can see. For NCI devices
dev->ops->fw_download is always nci_fw_download(). So while a vendor
START is pending, the new cmpxchg in nfc_fw_download() makes
NFC_CMD_FW_DOWNLOAD return -EBUSY on st-nci, where it used to return
-ENOTSUPP. Is that intended, given st-nci has no core firmware download
state to protect?

[ ... ]

> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index f521669293f05..1f335fa7f6f07 100644
> --- a/net/nfc/core.c
> +++ b/net/nfc/core.c

[ ... ]

> @@ -53,10 +54,14 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
>  		goto error;
>  	}
>  
> -	dev->fw_download_in_progress = true;
> +	if (atomic_cmpxchg_acquire(&dev->fw_download_in_progress, 0, 1)) {
> +		rc = -EBUSY;
> +		goto error;
> +	}
> +

[Severity: Medium]
Can this leave a pn544 device stuck in -EBUSY for good?

pn544_hci_i2c_fw_download() returns 0 as soon as it has scheduled the
work:

drivers/nfc/pn544/i2c.c:pn544_hci_i2c_fw_download() {
	...
	phy->fw_work_state = FW_WORK_STATE_START;

	schedule_work(&phy->fw_work);

	return 0;
}

pn544_hci_i2c_fw_work() then waits in FW_WORK_STATE_WAIT_WRITE_ANSWER,
WAIT_CHECK_ANSWER or WAIT_SECURE_WRITE_ANSWER. It moves on only when the
IRQ handler reschedules the work, which needs run_mode == PN544_FW_MODE
and an interrupt from the chip. drivers/nfc/pn544/ has no timer or
timeout for these states.

So if the controller never answers (hung chip, I2C fault, bad image),
nfc_fw_download_done() is never called. After that:

  - every NFC_CMD_FW_DOWNLOAD fails with -EBUSY from this cmpxchg
  - nfc_dev_up() fails with -EBUSY from atomic_read_acquire()

This lasts until the i2c driver is unbound, which completes with -ENODEV
in pn544_hci_i2c_remove(), or until a reboot.

Before this patch, a second NFC_CMD_FW_DOWNLOAD reset fw_work_state to
START and rescheduled the work, so userspace could recover. Should there
be a timeout or a core abort path before this recovery is removed?

>  	rc = dev->ops->fw_download(dev, firmware_name);
>  	if (rc)
> -		dev->fw_download_in_progress = false;
> +		atomic_set_release(&dev->fw_download_in_progress, 0);
>  
>  error:
>  	device_unlock(&dev->dev);
> @@ -73,9 +78,12 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
>  int nfc_fw_download_done(struct nfc_dev *dev, const char *firmware_name,
>  			 u32 result)
>  {
> -	dev->fw_download_in_progress = false;
> +	char name[NFC_FIRMWARE_NAME_MAXSIZE + 1];
> +
> +	strscpy(name, firmware_name, sizeof(name));
> +	atomic_set_release(&dev->fw_download_in_progress, 0);

[Severity: High]
Can a second completion of the same download clear the claim of a newer
download here?

nfc_fw_download_done() releases the claim unconditionally. There is no
owner or generation check. At this commit nfcmrvl can finish one
download twice: fw_dnld_timeout() runs in timer context, and both it and
fw_dnld_rx_work() can call fw_dnld_over().

fw_dnld_over() syncs the timer only if it is still pending:

drivers/nfc/nfcmrvl/fw_dnld.c:fw_dnld_over() {
	...
	if (timer_pending(&priv->fw_dnld.timer))
		timer_delete_sync(&priv->fw_dnld.timer);
	...
}

So it doesn't wait for a timeout callback that is already running. Also,
the rx_work loop keeps dequeuing frames after fw_dnld_over(0).

A second completion can then:

  - call release_firmware() again on the same firmware, or on a newer one
  - delete a newer download's timer
  - halt the chip
  - clear the newer download's fw_download_in_progress claim

Later in the same series, the commit "nfc: nfcmrvl: stop firmware work
before unregistering" seems to fix this. There, fw_dnld_over() claims
completion through fw_dnld->active under rx_lock, calls
timer_delete_sync() unconditionally, and runs the timeout as
timeout_work on rx_wq. At this commit, though, the window is still
open.

>  
> -	return nfc_genl_fw_download_done(dev, firmware_name, result);
> +	return nfc_genl_fw_download_done(dev, name, result);
>  }
>  EXPORT_SYMBOL(nfc_fw_download_done);

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

[PATCH net v6 6/6] nfc: nfcmrvl: stop firmware work before unregistering

[Shubham Antil]

nfcmrvl_nci_unregister_dev() destroys the NCI workqueues before draining
the firmware receive worker.  That worker can still submit NCI commands,
while the later abort can release firmware state that it is processing.
The firmware timeout can also complete the same download in parallel with
the receive worker.

Add active and stopping states protected by the firmware receive lock.
Once completion or teardown starts, reject new firmware receives and wait
for active receive entry points.  Shut down the firmware timer, cancel the
receive worker, and purge its queue before releasing firmware state.
Perform this shutdown before unregistering the NCI device so its
command workqueues remain available until the firmware worker has finished.
Queue timeout completion on the same single-thread workqueue and claim
completion under the receive lock so download state is released once.  Use
a local copy of the firmware name while reporting completion so a following
request cannot change the notification.  Unregister the NCI device before
withdrawing the UART back-pointers so core admission is closed first.
Process one receive frame per work invocation so queued timeout work cannot
be starved by a continuously replenished receive queue.

Initialize the firmware timer with the receive workqueue so the shutdown
path is valid even when a download was never started.  Serialize the
stopping transition with firmware start through the NFC device lock.
Use non-synchronous timeout-work cancellation because completion can run
from the timeout work itself.

OpenAI Codex, gpt-5.6-sol assisted a source audit of the UART teardown
path.

Fixes: 3194c6870158 ("NFC: nfcmrvl: add firmware download support")
Cc: stable@vger.kernel.org
Assisted-by: LLM KASAN
Signed-off-by: Shubham Antil <shubham@octane.security>
---
 drivers/nfc/nfcmrvl/fw_dnld.c | 199 +++++++++++++++++++++++++---------
 drivers/nfc/nfcmrvl/fw_dnld.h |   8 ++
 drivers/nfc/nfcmrvl/main.c    |   5 +-
 3 files changed, 156 insertions(+), 56 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/fw_dnld.c b/drivers/nfc/nfcmrvl/fw_dnld.c
index 8b9d5257320d..d3dec74a333d 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.c
+++ b/drivers/nfc/nfcmrvl/fw_dnld.c
@@ -92,11 +92,29 @@ static struct sk_buff *alloc_lc_skb(struct nfcmrvl_private *priv, uint8_t plen)
 
 static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
 {
-	if (priv->fw_dnld.fw) {
-		release_firmware(priv->fw_dnld.fw);
-		priv->fw_dnld.fw = NULL;
-		priv->fw_dnld.header = NULL;
-		priv->fw_dnld.binary_config = NULL;
+	struct nfcmrvl_fw_dnld *fw_dnld = &priv->fw_dnld;
+	char name[NFC_FIRMWARE_NAME_MAXSIZE + 1];
+	unsigned long flags;
+
+	spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+	if (!fw_dnld->active) {
+		spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+		return;
+	}
+	fw_dnld->active = false;
+	spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+
+	strscpy(name, fw_dnld->name);
+	wait_event(fw_dnld->rx_wait, !READ_ONCE(fw_dnld->rx_active));
+	timer_delete_sync(&fw_dnld->timer);
+	/* Timeout and receive work share a single-thread workqueue. */
+	cancel_work(&fw_dnld->timeout_work);
+
+	if (fw_dnld->fw) {
+		release_firmware(fw_dnld->fw);
+		fw_dnld->fw = NULL;
+		fw_dnld->header = NULL;
+		fw_dnld->binary_config = NULL;
 	}
 
 	atomic_set(&priv->ndev->cmd_cnt, 0);
@@ -104,8 +122,7 @@ static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
 	if (timer_pending(&priv->ndev->cmd_timer))
 		timer_delete_sync(&priv->ndev->cmd_timer);
 
-	if (timer_pending(&priv->fw_dnld.timer))
-		timer_delete_sync(&priv->fw_dnld.timer);
+	skb_queue_purge(&fw_dnld->rx_q);
 
 	nfc_info(priv->dev, "FW loading over (%d)]\n", error);
 
@@ -114,7 +131,7 @@ static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
 		nfcmrvl_chip_halt(priv);
 	}
 
-	nfc_fw_download_done(priv->ndev->nfc_dev, priv->fw_dnld.name, error);
+	nfc_fw_download_done(priv->ndev->nfc_dev, name, error);
 }
 
 static void fw_dnld_timeout(struct timer_list *t)
@@ -122,6 +139,18 @@ static void fw_dnld_timeout(struct timer_list *t)
 	struct nfcmrvl_private *priv = timer_container_of(priv, t,
 							  fw_dnld.timer);
 
+	queue_work(priv->fw_dnld.rx_wq, &priv->fw_dnld.timeout_work);
+}
+
+static void fw_dnld_timeout_work(struct work_struct *work)
+{
+	struct nfcmrvl_fw_dnld *fw_dnld = container_of(work,
+						       struct nfcmrvl_fw_dnld,
+						       timeout_work);
+	struct nfcmrvl_private *priv = container_of(fw_dnld,
+						    struct nfcmrvl_private,
+						    fw_dnld);
+
 	nfc_err(priv->dev, "FW loading timeout");
 	priv->fw_dnld.state = STATE_RESET;
 	fw_dnld_over(priv, -ETIMEDOUT);
@@ -395,6 +424,7 @@ static int process_state_boot(struct nfcmrvl_private *priv,
 
 static void fw_dnld_rx_work(struct work_struct *work)
 {
+	unsigned long flags;
 	int ret;
 	struct sk_buff *skb;
 	struct nfcmrvl_fw_dnld *fw_dnld = container_of(work,
@@ -404,53 +434,64 @@ static void fw_dnld_rx_work(struct work_struct *work)
 						    struct nfcmrvl_private,
 						    fw_dnld);
 
-	while ((skb = skb_dequeue(&fw_dnld->rx_q))) {
-		nfc_send_to_raw_sock(priv->ndev->nfc_dev, skb,
-				     RAW_PAYLOAD_NCI, NFC_DIRECTION_RX);
-		switch (fw_dnld->state) {
-		case STATE_RESET:
-			ret = process_state_reset(priv, skb);
-			break;
-		case STATE_INIT:
-			ret = process_state_init(priv, skb);
-			break;
-		case STATE_SET_REF_CLOCK:
-			ret = process_state_set_ref_clock(priv, skb);
-			break;
-		case STATE_SET_HI_CONFIG:
-			ret = process_state_set_hi_config(priv, skb);
-			break;
-		case STATE_OPEN_LC:
-			ret = process_state_open_lc(priv, skb);
-			break;
-		case STATE_FW_DNLD:
-			ret = process_state_fw_dnld(priv, skb);
-			break;
-		case STATE_CLOSE_LC:
-			ret = process_state_close_lc(priv, skb);
-			break;
-		case STATE_BOOT:
-			ret = process_state_boot(priv, skb);
-			break;
-		default:
-			ret = -EFAULT;
-		}
+	skb = skb_dequeue(&fw_dnld->rx_q);
+	if (!skb)
+		return;
 
-		kfree_skb(skb);
+	nfc_send_to_raw_sock(priv->ndev->nfc_dev, skb,
+			     RAW_PAYLOAD_NCI, NFC_DIRECTION_RX);
+	switch (fw_dnld->state) {
+	case STATE_RESET:
+		ret = process_state_reset(priv, skb);
+		break;
+	case STATE_INIT:
+		ret = process_state_init(priv, skb);
+		break;
+	case STATE_SET_REF_CLOCK:
+		ret = process_state_set_ref_clock(priv, skb);
+		break;
+	case STATE_SET_HI_CONFIG:
+		ret = process_state_set_hi_config(priv, skb);
+		break;
+	case STATE_OPEN_LC:
+		ret = process_state_open_lc(priv, skb);
+		break;
+	case STATE_FW_DNLD:
+		ret = process_state_fw_dnld(priv, skb);
+		break;
+	case STATE_CLOSE_LC:
+		ret = process_state_close_lc(priv, skb);
+		break;
+	case STATE_BOOT:
+		ret = process_state_boot(priv, skb);
+		break;
+	default:
+		ret = -EFAULT;
+	}
 
-		if (ret != 0) {
-			nfc_err(priv->dev, "FW loading error");
-			fw_dnld_over(priv, ret);
-			break;
-		}
+	kfree_skb(skb);
+
+	if (ret) {
+		nfc_err(priv->dev, "FW loading error");
+		fw_dnld_over(priv, ret);
 	}
+
+	spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+	if (!fw_dnld->stopping && fw_dnld->active &&
+	    !skb_queue_empty(&fw_dnld->rx_q))
+		queue_work(fw_dnld->rx_wq, &fw_dnld->rx_work);
+	spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
 }
 
 int nfcmrvl_fw_dnld_init(struct nfcmrvl_private *priv)
 {
 	char name[32];
 
+	spin_lock_init(&priv->fw_dnld.rx_lock);
+	init_waitqueue_head(&priv->fw_dnld.rx_wait);
 	INIT_WORK(&priv->fw_dnld.rx_work, fw_dnld_rx_work);
+	INIT_WORK(&priv->fw_dnld.timeout_work, fw_dnld_timeout_work);
+	timer_setup(&priv->fw_dnld.timer, fw_dnld_timeout, 0);
 	snprintf(name, sizeof(name), "%s_nfcmrvl_fw_dnld_rx_wq",
 		 dev_name(&priv->ndev->nfc_dev->dev));
 	priv->fw_dnld.rx_wq = create_singlethread_workqueue(name);
@@ -462,12 +503,24 @@ int nfcmrvl_fw_dnld_init(struct nfcmrvl_private *priv)
 
 void nfcmrvl_fw_dnld_deinit(struct nfcmrvl_private *priv)
 {
+	nfcmrvl_fw_dnld_abort(priv);
 	destroy_workqueue(priv->fw_dnld.rx_wq);
 }
 
 void nfcmrvl_fw_dnld_recv_frame(struct nfcmrvl_private *priv,
 				struct sk_buff *skb)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->fw_dnld.rx_lock, flags);
+	if (priv->fw_dnld.stopping || !priv->fw_dnld.active) {
+		spin_unlock_irqrestore(&priv->fw_dnld.rx_lock, flags);
+		kfree_skb(skb);
+		return;
+	}
+	priv->fw_dnld.rx_active++;
+	spin_unlock_irqrestore(&priv->fw_dnld.rx_lock, flags);
+
 	/* Discard command timer */
 	if (timer_pending(&priv->ndev->cmd_timer))
 		timer_delete_sync(&priv->ndev->cmd_timer);
@@ -475,13 +528,38 @@ void nfcmrvl_fw_dnld_recv_frame(struct nfcmrvl_private *priv,
 	/* Allow next command */
 	atomic_set(&priv->ndev->cmd_cnt, 1);
 
-	/* Queue and trigger rx work */
-	skb_queue_tail(&priv->fw_dnld.rx_q, skb);
-	queue_work(priv->fw_dnld.rx_wq, &priv->fw_dnld.rx_work);
+	spin_lock_irqsave(&priv->fw_dnld.rx_lock, flags);
+	if (!priv->fw_dnld.stopping && priv->fw_dnld.active) {
+		/* Queue and trigger rx work */
+		skb_queue_tail(&priv->fw_dnld.rx_q, skb);
+		queue_work(priv->fw_dnld.rx_wq, &priv->fw_dnld.rx_work);
+		skb = NULL;
+	}
+	priv->fw_dnld.rx_active--;
+	if (!priv->fw_dnld.rx_active &&
+	    (priv->fw_dnld.stopping || !priv->fw_dnld.active))
+		wake_up_all(&priv->fw_dnld.rx_wait);
+	spin_unlock_irqrestore(&priv->fw_dnld.rx_lock, flags);
+	kfree_skb(skb);
 }
 
 void nfcmrvl_fw_dnld_abort(struct nfcmrvl_private *priv)
 {
+	struct nfcmrvl_fw_dnld *fw_dnld = &priv->fw_dnld;
+	unsigned long flags;
+
+	device_lock(&priv->ndev->nfc_dev->dev);
+	spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+	fw_dnld->stopping = true;
+	spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+	device_unlock(&priv->ndev->nfc_dev->dev);
+
+	wait_event(fw_dnld->rx_wait, !READ_ONCE(fw_dnld->rx_active));
+	timer_shutdown_sync(&fw_dnld->timer);
+	cancel_work_sync(&fw_dnld->rx_work);
+	cancel_work_sync(&fw_dnld->timeout_work);
+	skb_queue_purge(&fw_dnld->rx_q);
+
 	fw_dnld_over(priv, -EHOSTDOWN);
 }
 
@@ -489,6 +567,7 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
 {
 	struct nfcmrvl_private *priv = nci_get_drvdata(ndev);
 	struct nfcmrvl_fw_dnld *fw_dnld = &priv->fw_dnld;
+	unsigned long flags;
 	int res;
 
 	if (!priv->support_fw_dnld)
@@ -497,6 +576,13 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
 	if (!firmware_name || !firmware_name[0])
 		return -EINVAL;
 
+	spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+	if (fw_dnld->stopping) {
+		spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+		return -ENODEV;
+	}
+	spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+
 	strscpy(fw_dnld->name, firmware_name);
 
 	/*
@@ -520,6 +606,7 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
 			firmware_name, fw_dnld->header->magic,
 			fw_dnld->header->phy);
 		release_firmware(fw_dnld->fw);
+		fw_dnld->fw = NULL;
 		fw_dnld->header = NULL;
 		return -EINVAL;
 	}
@@ -532,11 +619,6 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
 		fw_dnld->binary_config = &fw_dnld->header->firmware;
 	}
 
-	/* Configure a timer for timeout */
-	timer_setup(&priv->fw_dnld.timer, fw_dnld_timeout, 0);
-	mod_timer(&priv->fw_dnld.timer,
-		  jiffies + msecs_to_jiffies(FW_DNLD_TIMEOUT));
-
 	/* Ronfigure HI to be sure that it is the bootrom values */
 	priv->if_ops->nci_update_config(priv,
 					&fw_dnld->header->bootrom.config);
@@ -546,8 +628,19 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
 
 	/* First, reset the chip */
 	priv->fw_dnld.state = STATE_RESET;
+
+	spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+	fw_dnld->active = true;
+	spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+
 	nfcmrvl_chip_reset(priv);
 
+	spin_lock_irqsave(&fw_dnld->rx_lock, flags);
+	if (fw_dnld->active)
+		mod_timer(&fw_dnld->timer,
+			  jiffies + msecs_to_jiffies(FW_DNLD_TIMEOUT));
+	spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
+
 	/* Now wait for CORE_RESET_NTF or timeout */
 
 	return 0;
diff --git a/drivers/nfc/nfcmrvl/fw_dnld.h b/drivers/nfc/nfcmrvl/fw_dnld.h
index 7c4d91b01910..6bd8d580cd6a 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.h
+++ b/drivers/nfc/nfcmrvl/fw_dnld.h
@@ -8,6 +8,8 @@
 #ifndef __NFCMRVL_FW_DNLD_H__
 #define __NFCMRVL_FW_DNLD_H__
 
+#include <linux/spinlock.h>
+#include <linux/wait.h>
 #include <linux/workqueue.h>
 
 #define NFCMRVL_FW_MAGIC		0x88888888
@@ -72,7 +74,13 @@ struct nfcmrvl_fw_dnld {
 
 	struct workqueue_struct	*rx_wq;
 	struct work_struct rx_work;
+	struct work_struct timeout_work;
 	struct sk_buff_head rx_q;
+	spinlock_t rx_lock; /* protects stopping, active, and rx_active */
+	wait_queue_head_t rx_wait;
+	unsigned int rx_active;
+	bool stopping;
+	bool active;
 
 	struct timer_list timer;
 };
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 27fadd64155e..86cf65396253 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -185,13 +185,12 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
 {
 	struct nci_dev *ndev = priv->ndev;
 
+	nfcmrvl_fw_dnld_abort(priv);
 	nci_unregister_device(ndev);
+
 	if (priv->if_ops->nci_quiesce)
 		priv->if_ops->nci_quiesce(priv);
 
-	if (atomic_read(&priv->ndev->nfc_dev->fw_download_in_progress))
-		nfcmrvl_fw_dnld_abort(priv);
-
 	nfcmrvl_fw_dnld_deinit(priv);
 
 	nci_free_device(ndev);
-- 
2.43.0

Re: [PATCH net v6 6/6] nfc: nfcmrvl: stop firmware work before unregistering

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 1 (2) · 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] Teardown can leave ndev->cmd_timer armed after the NCI device is
  freed.
- [Medium] The new order (nfcmrvl_fw_dnld_abort() before
  nci_unregister_device()) can leak an NCI command skb.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  process_state_fw_dnld(), which the reworked fw_dnld_rx_work() still…
- [Medium] This problem wasn't introduced by this patch, but nfcmrvl
  reports firmware download failures to the NFC core as negative errno…
- [Medium] This problem wasn't introduced by this patch, but
  nfcmrvl_fw_dnld_start() sets fw_dnld->active = true and then calls…

--

nfc: nfcmrvl: stop firmware work before unregistering

This commit adds active and stopping states, protected by a new rx_lock,
to fence the firmware receive entry points. It moves timeout completion
into a work item on the firmware workqueue and processes one rx frame per
work invocation. It also calls nfcmrvl_fw_dnld_abort() before
nci_unregister_device(), so firmware work is drained while the NCI
workqueues still exist.

> diff --git a/drivers/nfc/nfcmrvl/fw_dnld.c b/drivers/nfc/nfcmrvl/fw_dnld.c
> index 8b9d5257320dc..d3dec74a333d3 100644
> --- a/drivers/nfc/nfcmrvl/fw_dnld.c
> +++ b/drivers/nfc/nfcmrvl/fw_dnld.c
> @@ -92,11 +92,29 @@ static struct sk_buff *alloc_lc_skb(struct nfcmrvl_private *priv, uint8_t plen)
>  
>  static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
>  {

[ ... ]

> +	if (fw_dnld->fw) {
> +		release_firmware(fw_dnld->fw);
> +		fw_dnld->fw = NULL;
> +		fw_dnld->header = NULL;
> +		fw_dnld->binary_config = NULL;
>  	}
>  
>  	atomic_set(&priv->ndev->cmd_cnt, 0);
> @@ -104,8 +122,7 @@ static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
>  	if (timer_pending(&priv->ndev->cmd_timer))
>  		timer_delete_sync(&priv->ndev->cmd_timer);
>  

[Severity: High]
Can ndev->cmd_timer still be armed when nci_free_device() frees ndev?

Suppose fw_dnld_rx_work() has queued two commands back to back. Then
nci_cmd_work() has sent the first one (cmd_cnt is 0 and cmd_timer is
armed), and the second skb is still on ndev->cmd_q.

If nci_cmd_timer() has already been detached for expiry when
fw_dnld_over() runs, timer_pending() returns false and the sync delete is
skipped:

fw_dnld_over()
    atomic_set(&priv->ndev->cmd_cnt, 0);
    if (timer_pending(&priv->ndev->cmd_timer))   /* false */

nci_cmd_timer()   /* delayed callback */
    atomic_set(&ndev->cmd_cnt, 1);
    queue_work(ndev->cmd_wq, &ndev->cmd_work);

Suppose that queue_work() happens after the flush in the !NCI_UP branch
of nci_close_device():

nci_close_device()
    flush_workqueue(ndev->cmd_wq);
    timer_delete_sync(&ndev->cmd_timer);   /* waits only for the callback */

nci_cmd_work()
    skb = skb_dequeue(&ndev->cmd_q);
    ...
    nci_send_frame(ndev, skb);
    mod_timer(&ndev->cmd_timer,
              jiffies + msecs_to_jiffies(NCI_CMD_TIMEOUT));

destroy_workqueue(ndev->cmd_wq) waits for cmd_work, but it does not
cancel the timer.

Before this patch, nfcmrvl_fw_dnld_abort() ran after
nci_unregister_device(). The timer_pending()/timer_delete_sync() pair in
fw_dnld_over() then removed a re-armed cmd_timer. With this patch, the
second abort from nfcmrvl_fw_dnld_deinit() returns early because active
is already false, so nothing deletes the timer.

Would the timer then fire on the kfree()d ndev, calling atomic_set() on
ndev->cmd_cnt and queue_work() on the destroyed cmd_wq? The window is
narrow, but nothing seems to prevent it, for example with PREEMPT_RT timer
threads or vCPU preemption.

Should the shutdown also purge leftover firmware commands on ndev->cmd_q,
or delete cmd_timer unconditionally?

[ ... ]

> @@ -114,7 +131,7 @@ static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
>  		nfcmrvl_chip_halt(priv);
>  	}
>  
> -	nfc_fw_download_done(priv->ndev->nfc_dev, priv->fw_dnld.name, error);
> +	nfc_fw_download_done(priv->ndev->nfc_dev, name, error);

[Severity: Medium]
This isn't a bug introduced by this patch, but is the sign of error
right here?

The kernel-doc for nfc_fw_download_done() describes @result as "The
positive value of a standard errno value". nfc_genl_fw_download_done()
then passes it unchanged to userspace via nla_put_u32() in
NFC_ATTR_FIRMWARE_DOWNLOAD_STATUS.

fw_dnld_over() gets -ETIMEDOUT from fw_dnld_timeout_work(), -EHOSTDOWN
from nfcmrvl_fw_dnld_abort(), and negative process_state_*() return
values from fw_dnld_rx_work(). Userspace therefore sees 0xFFFFFF92 or
0xFFFFFF90 instead of 110 or 112. The "%d" in the nfc_info() message
hides this in the log.

Other drivers negate the value first, for example
nxp_nci_fw_work_complete():

    nfc_fw_download_done(info->ndev->nfc_dev, fw_info->name, (u32) -result);

pn544_hci_i2c_fw_work_complete() does the same.

This patch rewrites this call and adds new paths into fw_dnld_over().
Could it pass -error here?

[ ... ]

> @@ -404,53 +434,64 @@ static void fw_dnld_rx_work(struct work_struct *work)

[ ... ]

> +	case STATE_FW_DNLD:
> +		ret = process_state_fw_dnld(priv, skb);
> +		break;

[Severity: High]
This is a pre-existing issue, but process_state_fw_dnld(), which still
gets its frames from here, trusts the 16-bit chunk length from the
controller. In SUBSTATE_WAIT_COMMAND, the only check is the complement:

    len = get_unaligned_le16(skb->data);
    ...
    if (((~len) & 0xFFFF) != comp_len) {
    ...
    priv->fw_dnld.chunk_len = len;

Then in SUBSTATE_WAIT_ACK_CREDIT:

    out_skb = alloc_lc_skb(priv, priv->fw_dnld.chunk_len);
    ...
    skb_put_data(out_skb,
                 ((uint8_t *)priv->fw_dnld.fw->data) + priv->fw_dnld.offset,
                 priv->fw_dnld.chunk_len);

alloc_lc_skb() takes uint8_t plen. With len = 0xFF00 and complement
0x00FF, the skb is sized for NCI_DATA_HDR_SIZE + 0. Would skb_put_data()
with the full 0xFF00 then hit skb_over_panic()?

SUBSTATE_WAIT_DATA_CREDIT also does this with no check against fw->size:

    priv->fw_dnld.offset += priv->fw_dnld.chunk_len;

Could a controller that keeps repeating command/credit cycles with
chunk_len <= 255 make skb_put_data() read past the end of the firmware
image, and send that memory back to the device?

The I2C, SPI and UART PHYs set support_fw_dnld, so they can reach this.

[ ... ]

> @@ -546,8 +628,19 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
>  
>  	/* First, reset the chip */
>  	priv->fw_dnld.state = STATE_RESET;
> +
> +	spin_lock_irqsave(&fw_dnld->rx_lock, flags);
> +	fw_dnld->active = true;
> +	spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);
> +
>  	nfcmrvl_chip_reset(priv);
>  
> +	spin_lock_irqsave(&fw_dnld->rx_lock, flags);
> +	if (fw_dnld->active)
> +		mod_timer(&fw_dnld->timer,
> +			  jiffies + msecs_to_jiffies(FW_DNLD_TIMEOUT));
> +	spin_unlock_irqrestore(&fw_dnld->rx_lock, flags);

[Severity: Medium]
This is a pre-existing issue, but what happens to the reset line if the
download fails while nfcmrvl_chip_reset() is sleeping?

active is set before the reset pulse, so nfcmrvl_fw_dnld_recv_frame()
accepts frames during the usleep_range(5000, 10000) in
nfcmrvl_chip_reset(). A frame that isn't CORE_RESET_NTF makes
process_state_reset() return -EINVAL. That could be stale transport data,
or a misbehaving or pty-backed controller:

fw_dnld_rx_work()
    process_state_reset()        /* -EINVAL */
    fw_dnld_over()
        nfcmrvl_chip_halt()      /* reset_gpio = 1 */
        nfc_fw_download_done()

nfcmrvl_fw_dnld_start() resumes
    nfcmrvl_chip_reset()
        gpiod_set_value(priv->config.reset_gpio, 0);

Doesn't this take the chip back out of reset after userspace has already
been told the download failed? The chip's CORE_RESET_NTF would then be
dropped, because fw_download_in_progress is clear.

The new "if (fw_dnld->active)" check before mod_timer() covers completion
during the reset for the timer, but not for the reset GPIO.

[ ... ]

> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index 27fadd64155e1..86cf653962535 100644
> --- a/drivers/nfc/nfcmrvl/main.c
> +++ b/drivers/nfc/nfcmrvl/main.c
> @@ -185,13 +185,12 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
>  {
>  	struct nci_dev *ndev = priv->ndev;
>  
> +	nfcmrvl_fw_dnld_abort(priv);
>  	nci_unregister_device(ndev);

[Severity: Medium]
Can this new ordering leak an NCI command skb left on ndev->cmd_q?

fw_dnld_rx_work()->process_state_*()->nci_send_cmd() puts an skb on
ndev->cmd_q and queues ndev->cmd_work on ndev->cmd_wq.
cancel_work_sync(&fw_dnld->rx_work) in nfcmrvl_fw_dnld_abort() does not
wait for that cmd_work. fw_dnld_over() then does:

    atomic_set(&priv->ndev->cmd_cnt, 0);

If cmd_work runs after that, nci_cmd_work() sees cmd_cnt == 0 and leaves
the skb on the queue:

    if (atomic_read(&ndev->cmd_cnt)) {
        skb = skb_dequeue(&ndev->cmd_q);

NCI_UP is clear during a firmware download, so nci_close_device() takes
the !NCI_UP branch. That branch only flushes cmd_wq and deletes the
timers, without purging cmd_q. nci_free_device() doesn't purge cmd_q
either, so the skb is lost when ndev is freed.

Before this patch, the abort ran after nci_unregister_device(). The
cmd_wq flush then ran with cmd_cnt still 1 from
nfcmrvl_fw_dnld_recv_frame(), so the queued command was consumed.

[ ... ]

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