Re: [PATCH net] nfc: nci: fix out-of-bounds write in nci_target_auto_activated()

[Simon Horman]

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 out-of-bounds write in nci_target_auto_activated()

This commit adds a bounds check to nci_target_auto_activated() to prevent
an out-of-bounds array write. If ndev->n_targets equals
NCI_MAX_DISCOVERED_TARGETS, it prints a debug message and returns early.

> diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
> index c96512bb86531..566ca839fa488 100644
> --- a/net/nfc/nci/ntf.c
> +++ b/net/nfc/nci/ntf.c
> @@ -603,6 +603,12 @@ static void nci_target_auto_activated(struct nci_dev *ndev,
>  	struct nfc_target *target;
>  	int rc;
>  
> +	/* This is a new target, check if we've enough room */
> +	if (ndev->n_targets == NCI_MAX_DISCOVERED_TARGETS) {
> +		pr_debug("not enough room, ignoring new target...\n");
> +		return;

[Severity: High]
Does this early return cause the state machine to stall?

Looking at nci_rf_intf_activated_ntf_packet(), the state is transitioned
to NCI_POLL_ACTIVE right before this function is called:

net/nfc/nci/ntf.c:nci_rf_intf_activated_ntf_packet() {
    ...
    atomic_set(&ndev->state, NCI_POLL_ACTIVE);
    if (err == NCI_STATUS_OK)
        nci_target_auto_activated(ndev, &ntf);
    ...
}

Because of the early return here, nfc_targets_found() is skipped. Does this
leave the state wedged in NCI_POLL_ACTIVE, permanently blocking future 
attempts to stop polling since nci_stop_poll() requires the state to be 
NCI_DISCOVERY?

> +	}
> +
>  	target = &ndev->targets[ndev->n_targets];

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but can the ntf union copy
uninitialized stack memory into persistent device structures?

In net/nfc/nci/ntf.c:nci_rf_discover_ntf_packet(), the local ntf struct
is declared on the stack:

    struct nci_rf_discover_ntf ntf;

If the incoming packet has ntf.rf_tech_specific_params_len == 0, the
extraction routines are bypassed, leaving the rf_tech_specific_params union
uninitialized. The struct is then passed to nci_add_new_target():

    if (add_target == true)
        nci_add_new_target(ndev, &ntf);

This eventually calls nci_add_new_protocol(), which blindly reads from this
union:

    nfca_poll = (struct rf_tech_specific_params_nfca_poll *)params;
    target->sens_res = nfca_poll->sens_res;
    target->sel_res = nfca_poll->sel_res;
    target->nfcid1_len = nfca_poll->nfcid1_len;

Could this uninitialized stack garbage be copied into ndev->targets, and
later broadcasted to userspace via Netlink in nfc_targets_found()?

[Severity: Critical]
This isn't a bug introduced by this patch, but does the general bytes length
calculation underflow?

In net/nfc/nci/ntf.c:nci_store_general_bytes_nfc_dep():

    ndev->remote_gb_len = min_t(__u8, 
        (ntf->activation_params.listen_nfc_dep.atr_req_len - 
         NFC_ATR_REQ_GT_OFFSET), NFC_ATR_REQ_GB_MAXSIZE);
         
    memcpy(ndev->remote_gb, 
        (ntf->activation_params.listen_nfc_dep.atr_req + 
         NFC_ATR_REQ_GT_OFFSET), ndev->remote_gb_len);

The length is computed by subtracting NFC_ATR_REQ_GT_OFFSET (14) from
atr_req_len. If atr_req_len is less than 14, this subtraction will
underflow.

Because of the min_t cast, this large underflowed value is bounded to
NFC_ATR_REQ_GB_MAXSIZE (48). The memcpy() then copies 48 bytes starting
from atr_req + 14 into ndev->remote_gb.

Since the source buffer is part of the ntf union allocated on the stack
in nci_rf_intf_activated_ntf_packet(), could this leak up to 48 bytes of
uninitialized kernel stack memory to userspace?

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of skb->len
validation in NCI response handlers allow out-of-bounds reads?

In nci_core_init_rsp_packet_v1() in net/nfc/nci/rsp.c, fields are read
and arrays are copied from the payload:

    ndev->num_supported_rf_interfaces = 
        rsp_1->num_supported_rf_interfaces;
        
    memcpy(ndev->supported_rf_interfaces, rsp_1->supported_rf_interfaces, 
           ndev->num_supported_rf_interfaces);

Similarly in nci_core_init_rsp_packet_v2():

    while (rf_interface_idx < ndev->num_supported_rf_interfaces) {
        ndev->supported_rf_interfaces[rf_interface_idx++] = 
            *supported_rf_interface++;
    }

Since the NCI control header is stripped via skb_pull(), leaving the
payload length in skb->len, what happens if an NFCC sends a short response
packet (e.g. plen = 1)? Could this cause the handlers to read adjacent
kernel slab memory?

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing boundary
validation when extracting activation parameters?

In net/nfc/nci/ntf.c:nci_extract_activation_params_iso_dep():

    nfca_poll->rats_res_len = min_t(__u8, *data++, NFC_ATS_MAXSIZE);
    if (nfca_poll->rats_res_len > 0) {
        memcpy(nfca_poll->rats_res, data, nfca_poll->rats_res_len);
    }

And in nci_extract_activation_params_nfc_dep():

    poll->atr_res_len = min_t(__u8, *data++, NFC_ATR_RES_MAXSIZE - 2);
    if (poll->atr_res_len > 0)
        memcpy(poll->atr_res, data, poll->atr_res_len);

These inner lengths are bounded by a maximum size like NFC_ATS_MAXSIZE, but
they are not validated against the remaining bytes in the SKB (skb->len).

If a malformed packet provides a large inner length but a short payload,
could this memcpy() read out of bounds into the device structure,
potentially leaking adjacent slab memory to userspace later?

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential
priority inversion or timeout deadlock involving the device lock?

Functions like nfc_stop_poll() in net/nfc/core.c acquire the device lock:

    device_lock(&dev->dev);
    ...
    if (dev->ops->stop_poll)
        dev->ops->stop_poll(dev);

This eventually calls nci_request(), which synchronously waits for a
completion signaled by the nci_rx_work thread. However, if an NTF packet
is received first, nci_rx_work processes it and invokes
nfc_targets_found(), which also attempts to acquire the device lock:

    device_lock(&dev->dev);

Since the calling thread already holds the device lock, nci_rx_work blocks
indefinitely. Because the RX worker is blocked, it cannot process the
pending RSP, causing nci_request() to time out and fail. Could this
deadlock the RX thread?