| Message ID | 20230909170310.1978851-1-syoshida@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c821a88bd720b0046433173185fd841a100d44ad |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] kcm: Fix memory leak in error path of kcm_sendmsg() | expand |
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!
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<---
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);
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(+)