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

[David Heidelberg]

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: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Ian Ray <ian.ray@xxxxxxxxxxxxxxxx>
> ---
> 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 <ian.ray@xxxxxxxxxxxxxxxx>
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?