Re: [PATCH nfc] nfc: nci: fix double completion race in nci_data_exchange_complete

From: 'Simon Horman' <[email protected]>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
Subject: nfc: nci: fix double completion race in nci_data_exchange_complete

This patch attempts to resolve a double completion race condition by
replacing clear_bit() with test_and_clear_bit() on the NCI_DATA_EXCHANGE
flag. This is intended to prevent a socket refcount underflow when
nci_close_device() and nci_rx_work() execute concurrently.

> diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
> index 5f98c73db5afd..4253edea5c8db 100644
> --- a/net/nfc/nci/data.c
> +++ b/net/nfc/nci/data.c
> @@ -46,11 +46,11 @@ void nci_data_exchange_complete(struct nci_dev *ndev, struct sk_buff *skb,

1.
Simon says: Although the issue flagged below is pre-existing, it appears
            to be exacerbated by this patch and thus I believe it
	    needs to be considered in that context.

[Severity: High]
This is a pre-existing issue, but does an NTF packet with an invalid
connection ID improperly clear the transceive state?

Looking earlier in nci_data_exchange_complete():

    conn_info = nci_get_conn_info_by_conn_id(ndev, conn_id);
    if (!conn_info) {
        kfree_skb(skb);
        clear_bit(NCI_DATA_EXCHANGE, &ndev->flags);
        return;
    }

If a misbehaving or malicious NFC hardware sends an error NTF packet with an
invalid connection ID while a valid RF transceive operation is active, it
would execute clear_bit(NCI_DATA_EXCHANGE, &ndev->flags). Would this abort
the ongoing transceive state machine and drop the actual response when it
arrives?

2.
Simon says: Conversely, the issue flagged below seems to be orthogonal
            to this patch. And I think it can be treated in the context
	    of possible follow-up.

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a use-after-free
race condition in nci_transceive()?

In nci_transceive(), the NCI_DATA_EXCHANGE flag is set using
test_and_set_bit() before the data_exchange_cb and data_exchange_cb_context
pointers are updated on the conn_info structure.

Since incoming packets and notifications are processed concurrently by
nci_rx_work, could an exceptionally fast response or a concurrent interface
error notification trigger nci_data_exchange_complete() immediately after
the bit is set but before the callback pointers are updated?

This could cause the lockless reader to fetch the stale callback and context
pointers from a previous transceive socket and execute them.

>  	timer_delete_sync(&ndev->data_timer);
>  	clear_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags);
>  
> -	/* Mark the exchange as done before calling the callback.
> -	 * The callback (e.g. rawsock_data_exchange_complete) may
> -	 * want to immediately queue another data exchange.
> -	 */
> -	clear_bit(NCI_DATA_EXCHANGE, &ndev->flags);
> +	/* Claim completion atomically -- both close and rx_work may race here */
> +	if (!test_and_clear_bit(NCI_DATA_EXCHANGE, &ndev->flags)) {
> +		kfree_skb(skb);
> +		return;
> +	}

3.
Simon says: Like issue 1. I believe the issue below should be considered
            in the context of this patch.

[Severity: High]
Will this change cause valid HCI and loopback packets to be dropped?

The NCI_DATA_EXCHANGE bit is only set during RF transceive operations in
nci_transceive(). Subsystems like HCI (via nci_hci_send_cmd()) and loopback
send data without setting this global flag.

If valid HCI or loopback data packets arrive, won't they fail this bit
check, causing the SKB to be dropped and skipping callbacks like
nci_hci_data_received_cb?

Additionally, if an asynchronous packet arrives concurrently while a valid
RF transceive is active, could it falsely consume the NCI_DATA_EXCHANGE bit?
When the actual RF response arrives, it would then fail the check, drop the
packet, and permanently leak the socket's reference count.

>  
>  	if (cb) {
>  		/* forward skb to nfc core */
>  		cb(cb_context, skb, err);

4.
Simon says: And like issue 3. I believe this is orthogonal to this patch
            and can be considered in the context of possible follow-up.

[Severity: High]
This is a pre-existing issue, but does this code leak the SKB when an
error status is returned?

When nci_add_rx_data_frag() processes an incoming fragment with a non-zero
status byte, it passes both the valid skb and the err code to
nci_data_exchange_complete(), which forwards them to the cb here.

Looking at core callbacks like rawsock_data_exchange_complete():

net/nfc/rawsock.c:rawsock_data_exchange_complete() {
    ...
    if (err)
        goto error;
    ...
error:
    sock_put(sk);
}

The error label bypasses the error_skb label where kfree_skb(skb) is called.
Does this result in an unconditional memory leak of the SKB whenever the
hardware or external target responds with an error status?

>  	} else if (skb) {
>  		pr_err("no rx callback, dropping rx data...\n");
[ ... ]