[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) {

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

[Doruk (0sec)]

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

Thanks for the review Simon! I went with the first option in a separate patch,
since I believe the missing minimum-length check affects every PDU type the
dispatcher hands off, not just CONNECT/CC:

[PATCH net] nfc: llcp: guard against short PDUs in nfc_llcp_rx_skb()

It adds "if (skb->len < LLCP_HEADER_SIZE) return;" at the top of
nfc_llcp_rx_skb(), so no handler can see a sub-header PDU and the
"skb->len - LLCP_HEADER_SIZE" subtraction can no longer wrap. With that
in place the connection-TLV parser's bound is safe. Happy to respin
either patch if you'd prefer both changes squashed into one.

Best,
Doruk

On Fri, Jul 10, 2026 03:49 PM, Simon Horman <horms@kernel.org> wrote:
>
> 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) {

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

[Simon Horman]

On Sat, Jul 11, 2026 at 03:42:10PM +0200, Doruk (0sec) wrote:
> > 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()?
> 
> Thanks for the review Simon! I went with the first option in a separate patch,
> since I believe the missing minimum-length check affects every PDU type the
> dispatcher hands off, not just CONNECT/CC:
> 
> [PATCH net] nfc: llcp: guard against short PDUs in nfc_llcp_rx_skb()
> 
> It adds "if (skb->len < LLCP_HEADER_SIZE) return;" at the top of
> nfc_llcp_rx_skb(), so no handler can see a sub-header PDU and the
> "skb->len - LLCP_HEADER_SIZE" subtraction can no longer wrap. With that
> in place the connection-TLV parser's bound is safe. Happy to respin
> either patch if you'd prefer both changes squashed into one.

Thanks, a separate patch sounds like a reasonable approach to me.

...