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

[Simon Horman <horms@kernel.org>]

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