Re: [PATCH 1/1] tipc: fix double-free in tipc_buf_append()

[Lee Jones <lee@kernel.org>]

On Tue, 21 Apr 2026, Tung Quang Nguyen wrote:

> >Subject: Re: [PATCH 1/1] tipc: fix double-free in tipc_buf_append()
> >
> >On Mon, 20 Apr 2026, Lee Jones wrote:
> >
> >> On Mon, 20 Apr 2026, Tung Quang Nguyen wrote:
> >>
> >> > >> Subject: [PATCH 1/1] tipc: fix double-free in tipc_buf_append()
> >> > >> >
> >> > >> >The tipc_msg_validate() function can potentially reallocate the
> >> > >> >skb it is validating, freeing the old one.  In
> >> > >> >tipc_buf_append(), it was being called with a pointer to a local
> >> > >> >variable which was a copy of the
> >> > >caller's skb pointer.
> >> > >> >
> >> > >> >If the skb was reallocated and validation subsequently failed,
> >> > >> >the error handling path would free the original skb pointer,
> >> > >> >which had already been freed, leading to double-free.
> >> > >> >
> >> > >> >Fix this by passing the caller's skb pointer-pointer directly to
> >> > >> >tipc_msg_validate(), ensuring any modification is reflected correctly.
> >> > >> >The local skb pointer is then updated from the (possibly
> >> > >> >modified) caller's pointer.
> >> > >> >
> >> > >> >Fixes: d618d09a68e4 ("tipc: enforce valid ratio between skb
> >> > >> >truesize and
> >> > >> >contents")
> >> > >> >Assisted-by: Gemini:gemini-3.1-pro-preview
> >> > >> >Signed-off-by: Lee Jones <lee@kernel.org>
> >> > >> >---
> >> > >> > net/tipc/msg.c | 3 ++-
> >> > >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >> > >> >
> >> > >> >diff --git a/net/tipc/msg.c b/net/tipc/msg.c index
> >> > >> >76284fc538eb..9f4f612ee027
> >> > >> >100644
> >> > >> >--- a/net/tipc/msg.c
> >> > >> >+++ b/net/tipc/msg.c
> >> > >> >@@ -177,8 +177,9 @@ int tipc_buf_append(struct sk_buff
> >> > >> >**headbuf, struct sk_buff **buf)
> >> > >> >
> >> > >> > 	if (fragid == LAST_FRAGMENT) {
> >> > >> > 		TIPC_SKB_CB(head)->validated = 0;
> >> > >> >-		if (unlikely(!tipc_msg_validate(&head)))
> >> > >> >+		if (unlikely(!tipc_msg_validate(headbuf)))
> >> > >> > 			goto err;
> >> > >> >+		head = *headbuf;
> >> > >> This is a known issue and was reported via
> >> > >> https://patchwork.kernel.org/project/netdevbpf/patch/202603302053
> >> > >> 13.24 33372-1-nicholas@carlini.com/ The author did not respond to
> >> > >> my comment.
> >> > >> Can you improve the fix by applying my patch?
> >> > >
> >> > >I'd be happy to make any required changes.
> >> > >
> >> > >However, is this approach superior to simply passing a reference?
> >> > >
> >> > >v1 appears to be simpler, easier to read and avoids the explanation.
> >> > >
> >> > As I explained, your fix adds extra overhead to normal path while the error
> >path is corner case and it rarely happens.
> >> > Whatever approach is applied, we need to add explanation to understand
> >more easily the logic and hidden trick in tipc_msg_validate().
> >>
> >> Very well.  I have made the recommended changes.
> >>
> >> The patch is currently in my build-test environment.
> >>
> >> I will post v2, when everything has been satisfied.
> >
> >Okay, I genuinely tried to apply your patch.  It builds just fine, but Gemini (the
> >AI I use to pre-review patches before submission) has some doubts that this is
> >the correct approach:
> >
> >> @@ -177,8 +177,20 @@ int tipc_buf_append(struct sk_buff **headbuf,
> >> struct sk_buff **buf)
> >>
> >>       if (fragid == LAST_FRAGMENT) {
> >>               TIPC_SKB_CB(head)->validated = 0;
> >> -             if (unlikely(!tipc_msg_validate(&head)))
> >> +
> >> +             /* If the reassembled skb has been freed in
> >> +              * tipc_msg_validate() because of an invalid truesize,
> >> +              * then head will point to a newly allocated reassembled
> >> +              * skb, while *headbuf points to freed reassembled skb.
> >> +              * In such cases, correct *headbuf for freeing the newly
> >> +              * allocated reassembled skb later.
> >> +              *
> >> +              * Note: It's done this way instead of passing &head          // I added
> >this part to give
> >> +              * to avoid slowing down the happy path since this failure    // the
> >reviewer some additoinal
> >> +              * is a rare event.                                           // context
> >> +              */
> >> +             if (unlikely(!tipc_msg_validate(headbuf))) {
> You did NOT apply my patch correctly. I did not suggest passing headbuf to tipc_msg_validate().

Ah, you're right.  I missed that line change.

Let me revisit.  Bear with.

> My patch is very simple:
> +               if (unlikely(!tipc_msg_validate(&head))) {
> +                       /* reassembled skb has been freed in
> +                        * tipc_msg_validate() because of invalid truesize.
> +                        * head now points to newly-allocated reassembled skb
> +                        * while *headbuf points to freed reassembled skb.
> +                        * So, correct *headbuf for freeing newly-allocated
> +                        * reassembled skb later.
> +                        */
> +                       if (head != *headbuf)
> +                               *headbuf = head;
> +
>                         goto err;
> +               }

-- 
Lee Jones [李琼斯]