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

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

[Lee Jones]

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;
 		*buf = head;
 		TIPC_SKB_CB(head)->tail = NULL;
 		*headbuf = NULL;
-- 
2.54.0.rc1.513.gad8abe7a5a-goog

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

[Tung Quang Nguyen]

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.2433372-1-nicholas@carlini.com/
The author did not respond to my comment.
Can you improve the fix by applying my patch?

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 76284fc538eb..01a693559589 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -177,8 +177,19 @@ 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(&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;
+               }
                *buf = head;
                TIPC_SKB_CB(head)->tail = NULL;
                *headbuf = NULL;
> 		*buf = head;
> 		TIPC_SKB_CB(head)->tail = NULL;
> 		*headbuf = NULL;
>--
>2.54.0.rc1.513.gad8abe7a5a-goog
>

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

[Lee Jones]

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.2433372-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.

> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 76284fc538eb..01a693559589 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -177,8 +177,19 @@ 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(&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;
> +               }
>                 *buf = head;
>                 TIPC_SKB_CB(head)->tail = NULL;
>                 *headbuf = NULL;
> > 		*buf = head;
> > 		TIPC_SKB_CB(head)->tail = NULL;
> > 		*headbuf = NULL;
> >--
> >2.54.0.rc1.513.gad8abe7a5a-goog
> >
> 

-- 
Lee Jones [李琼斯]

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

[Tung Quang Nguyen]

>> 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().
>> diff --git a/net/tipc/msg.c b/net/tipc/msg.c index
>> 76284fc538eb..01a693559589 100644
>> --- a/net/tipc/msg.c
>> +++ b/net/tipc/msg.c
>> @@ -177,8 +177,19 @@ 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(&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;
>> +               }
>>                 *buf = head;
>>                 TIPC_SKB_CB(head)->tail = NULL;
>>                 *headbuf = NULL;
>> > 		*buf = head;
>> > 		TIPC_SKB_CB(head)->tail = NULL;
>> > 		*headbuf = NULL;
>> >--
>> >2.54.0.rc1.513.gad8abe7a5a-goog
>> >
>>
>
>--
>Lee Jones [李琼斯]

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

[Lee Jones]

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.

-- 
Lee Jones [李琼斯]

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

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

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

[Lee Jones]

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