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

[netdev-bot+sashiko@kernel.org]

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