Re: [PATCH v2] NFC: nxp-nci: fix deadlock between firmware download and IRQ handler

On 10/08/2026 10:21, Ian Ray wrote:
> The firmware download work function holds `info_lock' while calling the
> nxp_nci_fw_send(), which waits for a completion.
> 
> The completion is signalled by the IRQ thread handler, which also takes
> `info_lock', causing a deadlock.
> 
> Fix by dropping the mutex while waiting for the completion.
> 
> Fixes: dece45855a8b ("NFC: nxp-nci: Add support for NXP NCI chips")
> Cc: [email protected]
> Signed-off-by: Ian Ray <[email protected]>
> ---
> Changes since v1:
> - Fixes tag
> - Cc for stable
> ---
>   drivers/nfc/nxp-nci/firmware.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 

Hello Ian,

I checked the Sashiko review and it seems found an issue (feel free to ignore 
the 2nd pre-existing), could you check please (below)?

Thanks
David

--- Patch [1]: [PATCH v2] NFC: nxp-nci: fix deadlock between firmware download 
and IRQ handler ---
commit aeb1accbd1a732a455bf37b530649e8d9f406914
Author: Ian Ray <[email protected]>
Subject: NFC: nxp-nci: fix deadlock between firmware download and IRQ handler

This commit addresses a deadlock during firmware download by dropping the
info_lock mutex while waiting for the IRQ handler to signal completion.

 > diff --git a/drivers/nfc/nxp-nci/firmware.c b/drivers/nfc/nxp-nci/firmware.c
 > index a9533977aff8e..38fcac26ce563 100644
 > --- a/drivers/nfc/nxp-nci/firmware.c
 > +++ b/drivers/nfc/nxp-nci/firmware.c
 > @@ -154,8 +154,13 @@ static int nxp_nci_fw_send(struct nxp_nci_info *info)
 >  		if (fw_info->fw)
 >  			schedule_work(&fw_info->work);
 >  	} else {
 > +		mutex_unlock(&info->info_lock);
 > +
 >  		completion_rc = wait_for_completion_interruptible_timeout(
 >  			&fw_info->cmd_completion, NXP_NCI_FW_ANSWER_TIMEOUT);
 > +
 > +		mutex_lock(&info->info_lock);
 > +

If wait_for_completion_interruptible_timeout() times out, could this open a
race condition leading to a use-after-free?

When the timeout expires, the IRQ handler might run before info_lock is
re-acquired here. Since the timeout path hasn't cleaned up fw_info->fw yet,
the IRQ handler would see fw_info->fw != NULL and erroneously call
schedule_work(&fw_info->work) for a late response:

drivers/nfc/nxp-nci/firmware.c:nxp_nci_fw_recv_frame() {
     ...
     if (fw_info->fw)
         schedule_work(&fw_info->work);
     ...
}

After re-acquiring the lock, this function returns -ETIMEDOUT, and the caller
nxp_nci_fw_work_complete() will free the firmware buffer.

Could the spuriously scheduled work then execute and dereference the freed
memory when it calls nxp_nci_fw_send_chunk():

     skb_put_data(skb, fw_info->data + fw_info->written, chunk_len);

 >  		if (completion_rc == 0)
 >  			return -ETIMEDOUT;
 >  	}

This is a pre-existing issue, but does nxp_nci_remove() have a use-after-free
if a firmware download is currently running?

nxp_nci_remove() calls nxp_nci_fw_work_complete() to free the firmware buffer
before cancelling the work queue:

drivers/nfc/nxp-nci/core.c:nxp_nci_remove() {
     ...
     if (info->mode == NXP_NCI_MODE_FW)
         nxp_nci_fw_work_complete(info, -ESHUTDOWN);
     cancel_work_sync(&info->fw_info.work);
     ...
}

Since nxp_nci_fw_work_complete() frees fw_info->fw without holding info_lock,
could the concurrent nxp_nci_fw_work() thread be actively copying data from
fw_info->data, resulting in a use-after-free or a double free if the worker
finishes and calls nxp_nci_fw_work_complete() as well?

This is a pre-existing issue, but is there a potential integer underflow and
out-of-bounds read earlier in nxp_nci_fw_send()?

Before dropping the lock, the firmware size is modified without a length
check:

drivers/nfc/nxp-nci/firmware.c:nxp_nci_fw_send() {
     ...
     if (fw_info->written == 0) {
         fw_info->frame_size = get_unaligned_be16(fw_info->data) &
                               NXP_NCI_FW_FRAME_LEN_MASK;
         fw_info->data += NXP_NCI_FW_HDR_LEN;
         fw_info->size -= NXP_NCI_FW_HDR_LEN;
     }
     ...
}

If the provided firmware file is smaller than NXP_NCI_FW_HDR_LEN (2 bytes),
fw_info->size (which is unsigned) will underflow to a very large value.

Would this cause the subsequent safety check to be bypassed:

     if (fw_info->frame_size > fw_info->size)
         return -EMSGSIZE;

and allow nxp_nci_fw_send_chunk() to read out-of-bounds heap memory?