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

Tung Quang Nguyen
2026-04-21T12:10:06Z

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