linux-kernel - Re: [syzbot] general protection fault in smc_tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Jun 2021 19:18:56 +0200
From:   Guvenc Gulce <guvenc@...ux.ibm.com>
To:     Pavel Skripkin <paskripkin@...il.com>,
        syzbot <syzbot+5dda108b672b54141857@...kaller.appspotmail.com>
Cc:     coreteam@...filter.org, davem@...emloft.net, dsahern@...nel.org,
        fw@...len.de, kadlec@...filter.org, kgraul@...ux.ibm.com,
        kuba@...nel.org, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, pablo@...filter.org,
        syzkaller-bugs@...glegroups.com, yoshfuji@...ux-ipv6.org
Subject: Re: [syzbot] general protection fault in smc_tx_sendmsg



On 21/06/2021 16:56, Pavel Skripkin wrote:
> On Sun, 20 Jun 2021 16:22:16 -0700
> syzbot <syzbot+5dda108b672b54141857@...kaller.appspotmail.com> wrote:
>
>> 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 <fw@...len.de>
>> 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:

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);