lkml.org 
[lkml]   [2021]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [syzbot] general protection fault in smc_tx_sendmsg
On Mon, 21 Jun 2021 19:18:56 +0200
Guvenc Gulce <guvenc@linux.ibm.com> wrote:

>
>
> On 21/06/2021 16:56, Pavel Skripkin wrote:
> > On Sun, 20 Jun 2021 16:22:16 -0700
> > syzbot <syzbot+5dda108b672b54141857@syzkaller.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@strlen.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:
>

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

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




With regards,
Pavel Skripkin

\
 
 \ /
  Last update: 2021-06-21 21:06    [W:0.050 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site