Re: [PATCH v3 2/3] nfc: st-nci: add raw NCI path for ST21NFCD

[sashiko-bot@kernel.org]

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

New issues:
- [High] The NCI device is registered before the `raw_nci` flag is fully initialized, allowing userspace to trigger `ndlc_send` with 0 headroom on a non-raw setup, causing an `skb_under_panic`.
- [Low] The patch modifies `st_nci_i2c_id_table` to use positional initializers instead of named initializers for `struct i2c_device_id`.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but if `devm_request_threaded_irq()` fails during probe, the function returns an error without unregistering or freeing the NCI device, leaving a dangling registered netdev.
- [High] This problem wasn't introduced by this patch, but module removal destroys the NCI device and cancels workqueues while the IRQ handler is still active, allowing the handler to access freed memory or reschedule cancelled work.
- [High] This problem wasn't introduced by this patch, but received SKBs containing SUPERVISOR frames (like ACK, NACK, WAIT) are never freed in `llt_ndlc_rcv_queue`, causing a permanent memory leak.
--

--- Patch [2]: [PATCH v3 2/3] nfc: st-nci: add raw NCI path for ST21NFCD ---
commit 5f6ff3e39e1921d843bbd676cd50cbaac4a522d8
Author: Kristian Brox <isyourbrainfoss@proton.me>

nfc: st-nci: add raw NCI path for ST21NFCD

This commit introduces a raw NCI path for the ST21NFCD. It adds
configuration properties to bypass NDLC PCB handling, ACK timers, and
proprietary SET_NFC_MODE commands.

> diff --git a/drivers/nfc/st-nci/i2c.c b/drivers/nfc/st-nci/i2c.c
> --- a/drivers/nfc/st-nci/i2c.c
> +++ b/drivers/nfc/st-nci/i2c.c

[ ... ]

> @@ -225,19 +274,31 @@ static int st_nci_i2c_probe(struct i2c_client *client)
>  		return -ENODEV;
>  	}
>  
> +	r = devm_regulator_get_enable_optional(dev, "vdd-io");
> +	if (r && r != -ENODEV)
> +		return dev_err_probe(dev, r, "failed to enable vdd-io\n");
> +
> +	r = PTR_ERR_OR_ZERO(devm_clk_get_optional_enabled(dev, NULL));
> +	if (r)
> +		return dev_err_probe(dev, r, "failed to enable clock\n");
> +
>  	phy->se_status.is_ese_present =
>  				device_property_read_bool(dev, "ese-present");
>  	phy->se_status.is_uicc_present =
>  				device_property_read_bool(dev, "uicc-present");
>  
>  	r = ndlc_probe(phy, &i2c_phy_ops, &client->dev,
> -			ST_NCI_FRAME_HEADROOM, ST_NCI_FRAME_TAILROOM,
> +			phy->raw_nci ? ST_NCI_RAW_FRAME_HEADROOM :
> +				       ST_NCI_FRAME_HEADROOM,
>  			ST_NCI_FRAME_TAILROOM,
>  			&phy->ndlc, &phy->se_status);
>  	if (r < 0) {
>  		nfc_err(&client->dev, "Unable to register ndlc layer\n");
>  		return r;
>  	}
>  
> +	phy->ndlc->raw_nci = phy->raw_nci;

[Severity: High]
Could this lead to an skb_under_panic?

Because ndlc_probe() registers the NCI device and exposes it to userspace,
is it possible for userspace to immediately issue a DEV_UP command before
phy->ndlc->raw_nci is assigned?

If a packet is queued right away, ndlc_send() might see raw_nci == false
and attempt to push a 1-byte NDLC header into an SKB allocated with
0 headroom (ST_NCI_RAW_FRAME_HEADROOM), underflowing the SKB.

> +
>  	phy->irq_active = true;
>  	r = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>  				st_nci_irq_thread_fn,
> @@ -257,23 +318,25 @@ static void st_nci_i2c_remove(struct i2c_client *client)
>  	if (r < 0)
>  		nfc_err(&client->dev, "Unable to register IRQ handler\n");
>  
>  	return r;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but if devm_request_threaded_irq()
fails here, does it leave a dangling NCI device?

Since ndlc_probe() already allocated and registered the NCI device with
the networking core, returning an error without unregistering it leaves the
netdev active. When devres later frees the phy and ndlc structures, could
userspace still access the freed interface and trigger a use-after-free panic?

>  static void st_nci_i2c_remove(struct i2c_client *client)
>  {
>  	struct st_nci_i2c_phy *phy = i2c_get_clientdata(client);
>  
>  	ndlc_remove(phy->ndlc);
>  }

[Severity: High]
This is a pre-existing issue, but does ndlc_remove() destroy the NCI device
and cancel workqueues while the IRQ handler is still active?

Since the devres-managed IRQ isn't explicitly disabled or freed here, if an
interrupt fires concurrently during module removal, could the handler schedule
cancelled work or access the already-freed ndlc->ndev, leading to a crash?

>  static const struct i2c_device_id st_nci_i2c_id_table[] = {
> -	{ .name = ST_NCI_DRIVER_NAME },
> -	{ }
> +	{ ST_NCI_DRIVER_NAME },
> +	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, st_nci_i2c_id_table);

[Severity: Low]
Is there a reason for removing the named initializer here?

The I2C subsystem guidelines require initialized arrays of type
struct i2c_device_id to use named initializers.

[ ... ]

> diff --git a/drivers/nfc/st-nci/ndlc.c b/drivers/nfc/st-nci/ndlc.c
> --- a/drivers/nfc/st-nci/ndlc.c
> +++ b/drivers/nfc/st-nci/ndlc.c

[ ... ]

> @@ -154,6 +161,10 @@ static void llt_ndlc_rcv_queue(struct llt_ndlc *ndlc)
>  		pr_debug("rcvQlen=%d\n", ndlc->rcv_q.qlen);
>  
>  	while ((skb = skb_dequeue(&ndlc->rcv_q)) != NULL) {
> +		if (ndlc->raw_nci) {
> +			nci_recv_frame(ndlc->ndev, skb);
> +			continue;
> +		}
>  		pcb = skb->data[0];
>  		skb_pull(skb, 1);
>  		if ((pcb & PCB_TYPE_MASK) == PCB_TYPE_SUPERVISOR) {
> 			switch (pcb & PCB_SYNC_MASK) {
> 			case PCB_SYNC_ACK:
> 				skb = skb_dequeue(&ndlc->ack_pending_q);
> 				kfree_skb(skb);

[Severity: High]
This isn't a bug introduced by this patch, but does the PCB_SYNC_ACK path
leak the received skb?

When a PCB_SYNC_ACK frame is dequeued from ndlc->rcv_q, the skb pointer
is overwritten by skb_dequeue(&ndlc->ack_pending_q), which is then freed.
It appears the original received SKB containing the ACK frame is never
freed, leading to a permanent memory leak during normal hardware communication.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-fp5-st21nfcd-v3-v3-0-5ee0a7f44d04@proton.me?part=2