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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Late initialization of `raw_nci` causes an `skb_under_panic` due to a race with userspace.
- [High] Improper power sequencing by driving the reset GPIO high before enabling VDD-IO.

Pre-existing issues:
- [High] Failing probe IRQ registration leaks the NCI device and causes a Use-After-Free.
- [High] Unsafe teardown order in `ndlc_remove` allows a dangling worker to be rescheduled, leading to a Use-After-Free.
- [High] Buffer overflow and `skb_over_panic` due to tailroom consumption mismatch in the legacy read path.
- [High] Unaligned memory access when parsing the legacy NDLC frame length.
- [High] Permanent SKB memory leak when a hardware write fails in the worker thread.
--

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

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

ST21NFCD does not use NDLC. When the compatible is st,st21nfcd,
talk raw NCI.

> diff --git a/drivers/nfc/st-nci/i2c.c b/drivers/nfc/st-nci/i2c.c
> index 9ae839a6f5cc0..566689d5aadb4 100644
> --- a/drivers/nfc/st-nci/i2c.c
> +++ b/drivers/nfc/st-nci/i2c.c
[ ... ]
> @@ -111,6 +122,42 @@ static int st_nci_i2c_read(struct st_nci_i2c_phy *phy,
>  	u8 buf[ST_NCI_I2C_MAX_SIZE];
>  	struct i2c_client *client = phy->i2c_dev;
>  
> +	if (phy->raw_nci) {
> +		r = i2c_master_recv(client, buf, ST_NCI_NCI_HDR_SIZE);
> +		if (r < 0) {
> +			usleep_range(1000, 4000);
> +			r = i2c_master_recv(client, buf, ST_NCI_NCI_HDR_SIZE);
> +		}
> +		if (r != ST_NCI_NCI_HDR_SIZE)
> +			return -EREMOTEIO;
> +
> +		len = buf[2];
> +		if (len > ST_NCI_I2C_MAX_SIZE) {
> +			nfc_err(&client->dev, "invalid frame len\n");
> +			return -EBADMSG;
> +		}
> +
> +		*skb = alloc_skb(ST_NCI_NCI_HDR_SIZE + len, GFP_KERNEL);
> +		if (!*skb)
> +			return -ENOMEM;
> +
> +		skb_put(*skb, ST_NCI_NCI_HDR_SIZE);
> +		memcpy((*skb)->data, buf, ST_NCI_NCI_HDR_SIZE);
> +
> +		if (!len)
> +			return 0;
> +
> +		r = i2c_master_recv(client, buf, len);
> +		if (r != len) {
> +			kfree_skb(*skb);
> +			return -EREMOTEIO;
> +		}
> +
> +		skb_put(*skb, len);
> +		memcpy((*skb)->data + ST_NCI_NCI_HDR_SIZE, buf, len);
> +		return 0;
> +	}
> +
>  	r = i2c_master_recv(client, buf, ST_NCI_I2C_MIN_SIZE);
>  	if (r < 0) {  /* Retry, chip was in standby */
>  		usleep_range(1000, 4000);

[Severity: High]
This is a pre-existing issue, but does the legacy read path consume more
headroom and tailroom than it allocates, causing an skb_over_panic?

st_nci_i2c_read() allocates an SKB of size 4 + len:

    *skb = alloc_skb(ST_NCI_I2C_MIN_SIZE + len, GFP_KERNEL);

Then it reserves 4 bytes, leaving len bytes of tailroom:

    skb_reserve(*skb, ST_NCI_I2C_MIN_SIZE);

But subsequent skb_put() calls consume 4 + len bytes of tailroom:

    skb_put(*skb, ST_NCI_I2C_MIN_SIZE);
    ...
    skb_put(*skb, len);

Because only len bytes were left in the tailroom, the operations require 4
more bytes than what was requested. If the allocation is just beneath a slab
boundary without extra padding, won't the final skb_put(len) trigger an
skb_over_panic?

[Severity: High]
This is also a pre-existing issue, but does parsing the legacy NDLC frame
length cause an unaligned memory access?

The frame header is read into a stack-allocated byte array:

    u8 buf[ST_NCI_I2C_MAX_SIZE];

Later in st_nci_i2c_read(), the code casts an unaligned offset directly to a
16-bit integer pointer:

    len = be16_to_cpu(*(__be16 *) (buf + 2));

On strict-alignment architectures, won't this unaligned dereference trigger
a kernel panic or alignment fault?

[ ... ]
> @@ -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");
> +

[Severity: High]
Can driving the reset GPIO high before enabling VDD-IO risk hardware damage?

In st_nci_i2c_probe(), the reset GPIO is requested and actively driven high
via GPIOD_OUT_HIGH:

    phy->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);

The vdd-io regulator and system clocks are then requested and enabled after
the GPIO is driven here.

Driving an active HIGH signal into a CMOS IC's I/O pin before its VDD rail
is powered violates power sequencing requirements. This forces voltage
through the chip's internal ESD protection diodes, back-powering the chip.
Should the regulator and clocks be enabled before driving the GPIO high?

>  	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]
Can assigning this flag after ndlc_probe() cause an skb_under_panic crash?

