[PATCH] nfc: llcp: validate PDU size in nfc_llcp_recv_dm() and nfc_llcp_recv_hdlc()

[PATCH] nfc: llcp: validate PDU size in nfc_llcp_recv_dm() and nfc_llcp_recv_hdlc()

[Aamir Ahmed]

nfc_llcp_recv_dm() reads the reason byte at skb->data[2], and
nfc_llcp_recv_hdlc() reads the sequence byte at the same offset
through nfc_llcp_ns() and nfc_llcp_nr().  Both run after
__nfc_llcp_recv(), which only guarantees LLCP_HEADER_SIZE (2) bytes
via pskb_may_pull().

A malformed PDU that is exactly two bytes long -- delivered directly by
the NFC controller, or as an aggregated PDU inside an AGF frame whose
inner length field is 2 -- causes both handlers to read one byte past
the guaranteed data.  In the direct case the byte is whatever follows
the valid payload in the skb buffer.  In the AGF case the inner skb is
allocated by nfc_alloc_recv_skb() with exactly two bytes of payload, so
the read is past the meaningful data and into whatever the slab
allocator left there.

For DM, the stale reason byte selects between NOBOUND/REJ and the
default socket-lookup path, potentially closing the wrong socket.  For
HDLC, the stale sequence byte corrupts N(S)/N(R) tracking: a bogus
N(R) drains the tx_pending_queue unconditionally, dropping in-flight I
frames and breaking the connection.

Add a minimum-length check to each handler: DM requires
LLCP_HEADER_SIZE + 1 (the reason byte) and HDLC requires
LLCP_HEADER_SIZE + LLCP_SEQUENCE_SIZE.

