Re: [PATCH net v5 1/2] nfc: nfcmrvl: publish UART state before registration

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The patch sets the TX-side UART state (nu->drv_data, nu->ndev)
  before nci_register_device(), but the RX side is still attached late.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  leaves it in place: the SPI transport has the same publish-after-…
- [High] Concern: on the normal teardown paths, nu->drv_data and nu->ndev
  are never withdrawn.
- [High] This problem wasn't introduced by this patch, but the USB
  transport has the same publish-after-registration bug, and it crashes…

--

nfc: nfcmrvl: publish UART state before registration

This commit adds nci_ready() and nci_unready() callbacks to
nfcmrvl_if_ops. nfcmrvl_nci_register_dev() calls nci_ready() before
nci_register_device(), so the UART transport now sets nu->drv_data,
nu->ndev and priv->support_fw_dnld before the NCI device is exposed. The
same function calls nci_unready() if registration fails, and it now calls
nfcmrvl_chip_halt() before registration.

> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index 6efa832191172..3d26a54ef51b4 100644
> --- a/drivers/nfc/nfcmrvl/main.c
> +++ b/drivers/nfc/nfcmrvl/main.c
> @@ -154,20 +154,25 @@ struct nfcmrvl_private *nfcmrvl_nci_register_dev(enum nfcmrvl_phy phy,
>  
>  	nci_set_drvdata(priv->ndev, priv);
>  
> +	/* Ensure that controller is powered off before exposing the device. */
> +	nfcmrvl_chip_halt(priv);
> +
> +	if (priv->if_ops->nci_ready)
> +		priv->if_ops->nci_ready(priv);
> +
>  	rc = nci_register_device(priv->ndev);

[Severity: High]
This is a pre-existing issue and this patch didn't introduce it. The SPI
transport seems to have the same publish-after-registration problem. spi_ops
in drivers/nfc/nfcmrvl/spi.c has no .nci_ready, and nfcmrvl_spi_probe()
still sets its state only after registration returns:

drivers/nfc/nfcmrvl/spi.c:nfcmrvl_spi_probe() {
    ...
	drv_data->priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_SPI,
						  drv_data, &spi_ops,
						  &drv_data->spi->dev,
						  pdata);
    ...
	drv_data->priv->support_fw_dnld = true;

	drv_data->nci_spi = nci_spi_allocate_spi(drv_data->spi, 0, 10,
						 drv_data->priv->ndev);
    ...
	init_completion(&drv_data->handshake_completion);
}

nfc_register_device() calls device_add() before it takes device_lock(). If
an NFC_CMD_DEV_UP arrives in that window, can this path reach
nci_spi_send() with drv_data->nci_spi still NULL?

nfc_dev_up()->nci_dev_up()->nci_open_device()->CORE_RESET->nci_cmd_work()
  ->nfcmrvl_nci_send()->nfcmrvl_spi_nci_send()
    ->nci_spi_send(drv_data->nci_spi == NULL, ...)
      hdr[1] = nspi->acknowledge_mode;

The SPI IRQ is also requested before registration. If SPI_INT falls before
probe finishes, nfcmrvl_spi_int_irq_thread_fn() would call
nci_spi_read(drv_data->nci_spi) and nfcmrvl_nci_recv_frame(drv_data->priv,
skb) while both pointers are still NULL.

The I2C transport also sets support_fw_dnld only after registration, in
nfcmrvl_i2c_probe().

Should SPI implement the new nci_ready hook as well?

[Severity: High]
This is also a pre-existing issue, not one introduced by this patch. The
USB transport looks like it has the same window, and there the crash would
happen in URB completion context. usb_ops has no .nci_ready, and
nfcmrvl_probe() in drivers/nfc/nfcmrvl/usb.c sets drv_data->priv only after
registration:

	priv = nfcmrvl_nci_register_dev(NFCMRVL_PHY_USB, drv_data, &usb_ops,
					&intf->dev, &config);
	if (IS_ERR(priv))
		return PTR_ERR(priv);

	drv_data->priv = priv;

