All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net v2] kcm: Fix memory leak in error path of kcm_sendmsg()
@ 2023-09-09 17:03 Shigeru Yoshida
  2023-09-11  9:25 ` patchwork-bot+netdevbpf
  2023-09-11 21:03 ` Kuniyuki Iwashima
  0 siblings, 2 replies; 3+ messages in thread
From: Shigeru Yoshida @ 2023-09-09 17:03 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni
  Cc: netdev, linux-kernel, Shigeru Yoshida, syzbot+6f98de741f7dbbfc4ccb

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6f98de741f7dbbfc4ccb
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
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;
 
 	err = sk_stream_error(sk, msg->msg_flags, err);
 
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH net v2] kcm: Fix memory leak in error path of kcm_sendmsg()
  2023-09-09 17:03 [PATCH net v2] kcm: Fix memory leak in error path of kcm_sendmsg() Shigeru Yoshida
@ 2023-09-11  9:25 ` patchwork-bot+netdevbpf
  2023-09-11 21:03 ` Kuniyuki Iwashima
  1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-09-11  9:25 UTC (permalink / raw)
  To: Shigeru Yoshida
  Cc: davem, edumazet, kuba, pabeni, netdev, linux-kernel,
	syzbot+6f98de741f7dbbfc4ccb

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Sun, 10 Sep 2023 02:03:10 +0900 you wrote:
> 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
> 
> [...]

Here is the summary with links:
  - [net,v2] kcm: Fix memory leak in error path of kcm_sendmsg()
    https://git.kernel.org/netdev/net/c/c821a88bd720

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH net v2] kcm: Fix memory leak in error path of kcm_sendmsg()
  2023-09-09 17:03 [PATCH net v2] kcm: Fix memory leak in error path of kcm_sendmsg() Shigeru Yoshida
  2023-09-11  9:25 ` patchwork-bot+netdevbpf
@ 2023-09-11 21:03 ` Kuniyuki Iwashima
  1 sibling, 0 replies; 3+ messages in thread
From: Kuniyuki Iwashima @ 2023-09-11 21:03 UTC (permalink / raw)
  To: syoshida
  Cc: davem, edumazet, kuba, linux-kernel, netdev, pabeni,
	syzbot+6f98de741f7dbbfc4ccb

From: Shigeru Yoshida <syoshida@redhat.com>
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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6f98de741f7dbbfc4ccb
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> ---
> 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<---

^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-11 23:21 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-09 17:03 [PATCH net v2] kcm: Fix memory leak in error path of kcm_sendmsg() Shigeru Yoshida
2023-09-11  9:25 ` patchwork-bot+netdevbpf
2023-09-11 21:03 ` Kuniyuki Iwashima

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.