Fixes: d646960f7986 ("NFC: Initial LLCP support")
Cc: stable@vger.kernel.org
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
 net/nfc/llcp_core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index cac1b5487064..89d6f4599e11 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -1077,6 +1077,12 @@ static void nfc_llcp_recv_hdlc(struct nfc_llcp_local *local,
 	ptype = nfc_llcp_ptype(skb);
 	dsap = nfc_llcp_dsap(skb);
 	ssap = nfc_llcp_ssap(skb);
+
+	if (skb->len < LLCP_HEADER_SIZE + LLCP_SEQUENCE_SIZE) {
+		nfc_llcp_send_dm(local, dsap, ssap, LLCP_DM_NOCONN);
+		return;
+	}
+
 	ns = nfc_llcp_ns(skb);
 	nr = nfc_llcp_nr(skb);
 
@@ -1254,6 +1260,10 @@ static void nfc_llcp_recv_dm(struct nfc_llcp_local *local,
 
 	dsap = nfc_llcp_dsap(skb);
 	ssap = nfc_llcp_ssap(skb);
+
+	if (skb->len < LLCP_HEADER_SIZE + 1)
+		return;
+
 	reason = skb->data[2];
 
 	pr_debug("%d %d reason %d\n", ssap, dsap, reason);
-- 
2.43.0

Re: [PATCH] nfc: llcp: validate PDU size in nfc_llcp_recv_dm() and nfc_llcp_recv_hdlc()

[Greg KH]

On Mon, Sep 07, 2026 at 12:36:45AM +0100, Aamir Ahmed wrote:
> nfc_llcp_recv_dm() reads the reason byte at skb->data[2], and
> nfc_llcp_recv_hdlc() reads the sequence byte at the same offset
> through nfc_llcp_ns() and nfc_llcp_nr().  Both run after
> __nfc_llcp_recv(), which only guarantees LLCP_HEADER_SIZE (2) bytes
> via pskb_may_pull().
> 
> A malformed PDU that is exactly two bytes long -- delivered directly by
> the NFC controller, or as an aggregated PDU inside an AGF frame whose
> inner length field is 2 -- causes both handlers to read one byte past
> the guaranteed data.  In the direct case the byte is whatever follows
> the valid payload in the skb buffer.  In the AGF case the inner skb is
> allocated by nfc_alloc_recv_skb() with exactly two bytes of payload, so
> the read is past the meaningful data and into whatever the slab
> allocator left there.
> 
> For DM, the stale reason byte selects between NOBOUND/REJ and the
> default socket-lookup path, potentially closing the wrong socket.  For
> HDLC, the stale sequence byte corrupts N(S)/N(R) tracking: a bogus
> N(R) drains the tx_pending_queue unconditionally, dropping in-flight I
> frames and breaking the connection.
> 
> Add a minimum-length check to each handler: DM requires
> LLCP_HEADER_SIZE + 1 (the reason byte) and HDLC requires
> LLCP_HEADER_SIZE + LLCP_SEQUENCE_SIZE.
> 
> Fixes: d646960f7986 ("NFC: Initial LLCP support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
> ---
>  net/nfc/llcp_core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

For all of these different patches, did you forget to add the required
"Assisted-by:" tag?

Please resubmit them all with that included.

thanks,

greg k-h

Re: [PATCH] nfc: llcp: validate PDU size in nfc_llcp_recv_dm() and nfc_llcp_recv_hdlc()

[Aamir Ahmed]

On Mon, Sep 07, 2026 at 04:33:06PM +0200, Greg KH wrote:
> For all of these different patches, did you forget to add the required
> "Assisted-by:" tag?
>
> Please resubmit them all with that included.

Apologies! yes this was an oversight on my part, will ensure the tag
and other conventions I missed are adhered to in future. Will send in
small batches to avoid flooding.

Kind Regards

Aamir A.

Re: [PATCH] nfc: llcp: validate PDU size in nfc_llcp_recv_dm() and nfc_llcp_recv_hdlc()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] In nfc_llcp_recv_hdlc() (net/nfc/llcp_core.c:1081-1085) the
  newly added short-PDU guard replies with nfc_llcp_send_dm(local, dsap,…
- [Low] Both new guards test the total length skb->len rather than the
  linear/pullable length (pskb_may_pull(skb, 3) or skb_headlen(skb) >=…

--

nfc: llcp: validate PDU size in nfc_llcp_recv_dm() and nfc_llcp_recv_hdlc()

The patch adds minimum length checks to two LLCP receive handlers so the
third PDU byte is not read when only the two header bytes are guaranteed by
__nfc_llcp_recv().  DM requires LLCP_HEADER_SIZE + 1 and HDLC requires
LLCP_HEADER_SIZE + LLCP_SEQUENCE_SIZE.

A couple of questions below about the new checks.

> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index cac1b5487064d..89d6f4599e11a 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -1077,6 +1077,12 @@ static void nfc_llcp_recv_hdlc(struct nfc_llcp_local *local,
>  	ptype = nfc_llcp_ptype(skb);
>  	dsap = nfc_llcp_dsap(skb);
>  	ssap = nfc_llcp_ssap(skb);
> +
> +	if (skb->len < LLCP_HEADER_SIZE + LLCP_SEQUENCE_SIZE) {
> +		nfc_llcp_send_dm(local, dsap, ssap, LLCP_DM_NOCONN);
> +		return;
> +	}
> +

[Severity: Medium]
Should the short-PDU case really answer with DM(NOCONN)?

This branch runs before the nfc_llcp_sock_get() lookup further down, so it
cannot tell "no such data link" from "an established data link that sent a
truncated I/RR/RNR frame":

net/nfc/llcp_core.c:nfc_llcp_recv_hdlc() {
	...
	llcp_sock = nfc_llcp_sock_get(local, dsap, ssap);
	if (llcp_sock == NULL) {
		nfc_llcp_send_dm(local, dsap, ssap, LLCP_DM_NOCONN);
		return;
	}
	...
}

Before this patch the only DM(NOCONN) emitted from this function was the one
above, i.e. it was sent only when the lookup actually failed, matching what
the reason code documents in net/nfc/llcp.h:

	/* Disconnection reason code */
	#define LLCP_DM_DISC    0x00
	#define LLCP_DM_NOCONN  0x01

Can this desynchronise the two endpoints?  nfc_llcp_send_dm() passes the
received dsap/ssap into llcp_add_header(), which swaps them, so the DM is
addressed back to the peer of the still-open connection.  The in-tree
decoder of that byte, nfc_llcp_recv_dm() in this same file, does the
following for any reason that is not NOBOUND/REJ:

	sk->sk_err = ENXIO;
	sk->sk_state = LLCP_CLOSED;
	sk->sk_state_change(sk);

so the peer socket goes to LLCP_CLOSED while the local socket stays in
LLCP_CONNECTED with tx_queue/tx_pending_queue intact until the link timer
reaps it.

The sibling hunk of this patch takes the opposite policy and just returns for
the same malformed-PDU condition.  Would silently dropping the frame here
too, as the other malformed-PDU paths in this file do, keep both ends
consistent and let the peer retransmit?

Also, the commit message only mentions adding "a minimum-length check to each
handler" with no mention of emitting a protocol response, which a stable
backporter would likely read as a pure bounds check.

[Severity: Low]
This isn't a bug I can demonstrate today, but the new checks test skb->len
while the bytes they protect are read out of the linear region:

net/nfc/llcp_core.c
static u8 nfc_llcp_ns(const struct sk_buff *pdu)
{
	return pdu->data[2] >> 4;
}

The only linearity guarantee on this path is in __nfc_llcp_recv():

	if (!pskb_may_pull(skb, LLCP_HEADER_SIZE)) {
		kfree_skb(skb);
		return;
	}

and nothing between there and the handlers (nfc_llcp_rx_work() ->
nfc_llcp_rx_skb() -> nfc_llcp_recv_hdlc()) pulls further.  For an skb with
skb->len == 3 and data_len == 1, both new conditions are false and
skb->data[2] is still read past the linear data; for an I PDU the following

		skb_pull(skb, LLCP_HEADER_SIZE + LLCP_SEQUENCE_SIZE);

would drive skb->len below skb->data_len and hit the BUG() in __skb_pull().

I could not find a producer of such an skb: grepping net/nfc/ and drivers/nfc/
for nr_frags, frag_list, skb_add_rx_frag and skb_fill_page_desc returns
nothing, every skb reaching __nfc_llcp_recv() comes from alloc_skb()-based
helpers with data_len == 0, and nci_add_rx_data_frag() coalesces fragments
with skb_cow_head() plus memcpy(skb_push(...)) so the skb stays linear.

Would pskb_may_pull(skb, LLCP_HEADER_SIZE + LLCP_SEQUENCE_SIZE) (or a
skb_headlen() test) be preferable here, so the guard both validates and
linearizes?

>  	ns = nfc_llcp_ns(skb);
>  	nr = nfc_llcp_nr(skb);
>  
> @@ -1254,6 +1260,10 @@ static void nfc_llcp_recv_dm(struct nfc_llcp_local *local,
>  
>  	dsap = nfc_llcp_dsap(skb);
>  	ssap = nfc_llcp_ssap(skb);
> +
> +	if (skb->len < LLCP_HEADER_SIZE + 1)
> +		return;
> +

[Severity: Low]
Same skb->len versus linear-length question as above applies to the
reason = skb->data[2] read below.  Note that nfc_llcp_recv_dm() takes a
const struct sk_buff *, so switching to pskb_may_pull() here would need
either a signature change or raising the guarantee in __nfc_llcp_recv() and
the AGF dispatch path.

>  	reason = skb->data[2];
>  
>  	pr_debug("%d %d reason %d\n", ssap, dsap, reason);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB0001E8602F36054CDD8979A2C8B32%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM