Re: [PATCH v2] nfc: nci: fix use of uninitialized memory in CORE_INIT_RSP parsing

[Simon Horman <horms@kernel.org>]

From: '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 use of uninitialized memory in CORE_INIT_RSP parsing

This commit adds bounds checking to nci_core_init_rsp_packet_v1() and
nci_core_init_rsp_packet_v2() to prevent reads of uninitialized memory
when parsing malformed CORE_INIT_RSP packets.

Link: https://syzkaller.appspot.com/bug?extid=46ca2592193f2fb3debc

> diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
> index 9eeb862825c5f..61fbe31b427eb 100644
> --- a/net/nfc/nci/rsp.c
> +++ b/net/nfc/nci/rsp.c
> @@ -50,11 +50,19 @@ static u8 nci_core_init_rsp_packet_v1(struct nci_dev *ndev,
[ ... ]
>  	ndev->nfcc_features = __le32_to_cpu(rsp_1->nfcc_features);
>  	ndev->num_supported_rf_interfaces = rsp_1->num_supported_rf_interfaces;
>  
>  	ndev->num_supported_rf_interfaces =
>  		min((int)ndev->num_supported_rf_interfaces,
>  		    NCI_MAX_SUPPORTED_RF_INTERFACES);
>  
> +	if (skb->len < sizeof(*rsp_1) + rsp_1->num_supported_rf_interfaces +
> +		       sizeof(*rsp_2))
> +		return NCI_STATUS_SYNTAX_ERROR;

Simon says: As noted, the issue flagged below pre-dates this patch.
	    I do not suggest increasing the scope of this patchset for
	    this.  But you may want to look at it in the context of
	    follow-up.

[Severity: High]
This isn't a bug introduced by this patch, but does temporarily updating
ndev->num_supported_rf_interfaces without holding ndev->req_lock create a
TOCTOU race?

The nci_core_init_rsp_packet_v1() function is processed asynchronously in
nci_rx_work(), temporarily setting ndev->num_supported_rf_interfaces to
the unclamped value from the packet before it is clamped to
NCI_MAX_SUPPORTED_RF_INTERFACES.

Concurrently, if a thread executing nci_init_complete_req() iterates over
ndev->supported_rf_interfaces using ndev->num_supported_rf_interfaces as
the limit, could it read past the bounds of the 4-element array?

Simon says: On the other hand, the issue flagged below seems to have
            been introduced by this patch and I'd appreciate it if
	    you could look over it.

Additionally, if the new length validation check returns early with
NCI_STATUS_SYNTAX_ERROR, doesn't it leave ndev->num_supported_rf_interfaces
updated but the array contents uninitialized, causing further state
corruption?

>  	memcpy(ndev->supported_rf_interfaces,
>  	       rsp_1->supported_rf_interfaces,
>  	       ndev->num_supported_rf_interfaces);
[ ... ]
> @@ -87,15 +99,26 @@ static u8 nci_core_init_rsp_packet_v2(struct nci_dev *ndev,
>  				      const struct sk_buff *skb)
>  {
>  	const struct nci_core_init_rsp_nci_ver2 *rsp = (void *)skb->data;
> -	const u8 *supported_rf_interface = rsp->supported_rf_interfaces;
> +	const u8 *supported_rf_interface;
> +	const u8 *skb_end = skb->data + skb->len;

Simon says: I believe the issue described below is a non-issue
            because I think that skbs are always linear in this context.

[Severity: Medium]
Does using skb->len to compute skb_end assume SKBs are strictly linear?

The skb->len field represents the total packet length, including any paged
fragments. If the SKB is nonlinear, skb_end will point past the end of the
linear buffer allocation.

Iterating through *supported_rf_interface++ up to skb_end could result in
out-of-bounds memory reads.

Could we use skb_tail_pointer() or pskb_may_pull() to guarantee contiguous
bytes in the linear region?

[ ... ]

Simon says: Like the first issue, I believe this one is pre-existing,
            should not effect the progress of this patch, but could
	    be looked at in the context of follow-up.

[Severity: High]
This is a pre-existing issue, but does lockless iteration and concurrent
modification of ndev->conn_info_list lead to linked list corruption and
use-after-free?

The ndev->conn_info_list is iterated locklessly by functions like
nci_get_conn_info_by_conn_id() (called from nci_tx_work()) and
nci_get_conn_info_by_dest_type_params() (called from nci_nfcc_loopback()).

Concurrently, nci_rx_work() can process an injected
NCI_OP_CORE_CONN_CLOSE_RSP packet via nci_core_conn_close_rsp_packet(),
which directly calls list_del() and devm_kfree() on the connection object
without acquiring any synchronization locks.

Because tx_work and rx_work operate on independent workqueues, couldn't an
injected packet free the object while it is actively being iterated by
another CPU, resulting in a use-after-free read and linked list corruption?