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

[Lee Jones]

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@xxxxxxxxxx>
> > >> >---
> > >> > 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@xxxxxxxxxxx/ 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 [李琼斯]