An early DEV_UP goes through nfcmrvl_usb_nci_open(), which submits the
bulk-IN URBs and sets NFCMRVL_USB_BULK_RUNNING. nfcmrvl_bulk_complete()
guards only on this check:

	if (!test_bit(NFCMRVL_NCI_RUNNING, &drv_data->flags))
		return;

NFCMRVL_NCI_RUNNING and NFCMRVL_USB_BULK_RUNNING are both bit 1, so the
guard passes. The handler then does:

		skb = nci_skb_alloc(drv_data->priv->ndev, urb->actual_length,
				    GFP_ATOMIC);

When the controller answers the CORE_RESET in that window, does this
dereference a NULL drv_data->priv?

>  	if (rc) {
>  		nfc_err(dev, "nci_register_device failed %d\n", rc);
>  		goto error_fw_dnld_deinit;
>  	}

[ ... ]

> diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
> index 9aedd168759c7..d688071f4b26d 100644
> --- a/drivers/nfc/nfcmrvl/uart.c
> +++ b/drivers/nfc/nfcmrvl/uart.c
> @@ -44,6 +44,23 @@ 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;
> +}

[Severity: Medium]
This sets the TX-side state before registration. Is the RX side still
attached too late?

nci_uart_set_driver() in net/nfc/nci/uart.c sets tty->disc_data only after
nu->ops.open() returns, and open() is the call that exposes the NCI device:

	ret = nu->ops.open(nu);
	...
	tty->disc_data = nu;

Until then, nci_uart_tty_receive() drops all incoming data:

	if (!nu || tty != nu->tty)
		return;

nci_uart_tty_wakeup() also returns early in that window.

nfc_dev_up() holds device_lock() across the whole nci_dev_up() reset
request. Meanwhile, the registering task waits on device_lock() in
nfc_register_device(), after device_add(). In the early DEV_UP case this
patch targets, the CORE_RESET goes out, but the response seems to be lost.
open() cannot return and set disc_data until the reset request times out.

Would the early DEV_UP then fail with a timeout instead of crashing, with
the NCIUARTSETDRIVER ioctl stalled for that time? Does tty->disc_data also
need to be set before the device is exposed?

> +
> +static void nfcmrvl_uart_nci_unready(struct nfcmrvl_private *priv)
> +{
> +	struct nci_uart *nu = priv->drv_data;
> +
> +	nu->drv_data = NULL;
> +	nu->ndev = NULL;
> +}

[Severity: High]
This is a pre-existing issue, not one this patch introduced, and the next
patch in this series ("nfc: nci: uart: quiesce write_work during teardown")
appears to fix it. The issue: nu->drv_data and nu->ndev are cleared only on
the registration failure path, never on normal teardown.

In nci_uart_tty_close(), nu->ops.close() runs before write_work is
cancelled:

	nu->ops.close(nu);
	nu->tty = NULL;
	module_put(nu->owner);

	cancel_work_sync(&nu->write_work);

nu->ops.close() reaches nfcmrvl_nci_unregister_dev(), which frees the
nci_dev and priv without quiescing UART TX. The try_module_get() failure
path in nci_uart_set_driver() calls nu->ops.close() and kfree(nu) without
cancelling write_work at all.

Say write_work is pending or running, for example because
nci_unregister_device()->nci_close_device() sent a CORE_RESET. Could
nfcmrvl_nci_uart_tx_start() or nfcmrvl_nci_uart_tx_done() then use the
freed priv, ndev or nu?

The later patch addresses this. It adds an nci_quiesce hook that calls
nci_uart_tx_stop() and nci_unready() in nfcmrvl_nci_unregister_dev() before
kfree(priv). It also adds a CLOSING check in nci_uart_tx_wakeup(). Finally,
it moves nci_uart_tx_stop() earlier in nci_uart_tty_close() and adds it to
the open error path in nci_uart_set_driver().

> +
>  static void nfcmrvl_uart_nci_update_config(struct nfcmrvl_private *priv,
>  					   const void *param)
>  {

[ ... ]

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