[PATCH net] nfc: llcp: bound the remaining LLCP TLV parsers to their buffers

[PATCH net] nfc: llcp: bound the remaining LLCP TLV parsers to their buffers

[Doruk Tan Ozturk]

Commit 27256cdb290e ("nfc: llcp: bound SNL TLV parsing to the skb and
add length checks") fixed the unbounded TLV walk in
nfc_llcp_recv_snl(), but three sibling parsers that share the exact
same pattern were left unbounded:

  - nfc_llcp_parse_gb_tlv()
  - nfc_llcp_parse_connection_tlv()
  - nfc_llcp_connect_sn()

Each walks a TLV list, reading a two-byte header (type, length)
followed by length bytes of value, without checking that the two
header bytes or the declared length stay within the buffer.
nfc_llcp_connect_sn() then returns a pointer to a service name of up to
255 bytes that may point past the end of the skb; it is subsequently
consumed by memcmp() in nfc_llcp_sock_from_sn().

nfc_llcp_parse_connection_tlv() is worse: it tracks the walk offset in
a u8, so a single crafted TLV with length == 254 advances the offset by
256, which wraps to 0. The loop condition "offset < tlv_array_len" then
never makes progress while the tlv pointer keeps marching forward,
producing an infinite loop with a runaway out-of-bounds read and a
guaranteed oops even without KASAN.

nfc_llcp_parse_connection_tlv() and nfc_llcp_connect_sn() are reachable
from nfc_llcp_recv_connect() and nfc_llcp_recv_cc(), i.e. from received
CONNECT and CC PDUs. A nearby NFC device can reach this without
authentication; LLCP link activation happens automatically after
NFC-DEP, and the nfc_llcp_rx_skb() dispatcher applies no minimum-length
guard.

Walk each TLV list by pointer, bounded by the end of the buffer
(skb_tail_pointer() for connect_sn, tlv_array + tlv_array_len for the
gb and connection parsers), and validate each declared length before
use, matching the approach already used for nfc_llcp_recv_snl().
Dropping the u8 offset also removes the wrap, and for very short
connect frames this avoids the size_t underflow of
"skb->len - LLCP_HEADER_SIZE".

Found by 0sec automated security-research tooling (https://0sec.ai).

Fixes: d646960f7986 ("NFC: Initial LLCP support")
Cc: stable@vger.kernel.org
Assisted-by: 0sec:claude-opus-4-8
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
 net/nfc/llcp_commands.c | 18 ++++++++++++------
 net/nfc/llcp_core.c     | 10 ++++++----
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c
index 291f26facbf3..1a0a2f4aca70 100644
--- a/net/nfc/llcp_commands.c
+++ b/net/nfc/llcp_commands.c
@@ -193,17 +193,21 @@ int nfc_llcp_parse_gb_tlv(struct nfc_llcp_local *local,
 			  const u8 *tlv_array, u16 tlv_array_len)
 {
 	const u8 *tlv = tlv_array;
-	u8 type, length, offset = 0;
+	const u8 *tlv_end = tlv_array + tlv_array_len;
+	u8 type, length;
 
 	pr_debug("TLV array length %d\n", tlv_array_len);
 
 	if (local == NULL)
 		return -ENODEV;
 
-	while (offset < tlv_array_len) {
+	while (tlv + 2 < tlv_end) {
 		type = tlv[0];
 		length = tlv[1];
 
+		if (tlv + 2 + length > tlv_end)
+			break;
+
 		pr_debug("type 0x%x length %d\n", type, length);
 
 		switch (type) {
@@ -227,7 +231,6 @@ int nfc_llcp_parse_gb_tlv(struct nfc_llcp_local *local,
 			break;
 		}
 
-		offset += length + 2;
 		tlv += length + 2;
 	}
 
@@ -243,17 +246,21 @@ int nfc_llcp_parse_connection_tlv(struct nfc_llcp_sock *sock,
 				  const u8 *tlv_array, u16 tlv_array_len)
 {
 	const u8 *tlv = tlv_array;
-	u8 type, length, offset = 0;
+	const u8 *tlv_end = tlv_array + tlv_array_len;
+	u8 type, length;
 
 	pr_debug("TLV array length %d\n", tlv_array_len);
 
 	if (sock == NULL)
 		return -ENOTCONN;
 
-	while (offset < tlv_array_len) {
+	while (tlv + 2 < tlv_end) {
 		type = tlv[0];
 		length = tlv[1];
 
+		if (tlv + 2 + length > tlv_end)
+			break;
+
 		pr_debug("type 0x%x length %d\n", type, length);
 
 		switch (type) {
@@ -270,7 +277,6 @@ int nfc_llcp_parse_connection_tlv(struct nfc_llcp_sock *sock,
 			break;
 		}
 
-		offset += length + 2;
 		tlv += length + 2;
 	}
 
diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index aed5fe1afef0..0de20279e046 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -849,13 +849,16 @@ static struct nfc_llcp_sock *nfc_llcp_sock_get_sn(struct nfc_llcp_local *local,
 static const u8 *nfc_llcp_connect_sn(const struct sk_buff *skb, size_t *sn_len)
 {
 	u8 type, length;
-	const u8 *tlv = &skb->data[2];
-	size_t tlv_array_len = skb->len - LLCP_HEADER_SIZE, offset = 0;
+	const u8 *tlv = &skb->data[LLCP_HEADER_SIZE];
+	const u8 *tlv_end = skb_tail_pointer(skb);
 
-	while (offset < tlv_array_len) {
+	while (tlv + 2 < tlv_end) {
 		type = tlv[0];
 		length = tlv[1];
 
+		if (tlv + 2 + length > tlv_end)
+			break;
+
 		pr_debug("type 0x%x length %d\n", type, length);
 
 		if (type == LLCP_TLV_SN) {
@@ -863,7 +866,6 @@ static const u8 *nfc_llcp_connect_sn(const struct sk_buff *skb, size_t *sn_len)
 			return &tlv[2];
 		}
 
-		offset += length + 2;
 		tlv += length + 2;
 	}
 
-- 
2.43.0

Re: [PATCH net] nfc: llcp: bound the remaining LLCP TLV parsers to their buffers

[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://netdev-ai.bots.linux.dev/sashiko/
---
nfc: llcp: bound the remaining LLCP TLV parsers to their buffers

The commit adds pointer-based bounds to nfc_llcp_parse_gb_tlv(),
nfc_llcp_parse_connection_tlv() and nfc_llcp_connect_sn(), replacing
the previous u8 offset walk which could wrap and produce an unbounded
TLV loop. It cites the size_t underflow of "skb->len - LLCP_HEADER_SIZE"
as one of the failure modes it wants to close.

> diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c
> index 291f26facbf3..1a0a2f4aca70 100644
> --- a/net/nfc/llcp_commands.c
> +++ b/net/nfc/llcp_commands.c

[ ... ]

> @@ -243,17 +246,21 @@ int nfc_llcp_parse_connection_tlv(struct nfc_llcp_sock *sock,
>  				  const u8 *tlv_array, u16 tlv_array_len)
>  {
>  	const u8 *tlv = tlv_array;
> -	u8 type, length, offset = 0;
> +	const u8 *tlv_end = tlv_array + tlv_array_len;
> +	u8 type, length;
>  
>  	pr_debug("TLV array length %d\n", tlv_array_len);
>  
>  	if (sock == NULL)
>  		return -ENOTCONN;
>  
> -	while (offset < tlv_array_len) {
> +	while (tlv + 2 < tlv_end) {
>  		type = tlv[0];
>  		length = tlv[1];
>  
> +		if (tlv + 2 + length > tlv_end)
> +			break;
> +

[High]
Is the size_t underflow the commit message mentions actually addressed
for nfc_llcp_parse_connection_tlv()?

The two callers, nfc_llcp_recv_connect() and nfc_llcp_recv_cc() in
net/nfc/llcp_core.c, still compute the length at the call site as
skb->len - LLCP_HEADER_SIZE and hand it to the parser:

    nfc_llcp_parse_connection_tlv(new_sock, &skb->data[LLCP_HEADER_SIZE],
                                  skb->len - LLCP_HEADER_SIZE);

and

    nfc_llcp_parse_connection_tlv(llcp_sock, &skb->data[LLCP_HEADER_SIZE],
                                  skb->len - LLCP_HEADER_SIZE);

If skb->len is 0 or 1, that subtraction (size_t at the call site,
truncated to the u16 tlv_array_len parameter) becomes 0xFFFE or 0xFFFF.

Inside the parser, that value now feeds:

    const u8 *tlv_end = tlv_array + tlv_array_len;

    while (tlv + 2 < tlv_end) {
            ...
            if (tlv + 2 + length > tlv_end)
                    break;

so tlv_end points roughly 64 KiB past &skb->data[LLCP_HEADER_SIZE] and
neither the loop condition nor the per-TLV check catches the walk
leaving the skb.

The commit message calls this out as one of the motivations:

    "Dropping the u8 offset also removes the wrap, and for very short
    connect frames this avoids the size_t underflow of
    'skb->len - LLCP_HEADER_SIZE'."

That mitigation is applied inside nfc_llcp_connect_sn() by using
skb_tail_pointer(skb) directly, but the equivalent change is not made
for the connection TLV path. nfc_llcp_rx_skb() has no minimum-length
guard, so a received CONNECT or CC PDU with skb->len < LLCP_HEADER_SIZE
still reaches these callers.

Would it make sense to either guard skb->len >= LLCP_HEADER_SIZE
before the two call sites, or take the same skb_tail_pointer() based
approach used in nfc_llcp_connect_sn() so the parser derives its bound
from the skb rather than from an untrusted subtraction?

> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index dc65c719f35f..5e1881199238 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -849,13 +849,16 @@ static struct nfc_llcp_sock *nfc_llcp_sock_get_sn(struct nfc_llcp_local *local,
>  static const u8 *nfc_llcp_connect_sn(const struct sk_buff *skb, size_t *sn_len)
>  {
>  	u8 type, length;
> -	const u8 *tlv = &skb->data[2];
> -	size_t tlv_array_len = skb->len - LLCP_HEADER_SIZE, offset = 0;
> +	const u8 *tlv = &skb->data[LLCP_HEADER_SIZE];
> +	const u8 *tlv_end = skb_tail_pointer(skb);
>  
> -	while (offset < tlv_array_len) {
> +	while (tlv + 2 < tlv_end) {