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

On 23/06/2026 19:21, Simon Horman wrote:
>> 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];
>
> [ ... ]

Thanks for sharing the review.

I don't believe the early return can stall the state machine, as
nci_target_auto_activated() does not touch ndev->state.

nci_rf_intf_activated_ntf_packet() sets ndev->state = NCI_POLL_ACTIVE
before it calls nci_target_auto_activated(), so the transition has
already happened by the time the new check runs. Neither
nci_target_auto_activated() nor nfc_targets_found() modifies ndev->state;
nfc_targets_found() only clears the nfc_dev dev->polling flag under
device_lock, which is independent of the NCI state machine that
nci_stop_poll() tests. So ndev->state is NCI_POLL_ACTIVE after an
auto-activation whether the target is appended or the guard returns
early; the state behaviour is the same with or without this patch.

The check is the same n_targets == NCI_MAX_DISCOVERED_TARGETS guard the
sibling nci_add_new_target() (the RF_DISCOVER_NTF path) has used since
the Fixes: commit; it only drops the out-of-spec 11th+ target, which a
conformant controller shouldn't produce. The device still returns to
NCI_DISCOVERY/NCI_IDLE through the normal RF_DEACTIVATE path.

> [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()?

I managed to repro this issue locally via KMSAN, I can queue up a patch
for this that I can submit separately, ntf just needs to be zero-initd.

> [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?

This one is already being fixed on the list:

[PATCH net v4 1/5] nfc: nci: fix u8 underflow in
nci_store_general_bytes_nfc_dep  (Lekë Hapçiu)
https://lore.kernel.org/all/[email protected]/

> [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?

I've since submitted a fix for this issue:

[PATCH net] nfc: nci: fix uninit-value in
nci_core_init_rsp_packet()
https://lore.kernel.org/netdev/[email protected]/

> [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?

This one is already being fixed on the list too:

[PATCH v3] nfc: nci: add data_len bound checks to activation
parameter extractors  (Bryam Vargas)
https://lore.kernel.org/all/[email protected]/

> [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?

No patch for this one, although I'm not sure how accurate it is.

Thanks,
Sam