[PATCH] nfc: st-nci: Add error handling to IRQ handlers

[PATCH] nfc: st-nci: Add error handling to IRQ handlers

[Greg Kroah-Hartman]

From: Griffin Kroah-Hartman <griffin@kroah.com>

Add ndlc_remove() to st_nci_i2c_probe() and st_nci_spi_probe() if the
devm_request_threaded_irq() function fails.  This is to properly unwind
after ndlc_probe() was called prior to this.

Assisted-by: gkh_clanker_2000
Cc: David Heidelberg <david@ixit.cz>
Cc: "Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/nfc/st-nci/i2c.c | 4 +++-
 drivers/nfc/st-nci/spi.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nfc/st-nci/i2c.c b/drivers/nfc/st-nci/i2c.c
index 9ae839a6f5cc..7f727d9b97ba 100644
--- a/drivers/nfc/st-nci/i2c.c
+++ b/drivers/nfc/st-nci/i2c.c
@@ -243,8 +243,10 @@ static int st_nci_i2c_probe(struct i2c_client *client)
 				st_nci_irq_thread_fn,
 				IRQF_ONESHOT,
 				ST_NCI_DRIVER_NAME, phy);
-	if (r < 0)
+	if (r < 0) {
 		nfc_err(&client->dev, "Unable to register IRQ handler\n");
+		ndlc_remove(phy->ndlc);
+	}
 
 	return r;
 }
diff --git a/drivers/nfc/st-nci/spi.c b/drivers/nfc/st-nci/spi.c
index 169eacc0a32a..6f1ffd8244fa 100644
--- a/drivers/nfc/st-nci/spi.c
+++ b/drivers/nfc/st-nci/spi.c
@@ -257,8 +257,10 @@ static int st_nci_spi_probe(struct spi_device *dev)
 				st_nci_irq_thread_fn,
 				IRQF_ONESHOT,
 				ST_NCI_SPI_DRIVER_NAME, phy);
-	if (r < 0)
+	if (r < 0) {
 		nfc_err(&dev->dev, "Unable to register IRQ handler\n");
+		ndlc_remove(phy->ndlc);
+	}
 
 	return r;
 }
-- 
2.55.0

Re: [PATCH] nfc: st-nci: Add error handling to IRQ handlers

[David Heidelberg]

On 07/07/2026 16:21, Greg Kroah-Hartman wrote:
> From: Griffin Kroah-Hartman <griffin@kroah.com>
> 
> Add ndlc_remove() to st_nci_i2c_probe() and st_nci_spi_probe() if the
> devm_request_threaded_irq() function fails.  This is to properly unwind
> after ndlc_probe() was called prior to this.
> 
> Assisted-by: gkh_clanker_2000
> Cc: David Heidelberg <david@ixit.cz>
> Cc: "Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   drivers/nfc/st-nci/i2c.c | 4 +++-
>   drivers/nfc/st-nci/spi.c | 4 +++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 
Heya,

the Sashiko review look reasonable here, check?

Thanks
David

--- Patch [1]: [PATCH] nfc: st-nci: Add error handling to IRQ handlers ---
commit ae7497e7a0338b16d82e7783afc2835614c291c1
Author: Griffin Kroah-Hartman <griffin@kroah.com>

nfc: st-nci: Add error handling to IRQ handlers

Add ndlc_remove() to st_nci_i2c_probe() and st_nci_spi_probe() if the
devm_request_threaded_irq() function fails.  This is to properly unwind
after ndlc_probe() was called prior to this.

 > diff --git a/drivers/nfc/st-nci/i2c.c b/drivers/nfc/st-nci/i2c.c
 > index 9ae839a6f5cc0..7f727d9b97ba8 100644
 > --- a/drivers/nfc/st-nci/i2c.c
 > +++ b/drivers/nfc/st-nci/i2c.c
 > @@ -243,8 +243,10 @@ static int st_nci_i2c_probe(struct i2c_client *client)
 >  				st_nci_irq_thread_fn,
 >  				IRQF_ONESHOT,
 >  				ST_NCI_DRIVER_NAME, phy);

This is a pre-existing issue, but is there a race condition here?

In st_nci_i2c_probe(), the driver calls ndlc_probe() which synchronously
registers the device to the netlink subsystem, exposing it to userspace.
The IRQ handler is requested immediately after.

