Re: [syzbot] general protection fault in smc_tx_sendmsg
From: Guvenc Gulce <hidden>
Date: 2021-06-21 17:19:24
Also in:
linux-s390, lkml, netfilter-devel
Subsystem:
networking [general], shared memory communications (smc) sockets, the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, D. Wythe, Dust Li, Sidraya Jayagond, Wenjia Zhang, Linus Torvalds
On 21/06/2021 16:56, Pavel Skripkin wrote:
quoted hunk ↗ jump to hunk
On Sun, 20 Jun 2021 16:22:16 -0700 syzbot [off-list ref] wrote:quoted
Hello, syzbot found the following issue on: HEAD commit: 0c337952 Merge tag 'wireless-drivers-next-2021-06-16' of g.. git tree: net-next console output: https://syzkaller.appspot.com/x/log.txt?x=1621de10300000 kernel config: https://syzkaller.appspot.com/x/.config?x=a6380da8984033f1 dashboard link: https://syzkaller.appspot.com/bug?extid=5dda108b672b54141857 syz repro: https://syzkaller.appspot.com/x/repro.syz?x=121d2d20300000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=100bd768300000 The issue was bisected to: commit f9006acc8dfe59e25aa75729728ac57a8d84fc32 Author: Florian Westphal [off-list ref] Date: Wed Apr 21 07:51:08 2021 +0000 netfilter: arp_tables: pass table pointer via nf_hook_opsI think, bisection is wrong this time :) It should be e0e4b8fa533858532f1b9ea9c6a4660d09beb37a ("net/smc: Add SMC statistics support") Some debug results: syzkaller repro just opens the socket and calls sendmsg. Ftrace log: 0) | smc_create() { 0) | smc_sock_alloc() { 0) + 88.493 us | smc_hash_sk(); 0) ! 131.487 us | } 0) ! 189.912 us | } 0) | smc_sendmsg() { 0) 2.808 us | smc_tx_sendmsg(); 0) ! 148.484 us | } That means, that smc_buf_create() wasn't called at all, so we need to check sndbuf_desc before dereferencing Something like this should workdiff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c index 075c4f4b4..e24071b12 100644 --- a/net/smc/smc_tx.c +++ b/net/smc/smc_tx.c@@ -154,7 +154,7 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len) goto out_err; } - if (len > conn->sndbuf_desc->len) + if (conn->sndbuf_desc && len > conn->sndbuf_desc->len) SMC_STAT_RMB_TX_SIZE_SMALL(smc, !conn->lnk); if (len > conn->peer_rmbe_size)Thoughts? +CC Guvenc Gulce With regards, Pavel Skripkin
Thanks for analyzing the cause. Your approach would work but I would prefer that we check the state of the socket before doing the statistics relevant if check. This will ensure that smc_buf_create() was already called. I am testing the fix at the moment which would look like the following:
diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index 075c4f4b41cf..289025cd545a 100644
--- a/net/smc/smc_tx.c
+++ b/net/smc/smc_tx.c@@ -154,6 +154,9 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len) goto out_err; } + if (sk->sk_state == SMC_INIT) + return -ENOTCONN; + if (len > conn->sndbuf_desc->len) SMC_STAT_RMB_TX_SIZE_SMALL(smc, !conn->lnk);