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

[Tung Quang Nguyen]

>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@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/202603302053
>> > >> 13.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))) {
You did NOT apply my patch correctly. I did not suggest passing headbuf to tipc_msg_validate().
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;
+               }

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