Re: [PATCH v4 3/3] arm64: dts: qcom: qcm6490-fairphone-fp5: add ST21NFCD NFC

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Leaked NCI device and potential Use-After-Free on IRQ request failure
- [High] Double IRQ disable causing permanent interrupt loss in ST_NCI
--

commit f44916e46bbad8ef743764748ded612267568862
Author: Kristian Brox <isyourbrainfoss@proton.me>

arm64: dts: qcom: qcm6490-fairphone-fp5: add ST21NFCD NFC
    
Enable the ST21NFCD on i2c9 (0x08), IRQ TLMM 41, reset TLMM 38
active-low. Compatible is st,st21nfcd (raw NCI).

> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> index f41ad446a39e2..b028d1bb25d4c 100644
> --- a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> +++ b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> @@ -933,7 +933,23 @@ redriver_ss_in: endpoint {
>  &i2c9 {
>  	status = "okay";
>  
> -	/* ST21NFC NFC @ 28 */
> +	nfc@8 {
> +		compatible = "st,st21nfcd";

[Severity: High]
This isn't a bug introduced by this patch, but I noticed some pre-existing
issues in the st-nci driver that this enables.

Does st_nci_i2c_probe() leak the NCI device and cause a use-after-free if
the IRQ request fails?

drivers/nfc/st-nci/i2c.c:st_nci_i2c_probe() {
    ...
	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;
}

If devm_request_threaded_irq() fails, it returns the error without calling
ndlc_remove() to unregister and free the NCI device. This leaves a dangling
network interface registered, retaining pointers to freed devres memory (phy
and ndlc structures). Any subsequent userspace access could dereference
freed memory.


[Severity: High]
This is a pre-existing issue, but can a race condition cause a double IRQ
disable and permanent interrupt loss?

In ndlc_close(), ndlc->powered is set to 0 before calling
ndlc->ops->disable(). If an interrupt fires during this window, the IRQ
thread races with the shutdown path:

drivers/nfc/st-nci/i2c.c:st_nci_irq_thread_fn() {
    ...
	if (!phy->ndlc->powered) {
		st_nci_i2c_disable(phy);
		return IRQ_HANDLED;
	}
    ...
}

drivers/nfc/st-nci/i2c.c:st_nci_i2c_disable() {
    ...
	disable_irq_nosync(phy->i2c_dev->irq);
	phy->irq_active = false;
}

Both paths could call disable_irq_nosync(), incrementing the IRQ descriptor
depth to 2. Subsequent enable_irq() calls would only decrement it once,
leaving the NFC controller permanently unresponsive to interrupts.

> +		reg = <0x08>;
> +
> +		interrupts-extended = <&tlmm 41 IRQ_TYPE_LEVEL_HIGH>;
> +		reset-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-fp5-st21nfcd-v4-v4-0-ded2f1c501be@proton.me?part=3