[PATCH v2] nfc: nci: add data_len bound checks to activation parameter extractors

[PATCH v2] nfc: nci: add data_len bound checks to activation parameter extractors

[Bryam Vargas]

nci_extract_activation_params_iso_dep() and
nci_extract_activation_params_nfc_dep() read an inner length byte from
the NCI RF_INTF_ACTIVATED_NTF payload and use it to memcpy() into fixed
kernel buffers, but neither function receives the caller-validated
activation_params_len.  A crafted NCI notification with
activation_params_len=1 and an inner length byte of up to 20 (NFC-A) or
50 (NFC-B) causes memcpy() to read that many bytes past the one valid
byte in the activation params region -- a slab out-of-bounds read of
kernel memory adjacent to the NCI skb.

The sibling nci_extract_rf_params_*() family was given equivalent
protection by commit 571dcbeb8e63 ("net: nfc: nci: Fix parameter
validation for packet data"), but the two activation parameter
extractors were not updated at that time.

Add a data_len parameter to both functions, guard against an empty
region before consuming the inner length byte, decrement the remaining
count after consuming it, and clamp the copy length to what is actually
available.  Update both call sites to pass ntf.activation_params_len,
which is already validated against the skb at ntf.c:801.

Fixes: e8c0dacd9836 ("NFC: Update names and structs to NCI spec 1.0 d18")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: no functional change. Resend of
    https://lore.kernel.org/all/20260607094822.322125-1-hexlabsecurity@proton.me/
    (2026-06-07), which did not reach the NFC maintainer: the MAINTAINERS
    address for NFC, david+nfc@ixit.cz, rejected delivery ("550 User
    doesn't exist" -- the +nfc subaddress expanded to
    #/Kernel/Linux/NFC@ixit.cz), so this is resent to david@ixit.cz. Also
    added the Cc: stable tag for the backport and dropped stable@vger from
    the direct Cc (it should follow the mainline merge, not precede it).

Verification (NFC-A ISO-DEP, NFC_ATS_MAXSIZE = 20):

  data_len  inner_len  without patch              with patch
  --------  ---------  -------------------------  --------------------
  1         0          rats_res_len=0, clean       same
  1         1          memcpy +1B OOB              clamped to 0, clean
  1         20         memcpy +20B OOB  <-- PoC   clamped to 0, clean
  2         2          memcpy +1B OOB              clamped to 1, clean
  21        20         memcpy 20B clean            same

NFC-B (attrib_res, max 50) and NFC-DEP poll/listen (atr_res/atr_req,
max 62) have the same shape and receive the same fix.

 net/nfc/nci/ntf.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
index c96512bb8653..753f4cf08748 100644
--- a/net/nfc/nci/ntf.c
+++ b/net/nfc/nci/ntf.c
@@ -525,15 +525,19 @@ static int nci_rf_discover_ntf_packet(struct nci_dev *ndev,

 static int nci_extract_activation_params_iso_dep(struct nci_dev *ndev,
 						 struct nci_rf_intf_activated_ntf *ntf,
-						 const __u8 *data)
+						 const __u8 *data, __u8 data_len)
 {
 	struct activation_params_nfca_poll_iso_dep *nfca_poll;
 	struct activation_params_nfcb_poll_iso_dep *nfcb_poll;

 	switch (ntf->activation_rf_tech_and_mode) {
 	case NCI_NFC_A_PASSIVE_POLL_MODE:
+		if (data_len < 1)
+			return -EINVAL;
 		nfca_poll = &ntf->activation_params.nfca_poll_iso_dep;
 		nfca_poll->rats_res_len = min_t(__u8, *data++, NFC_ATS_MAXSIZE);
+		data_len--;
+		nfca_poll->rats_res_len = min_t(__u8, nfca_poll->rats_res_len, data_len);
 		pr_debug("rats_res_len %d\n", nfca_poll->rats_res_len);
 		if (nfca_poll->rats_res_len > 0) {
 			memcpy(nfca_poll->rats_res,
@@ -542,8 +546,12 @@ static int nci_extract_activation_params_iso_dep(struct nci_dev *ndev,
 		break;

 	case NCI_NFC_B_PASSIVE_POLL_MODE:
+		if (data_len < 1)
+			return -EINVAL;
 		nfcb_poll = &ntf->activation_params.nfcb_poll_iso_dep;
 		nfcb_poll->attrib_res_len = min_t(__u8, *data++, 50);
+		data_len--;
+		nfcb_poll->attrib_res_len = min_t(__u8, nfcb_poll->attrib_res_len, data_len);
 		pr_debug("attrib_res_len %d\n", nfcb_poll->attrib_res_len);
 		if (nfcb_poll->attrib_res_len > 0) {
 			memcpy(nfcb_poll->attrib_res,
@@ -562,7 +570,7 @@ static int nci_extract_activation_params_iso_dep(struct nci_dev *ndev,

 static int nci_extract_activation_params_nfc_dep(struct nci_dev *ndev,
 						 struct nci_rf_intf_activated_ntf *ntf,
-						 const __u8 *data)
+						 const __u8 *data, __u8 data_len)
 {
 	struct activation_params_poll_nfc_dep *poll;
 	struct activation_params_listen_nfc_dep *listen;
@@ -570,9 +578,13 @@ static int nci_extract_activation_params_nfc_dep(struct nci_dev *ndev,
 	switch (ntf->activation_rf_tech_and_mode) {
 	case NCI_NFC_A_PASSIVE_POLL_MODE:
 	case NCI_NFC_F_PASSIVE_POLL_MODE:
+		if (data_len < 1)
+			return -EINVAL;
 		poll = &ntf->activation_params.poll_nfc_dep;
 		poll->atr_res_len = min_t(__u8, *data++,
 					  NFC_ATR_RES_MAXSIZE - 2);
+		data_len--;
+		poll->atr_res_len = min_t(__u8, poll->atr_res_len, data_len);
 		pr_debug("atr_res_len %d\n", poll->atr_res_len);
 		if (poll->atr_res_len > 0)
 			memcpy(poll->atr_res, data, poll->atr_res_len);
@@ -580,9 +592,13 @@ static int nci_extract_activation_params_nfc_dep(struct nci_dev *ndev,

 	case NCI_NFC_A_PASSIVE_LISTEN_MODE:
 	case NCI_NFC_F_PASSIVE_LISTEN_MODE:
+		if (data_len < 1)
+			return -EINVAL;
 		listen = &ntf->activation_params.listen_nfc_dep;
 		listen->atr_req_len = min_t(__u8, *data++,
 					    NFC_ATR_REQ_MAXSIZE - 2);
+		data_len--;
+		listen->atr_req_len = min_t(__u8, listen->atr_req_len, data_len);
 		pr_debug("atr_req_len %d\n", listen->atr_req_len);
 		if (listen->atr_req_len > 0)
 			memcpy(listen->atr_req, data, listen->atr_req_len);
@@ -806,12 +822,14 @@ static int nci_rf_intf_activated_ntf_packet(struct nci_dev *ndev,
 		switch (ntf.rf_interface) {
 		case NCI_RF_INTERFACE_ISO_DEP:
 			err = nci_extract_activation_params_iso_dep(ndev,
-								    &ntf, data);
+								    &ntf, data,
+								    ntf.activation_params_len);
 			break;

 		case NCI_RF_INTERFACE_NFC_DEP:
 			err = nci_extract_activation_params_nfc_dep(ndev,
-								    &ntf, data);
+								    &ntf, data,
+								    ntf.activation_params_len);
 			break;

 		case NCI_RF_INTERFACE_FRAME:
--
2.43.0

Re: [PATCH v2] nfc: nci: add data_len bound checks to activation parameter extractors

[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

Simon says:

The error type issue below - the first issue - does seem of
concern in the context of this patch.

While I think the other issues, which are flagged as pre-existing,
can be considered in the context of possible follow-up.

---
nfc: nci: add data_len bound checks to activation parameter extractors

This patch adds data_len bounds checking to the NCI activation parameter
extractors to prevent slab out-of-bounds reads. It introduces a data_len
parameter to guard against empty regions before consuming inner length
bytes.

> diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
> index c96512bb86531..753f4cf087489 100644
> --- a/net/nfc/nci/ntf.c
> +++ b/net/nfc/nci/ntf.c
> @@ -525,15 +525,19 @@ static int nci_rf_discover_ntf_packet(struct nci_dev *ndev,
>  
>  static int nci_extract_activation_params_iso_dep(struct nci_dev *ndev,
>  						 struct nci_rf_intf_activated_ntf *ntf,
> -						 const __u8 *data)
> +						 const __u8 *data, __u8 data_len)
>  {
>  	struct activation_params_nfca_poll_iso_dep *nfca_poll;
>  	struct activation_params_nfcb_poll_iso_dep *nfcb_poll;
>  
>  	switch (ntf->activation_rf_tech_and_mode) {
>  	case NCI_NFC_A_PASSIVE_POLL_MODE:
> +		if (data_len < 1)
> +			return -EINVAL;

[Severity: Medium]
Is the correct error type being returned here?

nci_extract_activation_params_iso_dep() is called by
nci_rf_intf_activated_ntf_packet() and its return value is assigned to err,
which is later passed to nci_req_complete(ndev, err).

nci_req_complete() expects an NCI_STATUS_* positive integer constant.
If the POSIX error code -EINVAL (-22) is returned, nci_to_errno() will
truncate it to 234. It will then be incorrectly translated to -ENOSYS
through the default case of its switch statement.

Should a valid NCI status code like NCI_STATUS_INVALID_PARAM or
NCI_STATUS_RF_PROTOCOL_ERROR be used instead? The same behavior appears
in the updated nci_extract_activation_params_nfc_dep() below.

>  		nfca_poll = &ntf->activation_params.nfca_poll_iso_dep;
>  		nfca_poll->rats_res_len = min_t(__u8, *data++, NFC_ATS_MAXSIZE);

[ ... ]

> @@ -806,12 +822,14 @@ static int nci_rf_intf_activated_ntf_packet(struct nci_dev *ndev,
>  		switch (ntf.rf_interface) {
>  		case NCI_RF_INTERFACE_ISO_DEP:
>  			err = nci_extract_activation_params_iso_dep(ndev,
> -								    &ntf, data);
> +								    &ntf, data,
> +								    ntf.activation_params_len);

[Severity: Critical]
This isn't a bug introduced by this patch, but could the parsing of
atr_res_len allow for a stack memory disclosure?

In nci_rf_intf_activated_ntf_packet(), the ntf structure is allocated on
the stack without zero-initialization.

If a malformed packet specifies an activation_params_len >= 1 but the inner
atr_res_len is clamped to less than 15 (NFC_ATR_RES_GT_OFFSET), would this
cause an integer underflow?

net/nfc/nci/ntf.c:nci_store_general_bytes_nfc_dep() {
    ...
    ndev->remote_gb_len = min_t(__u8,
        (ntf->activation_params.poll_nfc_dep.atr_res_len
            - NFC_ATR_RES_GT_OFFSET),
        NFC_ATR_RES_GB_MAXSIZE);
    ...
    memcpy(ndev->remote_gb,
           (ntf->activation_params.poll_nfc_dep.atr_res
            + NFC_ATR_RES_GT_OFFSET),
           ndev->remote_gb_len);
}

Because __u8 is unsigned, a negative result from the subtraction underflows
to a large positive value, which min_t() then clamps to
NFC_ATR_RES_GB_MAXSIZE (47).

Could this result in copying up to 47 uninitialized stack bytes into
ndev->remote_gb, which might later be broadcast to userspace?

[Severity: High]
This is a pre-existing issue, but is there also a 1-byte stack disclosure
in the NFC-A passive poll parameter parsing?

In nci_extract_rf_params_nfca_passive_poll(), sel_res is only initialized
if sel_res_len != 0:

net/nfc/nci/ntf.c:nci_extract_rf_params_nfca_passive_poll() {
    if (nfca_poll->sel_res_len != 0) {
        if (data_len < 1)
            return ERR_PTR(-EINVAL);
        nfca_poll->sel_res = *data++;
    }
}

If sel_res_len is 0, nfca_poll->sel_res remains uninitialized. Because
nfca_poll points into the local uninitialized ntf stack variable in
nci_rf_intf_activated_ntf_packet(), it retains whatever garbage was on
the stack.

Later, nci_add_new_protocol() unconditionally copies this uninitialized byte:

net/nfc/nci/ntf.c:nci_add_new_protocol() {
    ...
    target->sel_res = nfca_poll->sel_res;
    ...
}

When userspace queries the targets, this byte is sent out unconditionally:

net/nfc/netlink.c:nfc_genl_send_target() {
    ...
    if (nla_put_u8(msg, NFC_ATTR_TARGET_SEL_RES, target->sel_res))
    ...
}

Would it be safer to zero-initialize the ntf structure at the start of
nci_rf_intf_activated_ntf_packet(), or set sel_res to 0 when sel_res_len
is 0?

>  			break;
>  
>  		case NCI_RF_INTERFACE_NFC_DEP:

Re: [PATCH v2] nfc: nci: add data_len bound checks to activation parameter extractors

[Bryam Vargas]

Thanks for the review. v3 is posted as a new top-level thread:
https://lore.kernel.org/all/20260612-b4-disp-6d52d8b0-v3-1-ae1f21cdd8ab@proton.me/
Point-by-point below.

> [Severity: Medium] Is the correct error type being returned here?
> [...] -EINVAL [...] nci_to_errno() will truncate it [...] -ENOSYS [...]
> Should a valid NCI status code [...] be used instead?

Correct. These extractors return into the int 'err' in
nci_rf_intf_activated_ntf_packet(), which is handed to nci_req_complete()
and later run through nci_to_errno(). nci_to_errno() takes a __u8, so the
-EINVAL is truncated (to 234) at that argument and falls through to the
default case as -ENOSYS. The contract here is an NCI_STATUS_* code: 'err'
is initialized to NCI_STATUS_OK and the functions' own default cases
already return NCI_STATUS_RF_PROTOCOL_ERROR.

v3 returns NCI_STATUS_RF_PROTOCOL_ERROR from the new short-region guards,
which matches the sibling default cases and maps to -EPROTO via
nci_to_errno(). NCI_STATUS_INVALID_PARAM is equally valid (also -EPROTO) --
I went with RF_PROTOCOL_ERROR for consistency with the surrounding code;
glad to switch if you prefer INVALID_PARAM. v3 is otherwise identical to v2
and is posted as a new top-level thread.

> [Severity: Critical] [...] atr_res_len [...] integer underflow [...]
> up to 47 uninitialized stack bytes into ndev->remote_gb [...] broadcast
> to userspace?

The uninitialized-stack read is real: in nci_store_general_bytes_nfc_dep()
an atr_res_len below NFC_ATR_RES_GT_OFFSET makes min_t() clamp to
NFC_ATR_RES_GB_MAXSIZE and memcpy() copies stale bytes of the on-stack
atr_res[] into ndev->remote_gb. I could not, however, confirm the
"broadcast to userspace" part: remote_gb has no netlink attribute (there is
no NFC_ATTR_TARGET_GENERAL_BYTES), and its only path out is LLCP, which is
gated by the 3-byte llcp_magic memcmp on those same uninitialized bytes --
so a deterministic leak of the 47 bytes isn't reachable that way. It's
still a genuine uninitialized read worth closing; see below.

> [Severity: High] [...] sel_res [...] uninitialized [...] nci_add_new_protocol()
> [...] nfc_genl_send_target() [...] sent out unconditionally [...]

Confirmed, and this one does reach userspace: with sel_res_len == 0,
nci_extract_rf_params_nfca_passive_poll() leaves sel_res unwritten in the
on-stack ntf, nci_add_new_protocol() copies it unconditionally, and
nfc_genl_send_target() puts it into NFC_ATTR_TARGET_SEL_RES on
NFC_CMD_GET_TARGET -- a one-byte stack disclosure (the discover-NTF path
reaches it without the err==NCI_STATUS_OK gate).

Both pre-existing issues are closed by zero-initializing the on-stack
notification structs:

    struct nci_rf_discover_ntf ntf = {};        /* nci_rf_discover_ntf_packet */
    struct nci_rf_intf_activated_ntf ntf = {};  /* nci_rf_intf_activated_ntf_packet */

I'll send that as a separate patch rather than fold it into this one, since
it's an independent change in different functions.