Re: [PATCH net v3] nfc: nci: Fix conn_info use-after-free

[Simon Horman <horms@kernel.org>]

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
---
nfc: nci: Fix conn_info use-after-free

This commit fixes a use-after-free in nci_tx_work() by introducing a dedicated
spinlock (conn_info_lock) to protect the conn_info_list and conn_info aliases
during lookup, insertion, and removal.

> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -1333,11 +1367,16 @@ void nci_unregister_device(struct nci_dev *ndev)
>  	destroy_workqueue(ndev->rx_wq);
>  	destroy_workqueue(ndev->tx_wq);
>  
> +	spin_lock_bh(&ndev->conn_info_lock);
>  	list_for_each_entry_safe(conn_info, n, &ndev->conn_info_list, list) {
>  		list_del(&conn_info->list);
> +		if (conn_info == ndev->rf_conn_info)
> +			ndev->rf_conn_info = NULL;
> +		if (conn_info == ndev->hci_dev->conn_info)
> +			ndev->hci_dev->conn_info = NULL;

[Severity: Critical]
Can this cause a NULL pointer dereference for devices without HCI support?

For NCI devices without HCI support (like nfcmrvl or s3fwrn5), ndev->hci_dev
is NULL. Because all NCI devices add at least the static RF connection to
conn_info_list during discovery, this loop will always execute.

Evaluating conn_info == ndev->hci_dev->conn_info will crash when
ndev->hci_dev is NULL.

Should this check ndev->hci_dev before accessing its conn_info field? The
same issue appears in nci_core_conn_close_rsp_packet() in net/nfc/nci/rsp.c.

> diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
> --- a/net/nfc/nci/data.c
> +++ b/net/nfc/nci/data.c
> @@ -171,40 +174,47 @@ static int nci_queue_tx_data_frags(struct nci_dev *ndev,
[ ... ]
> +	spin_lock_bh(&ndev->conn_info_lock);
>  	ndev->cur_conn_id = conn_id;
> -	queue_work(ndev->tx_wq, &ndev->tx_work);

[Severity: High]
This is a pre-existing issue, but does ndev->cur_conn_id suffer from
concurrent overwrites?

If a logical connection is being closed, nci_core_conn_close() sets
ndev->cur_conn_id and waits for the response.

If nci_send_data() is called concurrently, it overwrites ndev->cur_conn_id
without holding req_lock.

When the close response arrives, nci_core_conn_close_rsp_packet() will free
the conn_info for the actively transmitting connection instead of the closed
one. Does cur_conn_id need better protection or a different tracking
mechanism?

> @@ -282,11 +292,15 @@ void nci_rx_data_packet(struct nci_dev *ndev, struct sk_buff *skb)
[ ... ]
>  	/* strip the nci data header */
>  	skb_pull(skb, NCI_DATA_HDR_SIZE);

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds read
if the packet is empty?

If a target sends an empty packet (skb->len == 0), the skb_pull() just above
fails gracefully leaving skb->len at 0.

Then in nci_rx_data_packet(), the code accesses skb->data[skb->len - 1] to
read the status byte, reading out-of-bounds memory.

Does this need an skb->len > 0 check before accessing the status byte?

> diff --git a/net/nfc/nci/hci.c b/net/nfc/nci/hci.c
> --- a/net/nfc/nci/hci.c
> +++ b/net/nfc/nci/hci.c
> @@ -599,10 +632,20 @@ int nci_hci_set_param(struct nci_dev *ndev, u8 gate, u8 idx,
[ ... ]
> +		message = (struct nci_hcp_message *)rx_skb->data;
>  		r = nci_hci_result_to_errno(
>  			NCI_HCP_MSG_GET_CMD(message->header));
> -		skb_pull(conn_info->rx_skb, NCI_HCI_HCP_MESSAGE_HEADER_LEN);
> +		skb_pull(rx_skb, NCI_HCI_HCP_MESSAGE_HEADER_LEN);
> +		spin_unlock_bh(&ndev->conn_info_lock);
>  	}

[Severity: High]
This is a pre-existing issue, but does this leak the response skb?

After pulling the header to check the result, nci_hci_set_param() returns
without consuming the skb or calling kfree_skb().

The buffer remains in conn_info->rx_skb, and the next time an HCI response
is received, nci_hci_resp_received() unconditionally overwrites the pointer,
permanently leaking the memory.

Should the skb be freed if it's not being passed back to the caller?

> diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
> --- a/net/nfc/nci/rsp.c
> +++ b/net/nfc/nci/rsp.c
> @@ -298,20 +313,20 @@ static void nci_core_conn_create_rsp_packet(struct nci_dev *ndev,
>  		conn_info->dest_params->id = ndev->cur_params.id;
>  		conn_info->dest_params->protocol = ndev->cur_params.protocol;
>  		conn_info->conn_id = rsp->conn_id;
> +		conn_info->max_pkt_payload_len = rsp->max_ctrl_pkt_payload_len;
> +		atomic_set(&conn_info->credits_cnt, rsp->credits_cnt);

[Severity: High]
This is a pre-existing issue, but does this properly validate the length
of the response packet?

The skb->data is cast to struct nci_core_conn_create_rsp, but if the
hardware sends a truncated response (e.g., just the status byte), the code
blindly accesses the connection parameters and credit count from
uninitialized adjacent memory.

Does this need an skb->len check before parsing the structure?