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