[PATCH] nfc: llcp: check skb tailroom before appending TLV

[PATCH] nfc: llcp: check skb tailroom before appending TLV

[Sam Swicegood]

llcp_add_tlv() appends TLV data to an skb using skb_put_data()
without first verifying that sufficient tailroom is available.

Add a tailroom check to avoid writing past end of the skb when
building LLCP PDUs.

Signed-off-by: Sam Swicegood <sam@gib.games>
---
 net/nfc/llcp_commands.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c
index b652323bc2c1..10acc5da954f 100644
--- a/net/nfc/llcp_commands.c
+++ b/net/nfc/llcp_commands.c
@@ -300,9 +300,10 @@ static struct sk_buff *llcp_add_header(struct sk_buff *pdu,
 static struct sk_buff *llcp_add_tlv(struct sk_buff *pdu, const u8 *tlv,
 				    u8 tlv_length)
 {
-	/* XXX Add an skb length check */
+	if (!pdu || !tlv)
+		return NULL;
 
-	if (tlv == NULL)
+	if (skb_tailroom(pdu) < tlv_length)
 		return NULL;
 
 	skb_put_data(pdu, tlv, tlv_length);
-- 
2.43.0

Re: [PATCH] nfc: llcp: check skb tailroom before appending TLV

[Kuniyuki Iwashima]

On Wed, Feb 18, 2026 at 1:46 PM Sam Swicegood <samswicegood@gmail.com> wrote:
>
> llcp_add_tlv() appends TLV data to an skb using skb_put_data()
> without first verifying that sufficient tailroom is available.

This is because all callers (nfc_llcp_send_connect() and
nfc_llcp_send_cc()) calculate the necessary tailroom and
allocate skb with llcp_allocate_pdu(), no ?


>
> Add a tailroom check to avoid writing past end of the skb when
> building LLCP PDUs.
>
> Signed-off-by: Sam Swicegood <sam@gib.games>
> ---
>  net/nfc/llcp_commands.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c
> index b652323bc2c1..10acc5da954f 100644
> --- a/net/nfc/llcp_commands.c
> +++ b/net/nfc/llcp_commands.c
> @@ -300,9 +300,10 @@ static struct sk_buff *llcp_add_header(struct sk_buff *pdu,
>  static struct sk_buff *llcp_add_tlv(struct sk_buff *pdu, const u8 *tlv,
>                                     u8 tlv_length)
>  {
> -       /* XXX Add an skb length check */
> +       if (!pdu || !tlv)
> +               return NULL;
>
> -       if (tlv == NULL)
> +       if (skb_tailroom(pdu) < tlv_length)
>                 return NULL;
>
>         skb_put_data(pdu, tlv, tlv_length);
> --
> 2.43.0
>

RE: [PATCH] nfc: llcp: check skb tailroom before appending TLV

[Sam Swicegood]

Yes, both current callers compute size and llcp_allocate_pdu() allocates accordingly.

My intent here was to make llcp_add_tlv() self-contained and defensive:
it currently assumes the skb has enough tailroom and will still append
even if that assumption is violated. The check converts a potential overflow
into a clean failure and removes the in-code todo that explicitly calls out
the missing validation.

If you’d prefer not to change behavior (returning NULL), I can imstead rework as a 
WARN_ON_ONCE(skb_tailroom(pdu) < tlv_length) and keep the existing flow,
or adjust the helper to return an error code and have callers handle it explicitly.

-----Original Message-----
From: Kuniyuki Iwashima <kuniyu@google.com> 
Sent: Wednesday, February 18, 2026 4:07 PM
To: Sam Swicegood <samswicegood@gmail.com>
Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; horms@kernel.org; Sam Swicegood <sam@gib.games>
Subject: Re: [PATCH] nfc: llcp: check skb tailroom before appending TLV

On Wed, Feb 18, 2026 at 1:46 PM Sam Swicegood <samswicegood@gmail.com> wrote:
>
> llcp_add_tlv() appends TLV data to an skb using skb_put_data() without 
> first verifying that sufficient tailroom is available.

This is because all callers (nfc_llcp_send_connect() and
nfc_llcp_send_cc()) calculate the necessary tailroom and allocate skb with llcp_allocate_pdu(), no ?


>
> Add a tailroom check to avoid writing past end of the skb when 
> building LLCP PDUs.
>
> Signed-off-by: Sam Swicegood <sam@gib.games>
> ---
>  net/nfc/llcp_commands.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c index 
> b652323bc2c1..10acc5da954f 100644
> --- a/net/nfc/llcp_commands.c
> +++ b/net/nfc/llcp_commands.c
> @@ -300,9 +300,10 @@ static struct sk_buff *llcp_add_header(struct 
> sk_buff *pdu,  static struct sk_buff *llcp_add_tlv(struct sk_buff *pdu, const u8 *tlv,
>                                     u8 tlv_length)  {
> -       /* XXX Add an skb length check */
> +       if (!pdu || !tlv)
> +               return NULL;
>
> -       if (tlv == NULL)
> +       if (skb_tailroom(pdu) < tlv_length)
>                 return NULL;
>
>         skb_put_data(pdu, tlv, tlv_length);
> --
> 2.43.0
>

Re: [PATCH] nfc: llcp: check skb tailroom before appending TLV

[Kuniyuki Iwashima]

On Wed, Feb 18, 2026 at 2:34 PM Sam Swicegood <sam@gib.games> wrote:
>
> Yes, both current callers compute size and llcp_allocate_pdu() allocates accordingly.
>
> My intent here was to make llcp_add_tlv() self-contained and defensive:
> it currently assumes the skb has enough tailroom and will still append
> even if that assumption is violated. The check converts a potential overflow
> into a clean failure and removes the in-code todo that explicitly calls out
> the missing validation.
>
> If you’d prefer not to change behavior (returning NULL), I can imstead rework as a
> WARN_ON_ONCE(skb_tailroom(pdu) < tlv_length) and keep the existing flow,
> or adjust the helper to return an error code and have callers handle it explicitly.

I think defensive programming or WARN_ON_ONCE() for
decade-safe code adds no value here.

I'd rather make it return void as it's simply not used since
de9e5aeb4f40e .

But it's net-next material, and it will open next week.
Please read this guideline for the next submission.
https://docs.kernel.org/process/maintainer-netdev.html

Thanks.


>
> -----Original Message-----
> From: Kuniyuki Iwashima <kuniyu@google.com>
> Sent: Wednesday, February 18, 2026 4:07 PM
> To: Sam Swicegood <samswicegood@gmail.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; horms@kernel.org; Sam Swicegood <sam@gib.games>
> Subject: Re: [PATCH] nfc: llcp: check skb tailroom before appending TLV
>
> On Wed, Feb 18, 2026 at 1:46 PM Sam Swicegood <samswicegood@gmail.com> wrote:
> >
> > llcp_add_tlv() appends TLV data to an skb using skb_put_data() without
> > first verifying that sufficient tailroom is available.
>
> This is because all callers (nfc_llcp_send_connect() and
> nfc_llcp_send_cc()) calculate the necessary tailroom and allocate skb with llcp_allocate_pdu(), no ?
>
>
> >
> > Add a tailroom check to avoid writing past end of the skb when
> > building LLCP PDUs.
> >
> > Signed-off-by: Sam Swicegood <sam@gib.games>
> > ---
> >  net/nfc/llcp_commands.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c index
> > b652323bc2c1..10acc5da954f 100644
> > --- a/net/nfc/llcp_commands.c
> > +++ b/net/nfc/llcp_commands.c
> > @@ -300,9 +300,10 @@ static struct sk_buff *llcp_add_header(struct
> > sk_buff *pdu,  static struct sk_buff *llcp_add_tlv(struct sk_buff *pdu, const u8 *tlv,
> >                                     u8 tlv_length)  {
> > -       /* XXX Add an skb length check */
> > +       if (!pdu || !tlv)
> > +               return NULL;
> >
> > -       if (tlv == NULL)
> > +       if (skb_tailroom(pdu) < tlv_length)
> >                 return NULL;
> >
> >         skb_put_data(pdu, tlv, tlv_length);
> > --
> > 2.43.0
> >