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

Lee Jones
2026-04-21T12:28:10Z

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 [李琼斯]