On Mon, 21 Jun 2021 19:18:56 +0200
Guvenc Gulce [off-list ref] wrote:

On 21/06/2021 16:56, Pavel Skripkin wrote:

quoted

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_ops

I 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 work

diff --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:

This sounds better. I knew, that my approach will just "silence" the
bug, that's why I CCed you for better approach :)

With regards,
Pavel Skripkin