Because ndlc_probe() registers the NCI device, the interface is exposed to
userspace before phy->ndlc->raw_nci is initialized. If userspace brings up
the device immediately, st_nci_init() and subsequent commands can execute
while raw_nci is still false.

If ndlc_send() sees raw_nci == false, it will attempt to push the NDLC
header:

drivers/nfc/st-nci/ndlc.c:ndlc_send() {
    ...
    if (!ndlc->raw_nci) {
        ...
        *(u8 *)skb_push(skb, 1) = pcb;
    }
    ...
}

Because the SKB was allocated for a raw NCI device with tx_headroom = 0,
this skb_push() call would panic. Should raw_nci be initialized before
ndlc_probe() exposes the device to userspace?

>  
>  	phy->irq_active = true;
>  	r = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>  				st_nci_irq_thread_fn,
>  				IRQF_ONESHOT,
>  				ST_NCI_DRIVER_NAME, phy);
>  	if (r < 0)
>  		nfc_err(&client->dev, "Unable to register IRQ handler\n");
>  
>  	return r;
>  }

[Severity: High]
This is a pre-existing issue, but does failing to register the IRQ here leak
the NCI device and cause a use-after-free?

ndlc_probe() allocates and registers the NCI device (ndev), but ndev is not
devres-managed and requires st_nci_remove() to unregister and free it.

If devm_request_threaded_irq() fails, the probe function immediately returns
an error code. This causes devres to free phy and other devm-managed memory,
but leaves the network interface registered. Could subsequent userspace
interaction with the lingering interface dereference the freed pointers?

> diff --git a/drivers/nfc/st-nci/ndlc.c b/drivers/nfc/st-nci/ndlc.c
> index be4808859cfa9..b3192460c96e3 100644
> --- a/drivers/nfc/st-nci/ndlc.c
> +++ b/drivers/nfc/st-nci/ndlc.c
[ ... ]
> @@ -103,6 +106,10 @@ static void llt_ndlc_send_queue(struct llt_ndlc *ndlc)
>  			ndlc->hard_fault = r;
>  			break;
>  		}
> +		if (ndlc->raw_nci) {
> +			kfree_skb(skb);
> +			continue;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but does failing a hardware write
leak the dequeued SKB permanently?

In llt_ndlc_send_queue(), an SKB is dequeued and transmitted:

    skb = skb_dequeue(&ndlc->send_q);
    r = ndlc->ops->write(ndlc->phy_id, skb);
    if (r < 0) {
        ndlc->hard_fault = r;
        break;
    }

If the write fails, the loop breaks immediately. The dequeued SKB is neither
freed nor re-queued, stranding the memory permanently. Furthermore, because
hard_fault is set, won't all subsequently scheduled SKBs also dequeue, fail,
and leak, allowing memory exhaustion?

[ ... ]
> @@ -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);

[Severity: High]
This is also a pre-existing issue, but does ndlc_remove() have an unsafe
teardown order that can cause a use-after-free?

In ndlc_remove(), cancel_work_sync(&ndlc->sm_work) is called to cancel
pending work before the hardware IRQ is disabled in st_nci_remove():

drivers/nfc/st-nci/ndlc.c:ndlc_remove() {
    ...
    cancel_work_sync(&ndlc->sm_work);
    ...
    st_nci_remove(ndlc->ndev);
    ...
}

If an interrupt fires in the window between cancel_work_sync() and the IRQ
being disabled, st_nci_irq_thread_fn() will read a packet and call
ndlc_recv(), which executes schedule_work(&ndlc->sm_work).

When driver removal finishes, devres frees the ndlc structure, leaving the
newly scheduled worker to execute on freed memory. Is it possible to disable
the IRQ before cancelling the work?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819210906.2500789-1-isyourbrainfoss@proton.me?part=2