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

[Tung Quang Nguyen]

>Subject: [PATCH v2 1/1] tipc: fix double-free in tipc_buf_append()
>
>tipc_msg_validate() 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 checking if head now points to a newly allocated reassembled skb.
>If it does, reassign *headbuf for later freeing operations.
>
>Fixes: d618d09a68e4 ("tipc: enforce valid ratio between skb truesize and
>contents")
>Suggested-by: Tung Nguyen <tung.quang.nguyen@xxxxxxxx>
>Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
>---
>1v => v2: Keep the passed pointer type the same, but reassign on-change
>
> net/tipc/msg.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
>diff --git a/net/tipc/msg.c b/net/tipc/msg.c index 76284fc538eb..b0bba0feef56
>100644
>--- a/net/tipc/msg.c
>+++ b/net/tipc/msg.c
>@@ -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.
>+		 */
>+		if (unlikely(!tipc_msg_validate(&head))) {
>+			if (head != *headbuf)
>+				*headbuf = head;
> 			goto err;
>+		}
>+
> 		*buf = head;
> 		TIPC_SKB_CB(head)->tail = NULL;
> 		*headbuf = NULL;
>--
>2.54.0.rc1.555.g9c883467ad-goog
Reviewed-by: Tung Nguyen <tung.quang.nguyen@xxxxxxxx>