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