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

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.

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

[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