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

[Simon Horman]

On Wed, Jun 24, 2026 at 12:33:21AM +0200, Sam P wrote:
> 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.

Thanks, I agree with your analysis.

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

Thanks, I agree this can be handled separately.

> > [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/20260424180151.3808557-2-snowwlake@xxxxxxxxxx/

Ack.

> > [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/20260623222402.175798-1-sam@xxxxxxxx/

Ack.

> > [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/20260612-b4-disp-6d52d8b0-v3-1-e26221f8826d@xxxxxxxxx/

Ack.

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

With the above in mind, this now looks good to me.

Reviewed-by: Simon Horman <horms@xxxxxxxxxx>