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

Lee Jones
2026-04-21T10:35:49Z

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/20260330205313.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))) {
> + if (head != *headbuf)
> + *headbuf = head;
> goto err;
> + }
> +

"It looks like this logic might re-introduce the double-free bug.
The call to `tipc_msg_validate(headbuf)` is correct, as it passes the
pointer-to-pointer and allows the callee to update `*headbuf` if the skb
is reallocated.

However, the subsequent check seems to undo this. If a reallocation
happens, `head` will hold the pointer to the old, freed skb, while
`*headbuf` will hold the new one. The condition `head != *headbuf` will
be true, and the assignment `*headbuf = head` will restore the stale
pointer, leading to a double-free on the `err` path.

The preceding comment also appears to have the pointer roles reversed.

Would it be simpler and more correct to remove the `if (head != *headbuf)`
check and the large comment block? The change from `&head` to `headbuf`
in the function call seems to be the only change required to fix the bug.
Also, please update the commit message to reflect the corrected logic."

I suggest that we go with the original patch. Although I find it admirable
that you are thinking about and attempting to protect the more common
happy-path, I think the resultant single additional variable assignment
is negligible and that the simplicity of the previous fix has greater
benefits in terms of code readability and maintainability.

If you like, I can add a small comment, but I doubt even that is necessary.

--
Lee Jones [李琼斯]