Re: [PATCH net v2] kcm: Fix memory leak in error path of kcm_sendmsg()

[Kuniyuki Iwashima]

From: Shigeru Yoshida <syoshida@xxxxxxxxxx>
Date: Sun, 10 Sep 2023 02:03:10 +0900
> syzbot reported a memory leak like below:
> 
> BUG: memory leak
> unreferenced object 0xffff88810b088c00 (size 240):
>   comm "syz-executor186", pid 5012, jiffies 4294943306 (age 13.680s)
>   hex dump (first 32 bytes):
>     00 89 08 0b 81 88 ff ff 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff83e5d5ff>] __alloc_skb+0x1ef/0x230 net/core/skbuff.c:634
>     [<ffffffff84606e59>] alloc_skb include/linux/skbuff.h:1289 [inline]
>     [<ffffffff84606e59>] kcm_sendmsg+0x269/0x1050 net/kcm/kcmsock.c:815
>     [<ffffffff83e479c6>] sock_sendmsg_nosec net/socket.c:725 [inline]
>     [<ffffffff83e479c6>] sock_sendmsg+0x56/0xb0 net/socket.c:748
>     [<ffffffff83e47f55>] ____sys_sendmsg+0x365/0x470 net/socket.c:2494
>     [<ffffffff83e4c389>] ___sys_sendmsg+0xc9/0x130 net/socket.c:2548
>     [<ffffffff83e4c536>] __sys_sendmsg+0xa6/0x120 net/socket.c:2577
>     [<ffffffff84ad7bb8>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>     [<ffffffff84ad7bb8>] do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
>     [<ffffffff84c0008b>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> In kcm_sendmsg(), kcm_tx_msg(head)->last_skb is used as a cursor to append
> newly allocated skbs to 'head'. If some bytes are copied, an error occurred,
> and jumped to out_error label, 'last_skb' is left unmodified. A later
> kcm_sendmsg() will use an obsoleted 'last_skb' reference, corrupting the
> 'head' frag_list and causing the leak.
> 
> This patch fixes this issue by properly updating the last allocated skb in
> 'last_skb'.
> 
> Fixes: ab7ac4eb9832 ("kcm: Kernel Connection Multiplexor module")
> Reported-and-tested-by: syzbot+6f98de741f7dbbfc4ccb@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=6f98de741f7dbbfc4ccb
> Signed-off-by: Shigeru Yoshida <syoshida@xxxxxxxxxx>
> ---
> v1->v2:
> - Update the commit message to include more detailed root cause. 
> ---
>  net/kcm/kcmsock.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> index 393f01b2a7e6..34d4062f639a 100644
> --- a/net/kcm/kcmsock.c
> +++ b/net/kcm/kcmsock.c
> @@ -939,6 +939,8 @@ static int kcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>  
>  	if (head != kcm->seq_skb)
>  		kfree_skb(head);
> +	else if (copied)
> +		kcm_tx_msg(head)->last_skb = skb;
>

Sorry for being late, but this seems wrong to me.

I think we should purge the queue as we do so for UDP by
udp_flush_pending_frames(); otherwise, even when we get an
error, there could be some data appended to the tail of the
buffer and we cannot know how many bytes it is.

I'll send the following patch:

---8<---
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index 740539a218b7..fb27ca675acb 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -937,10 +937,8 @@ static int kcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 		goto partial_message;
 	}
 
-	if (head != kcm->seq_skb)
-		kfree_skb(head);
-	else if (copied)
-		kcm_tx_msg(head)->last_skb = skb;
+	kfree_skb(head);
+	kcm->seq_skb = NULL;
 
 	err = sk_stream_error(sk, msg->msg_flags, err);
 
---8<---