If userspace opens the device in the brief window between ndlc_probe()
and devm_request_threaded_irq(), could st_nci_open() attempt to enable
an unrequested IRQ and cause communication timeouts?

 > -	if (r < 0)
 > +	if (r < 0) {
 >  		nfc_err(&client->dev, "Unable to register IRQ handler\n");
 > +		ndlc_remove(phy->ndlc);

Does calling ndlc_remove() here lead to an unbalanced IRQ disable?

When devm_request_threaded_irq() fails, this new error path calls
ndlc_remove(). This function eventually calls st_nci_i2c_disable(),
which unconditionally executes disable_irq_nosync().

Since the IRQ registration just failed, would disabling this unrequested
shared IRQ line break other devices and trigger an unbalanced IRQ disable
warning?

Will this error path also cause a blocking wait and memory leak?

When ndlc_remove() cascades to ndlc_close(), it sends a synchronous NCI
mode-set command via nci_prop_cmd().

Since the device was never opened by userspace during this failed probe,
the ST_NCI_RUNNING flag is not set. This causes st_nci_send() to return
-EBUSY and drop the transmission without freeing the sk_buff.

Because nci_prop_cmd() waits synchronously for a response that will never
arrive, won't this block the probe failure path for the full 2-second
timeout?

This is a pre-existing issue, but does the teardown order in ndlc_remove()
allow work items to be re-armed after cancellation?

In ndlc_remove(), background work is cancelled before deregistration:

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

However, st_nci_remove() delegates to ndlc_close(), which queues a
mode-set command. If the interface was running, this command flows to
ndlc_send(), which calls schedule_work(&ndlc->sm_work).

Could this re-arm the work item after the cancellation sync, allowing it
to outlive the device's managed memory and cause a use-after-free?

This is a pre-existing issue, but are the Secure Element timers leaking
during device removal?

The timers bwi_timer and se_active_timer are initialized in
st_nci_se_init() using managed memory, but st_nci_remove() orchestrates
teardown without ever calling the designated st_nci_se_deinit() cleanup
function:

drivers/nfc/st-nci/core.c:st_nci_remove() {
     struct st_nci_info *info = nci_get_drvdata(ndev);

     ndlc_close(info->ndlc);

     nci_unregister_device(ndev);
     nci_free_device(ndev);
}

If a timer is running when the module unbinds, could it survive the
devres cleanup and cause a use-after-free when it fires?

This is a pre-existing issue, but is there a missing cleanup step if
st_nci_se_init() fails?

In st_nci_probe(), the device is registered to the core, but if the
subsequent Secure Element initialization fails, it returns early:

drivers/nfc/st-nci/core.c:st_nci_probe() {
     ...
     r = nci_register_device(ndlc->ndev);
     if (r) {
         pr_err("Cannot register nfc device to nci core\n");
         goto err_reg_dev;
     }

     return st_nci_se_init(ndlc->ndev, se_status);
}

If st_nci_se_init() returns an error, the devres cleanup will free the
driver-private objects, but the NCI interface is never unregistered.
Would this leave a dangling interface accessible to userspace, leading
to a use-after-free?

 > +	}
 >
 >  	return r;
 >  }

[ ... ]

Re: [PATCH] nfc: st-nci: Add error handling to IRQ handlers

[Griffin Kroah-Hartman]

On 7/19/26 2:27 PM, David Heidelberg wrote:
> On 07/07/2026 16:21, Greg Kroah-Hartman wrote:
>> From: Griffin Kroah-Hartman <griffin@kroah.com>
>>
>> Add ndlc_remove() to st_nci_i2c_probe() and st_nci_spi_probe() if the
>> devm_request_threaded_irq() function fails.  This is to properly unwind
>> after ndlc_probe() was called prior to this.
>>
>> Assisted-by: gkh_clanker_2000
>> Cc: David Heidelberg <david@ixit.cz>
>> Cc: "Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>> Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>   drivers/nfc/st-nci/i2c.c | 4 +++-
>>   drivers/nfc/st-nci/spi.c | 4 +++-
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
> Heya,
>
> the Sashiko review look reasonable here, check?
>
> > diff --git a/drivers/nfc/st-nci/i2c.c b/drivers/nfc/st-nci/i2c.c
> > index 9ae839a6f5cc0..7f727d9b97ba8 100644
> > --- a/drivers/nfc/st-nci/i2c.c
> > +++ b/drivers/nfc/st-nci/i2c.c
> > @@ -243,8 +243,10 @@ static int st_nci_i2c_probe(struct i2c_client 
> *client)
> >                  st_nci_irq_thread_fn,
> >                  IRQF_ONESHOT,
> >                  ST_NCI_DRIVER_NAME, phy);
>
> If userspace opens the device in the brief window between ndlc_probe()
> and devm_request_threaded_irq(), could st_nci_open() attempt to enable
> an unrequested IRQ and cause communication timeouts?
>
> > -    if (r < 0)
> > +    if (r < 0) {
> >          nfc_err(&client->dev, "Unable to register IRQ handler\n");
> > +        ndlc_remove(phy->ndlc);
>
> Does calling ndlc_remove() here lead to an unbalanced IRQ disable?
>
> When devm_request_threaded_irq() fails, this new error path calls
> ndlc_remove(). This function eventually calls st_nci_i2c_disable(),
> which unconditionally executes disable_irq_nosync().
>
> Since the IRQ registration just failed, would disabling this unrequested
> shared IRQ line break other devices and trigger an unbalanced IRQ disable
> warning?
>
> Will this error path also cause a blocking wait and memory leak?
>
> When ndlc_remove() cascades to ndlc_close(), it sends a synchronous NCI
> mode-set command via nci_prop_cmd().
>
> Since the device was never opened by userspace during this failed probe,
> the ST_NCI_RUNNING flag is not set. This causes st_nci_send() to return
> -EBUSY and drop the transmission without freeing the sk_buff.
>
> Because nci_prop_cmd() waits synchronously for a response that will never
> arrive, won't this block the probe failure path for the full 2-second
> timeout? 

I agree, unwinding this error tree was much more complicated than I had 
assumed,

these warning messages look reasonable to me as well.


I will forgo this patch and ignore this change for now, Thanks for the 
catch!


Griffin