bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* general protection fault in xsk_poll
@ 2019-08-20  1:18 syzbot
  2019-08-20 10:04 ` [PATCH bpf-next] xsk: proper socket state check " Björn Töpel
  0 siblings, 1 reply; 6+ messages in thread
From: syzbot @ 2019-08-20  1:18 UTC (permalink / raw)
  To: ast, bjorn.topel, bpf, daniel, davem, hawk, jakub.kicinski,
	john.fastabend, jonathan.lemon, kafai, linux-kernel,
	magnus.karlsson, netdev, songliubraving, syzkaller-bugs,
	xdp-newbies, yhs

Hello,

syzbot found the following crash on:

HEAD commit:    da657043 Add linux-next specific files for 20190819
git tree:       linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=16af124c600000
kernel config:  https://syzkaller.appspot.com/x/.config?x=739a9b3ab3d8c770
dashboard link: https://syzkaller.appspot.com/bug?extid=c82697e3043781e08802
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=109e1922600000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1445bf02600000

The bug was bisected to:

commit 77cd0d7b3f257fd0e3096b4fdcff1a7d38e99e10
Author: Magnus Karlsson <magnus.karlsson@intel.com>
Date:   Wed Aug 14 07:27:17 2019 +0000

     xsk: add support for need_wakeup flag in AF_XDP rings

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=15e1ea4c600000
final crash:    https://syzkaller.appspot.com/x/report.txt?x=17e1ea4c600000
console output: https://syzkaller.appspot.com/x/log.txt?x=13e1ea4c600000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com
Fixes: 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP  
rings")

kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 7959 Comm: syz-executor611 Not tainted 5.3.0-rc5-next-20190819  
#68
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
RIP: 0010:xsk_poll+0x95/0x540 net/xdp/xsk.c:386
Code: 80 3c 02 00 0f 85 70 04 00 00 4c 8b a3 88 04 00 00 48 b8 00 00 00 00  
00 fc ff df 49 8d bc 24 96 00 00 00 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48  
89 fa 83 e2 07 38 d0 7f 08 84 c0 0f 85 bf 03 00 00
RSP: 0018:ffff8880926f7850 EFLAGS: 00010207
RAX: dffffc0000000000 RBX: ffff88809a141700 RCX: ffffffff859b07aa
RDX: 0000000000000012 RSI: ffffffff859b07c4 RDI: 0000000000000096
RBP: ffff8880926f7880 R08: ffff88809698a580 R09: ffffed1013428329
R10: ffffed1013428328 R11: ffff88809a141947 R12: 0000000000000000
R13: 0000000000000304 R14: ffff888095d4d840 R15: ffff888092bdd020
FS:  0000555557529880(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000280 CR3: 0000000098281000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
  sock_poll+0x15e/0x480 net/socket.c:1256
  vfs_poll include/linux/poll.h:90 [inline]
  do_pollfd fs/select.c:859 [inline]
  do_poll fs/select.c:907 [inline]
  do_sys_poll+0x7c2/0xde0 fs/select.c:1001
  __do_sys_ppoll fs/select.c:1101 [inline]
  __se_sys_ppoll fs/select.c:1081 [inline]
  __x64_sys_ppoll+0x259/0x310 fs/select.c:1081
  do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x440159
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7  
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffd9fbd16e8 EFLAGS: 00000246 ORIG_RAX: 000000000000010f
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440159
RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000020000280
RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004019e0
R13: 0000000000401a70 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
---[ end trace da907175426b4065 ]---
RIP: 0010:xsk_poll+0x95/0x540 net/xdp/xsk.c:386
Code: 80 3c 02 00 0f 85 70 04 00 00 4c 8b a3 88 04 00 00 48 b8 00 00 00 00  
00 fc ff df 49 8d bc 24 96 00 00 00 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48  
89 fa 83 e2 07 38 d0 7f 08 84 c0 0f 85 bf 03 00 00
RSP: 0018:ffff8880926f7850 EFLAGS: 00010207
RAX: dffffc0000000000 RBX: ffff88809a141700 RCX: ffffffff859b07aa
RDX: 0000000000000012 RSI: ffffffff859b07c4 RDI: 0000000000000096
RBP: ffff8880926f7880 R08: ffff88809698a580 R09: ffffed1013428329
R10: ffffed1013428328 R11: ffff88809a141947 R12: 0000000000000000
R13: 0000000000000304 R14: ffff888095d4d840 R15: ffff888092bdd020
FS:  0000555557529880(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000280 CR3: 0000000098281000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH bpf-next] xsk: proper socket state check in xsk_poll
  2019-08-20  1:18 general protection fault in xsk_poll syzbot
@ 2019-08-20 10:04 ` Björn Töpel
  2019-08-20 14:30   ` Daniel Borkmann
  0 siblings, 1 reply; 6+ messages in thread
From: Björn Töpel @ 2019-08-20 10:04 UTC (permalink / raw)
  To: syzbot+c82697e3043781e08802, ast, daniel, netdev
  Cc: bjorn.topel, bpf, davem, hawk, jakub.kicinski, john.fastabend,
	jonathan.lemon, kafai, linux-kernel, magnus.karlsson,
	songliubraving, syzkaller-bugs, xdp-newbies, yhs, hdanton

From: Björn Töpel <bjorn.topel@intel.com>

The poll() implementation for AF_XDP sockets did not perform the
proper state checks, prior accessing the socket umem. This patch fixes
that by performing a xsk_is_bound() check.

Suggested-by: Hillf Danton <hdanton@sina.com>
Reported-by: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com
Fixes: 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP rings")
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
 net/xdp/xsk.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index ee4428a892fa..08bed5e92af4 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -356,13 +356,20 @@ static int xsk_generic_xmit(struct sock *sk, struct msghdr *m,
 	return err;
 }
 
+static bool xsk_is_bound(struct xdp_sock *xs)
+{
+	struct net_device *dev = READ_ONCE(xs->dev);
+
+	return dev && xs->state == XSK_BOUND;
+}
+
 static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
 {
 	bool need_wait = !(m->msg_flags & MSG_DONTWAIT);
 	struct sock *sk = sock->sk;
 	struct xdp_sock *xs = xdp_sk(sk);
 
-	if (unlikely(!xs->dev))
+	if (unlikely(!xsk_is_bound(xs)))
 		return -ENXIO;
 	if (unlikely(!(xs->dev->flags & IFF_UP)))
 		return -ENETDOWN;
@@ -383,6 +390,9 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock,
 	struct net_device *dev = xs->dev;
 	struct xdp_umem *umem = xs->umem;
 
+	if (unlikely(!xsk_is_bound(xs)))
+		return mask;
+
 	if (umem->need_wakeup)
 		dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
 						umem->need_wakeup);
@@ -417,7 +427,7 @@ static void xsk_unbind_dev(struct xdp_sock *xs)
 {
 	struct net_device *dev = xs->dev;
 
-	if (!dev || xs->state != XSK_BOUND)
+	if (!xsk_is_bound(xs))
 		return;
 
 	xs->state = XSK_UNBOUND;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] xsk: proper socket state check in xsk_poll
  2019-08-20 10:04 ` [PATCH bpf-next] xsk: proper socket state check " Björn Töpel
@ 2019-08-20 14:30   ` Daniel Borkmann
  2019-08-20 15:29     ` Björn Töpel
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2019-08-20 14:30 UTC (permalink / raw)
  To: Björn Töpel, syzbot+c82697e3043781e08802, ast, netdev
  Cc: bjorn.topel, bpf, davem, hawk, jakub.kicinski, john.fastabend,
	jonathan.lemon, kafai, linux-kernel, magnus.karlsson,
	songliubraving, syzkaller-bugs, xdp-newbies, yhs, hdanton

On 8/20/19 12:04 PM, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
> 
> The poll() implementation for AF_XDP sockets did not perform the
> proper state checks, prior accessing the socket umem. This patch fixes
> that by performing a xsk_is_bound() check.
> 
> Suggested-by: Hillf Danton <hdanton@sina.com>
> Reported-by: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com
> Fixes: 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP rings")
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
>   net/xdp/xsk.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index ee4428a892fa..08bed5e92af4 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -356,13 +356,20 @@ static int xsk_generic_xmit(struct sock *sk, struct msghdr *m,
>   	return err;
>   }
>   
> +static bool xsk_is_bound(struct xdp_sock *xs)
> +{
> +	struct net_device *dev = READ_ONCE(xs->dev);
> +
> +	return dev && xs->state == XSK_BOUND;
> +}
> +
>   static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
>   {
>   	bool need_wait = !(m->msg_flags & MSG_DONTWAIT);
>   	struct sock *sk = sock->sk;
>   	struct xdp_sock *xs = xdp_sk(sk);
>   
> -	if (unlikely(!xs->dev))
> +	if (unlikely(!xsk_is_bound(xs)))
>   		return -ENXIO;
>   	if (unlikely(!(xs->dev->flags & IFF_UP)))
>   		return -ENETDOWN;
> @@ -383,6 +390,9 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock,
>   	struct net_device *dev = xs->dev;
>   	struct xdp_umem *umem = xs->umem;
>   
> +	if (unlikely(!xsk_is_bound(xs)))
> +		return mask;
> +
>   	if (umem->need_wakeup)
>   		dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
>   						umem->need_wakeup);
> @@ -417,7 +427,7 @@ static void xsk_unbind_dev(struct xdp_sock *xs)
>   {
>   	struct net_device *dev = xs->dev;
>   
> -	if (!dev || xs->state != XSK_BOUND)
> +	if (!xsk_is_bound(xs))
>   		return;

I think I'm a bit confused by your READ_ONCE() usage. ;-/ I can see why you're
using it in xsk_is_bound() above, but then at the same time all the other callbacks
like xsk_poll() or xsk_unbind_dev() above have a struct net_device *dev = xs->dev
right before the test. Could you elaborate?

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] xsk: proper socket state check in xsk_poll
  2019-08-20 14:30   ` Daniel Borkmann
@ 2019-08-20 15:29     ` Björn Töpel
  2019-08-20 21:24       ` Daniel Borkmann
  0 siblings, 1 reply; 6+ messages in thread
From: Björn Töpel @ 2019-08-20 15:29 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: syzbot+c82697e3043781e08802, Alexei Starovoitov, Netdev,
	Björn Töpel, bpf, David Miller, Jesper Dangaard Brouer,
	Jakub Kicinski, John Fastabend, Jonathan Lemon, Martin KaFai Lau,
	LKML, Karlsson, Magnus, Song Liu, syzkaller-bugs, Xdp,
	Yonghong Song, hdanton

On Tue, 20 Aug 2019 at 16:30, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/20/19 12:04 PM, Björn Töpel wrote:
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > The poll() implementation for AF_XDP sockets did not perform the
> > proper state checks, prior accessing the socket umem. This patch fixes
> > that by performing a xsk_is_bound() check.
> >
> > Suggested-by: Hillf Danton <hdanton@sina.com>
> > Reported-by: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com
> > Fixes: 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP rings")
> > Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> > ---
> >   net/xdp/xsk.c | 14 ++++++++++++--
> >   1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index ee4428a892fa..08bed5e92af4 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -356,13 +356,20 @@ static int xsk_generic_xmit(struct sock *sk, struct msghdr *m,
> >       return err;
> >   }
> >
> > +static bool xsk_is_bound(struct xdp_sock *xs)
> > +{
> > +     struct net_device *dev = READ_ONCE(xs->dev);
> > +
> > +     return dev && xs->state == XSK_BOUND;
> > +}
> > +
> >   static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
> >   {
> >       bool need_wait = !(m->msg_flags & MSG_DONTWAIT);
> >       struct sock *sk = sock->sk;
> >       struct xdp_sock *xs = xdp_sk(sk);
> >
> > -     if (unlikely(!xs->dev))
> > +     if (unlikely(!xsk_is_bound(xs)))
> >               return -ENXIO;
> >       if (unlikely(!(xs->dev->flags & IFF_UP)))
> >               return -ENETDOWN;
> > @@ -383,6 +390,9 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock,
> >       struct net_device *dev = xs->dev;
> >       struct xdp_umem *umem = xs->umem;
> >
> > +     if (unlikely(!xsk_is_bound(xs)))
> > +             return mask;
> > +
> >       if (umem->need_wakeup)
> >               dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
> >                                               umem->need_wakeup);
> > @@ -417,7 +427,7 @@ static void xsk_unbind_dev(struct xdp_sock *xs)
> >   {
> >       struct net_device *dev = xs->dev;
> >
> > -     if (!dev || xs->state != XSK_BOUND)
> > +     if (!xsk_is_bound(xs))
> >               return;
>
> I think I'm a bit confused by your READ_ONCE() usage. ;-/ I can see why you're
> using it in xsk_is_bound() above, but then at the same time all the other callbacks
> like xsk_poll() or xsk_unbind_dev() above have a struct net_device *dev = xs->dev
> right before the test. Could you elaborate?
>

Yes, now I'm confused as well! Digging deeper... I believe there are a
couple of places in xsk.c that do not have
READ_ONCE/WRITE_ONCE-correctness. Various xdp_sock members are read
lock-less outside the control plane mutex (mutex member of struct
xdp_sock). This needs some re-work. I'll look into using the newly
introduced state member (with corresponding read/write barriers) for
this.

I'll cook some patch(es) that address this, but first it sounds like I
need to reread [1] two, or three times. At least. ;-)


Thanks,
Björn


[1] https://lwn.net/Articles/793253/


> Thanks,
> Daniel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] xsk: proper socket state check in xsk_poll
  2019-08-20 15:29     ` Björn Töpel
@ 2019-08-20 21:24       ` Daniel Borkmann
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2019-08-20 21:24 UTC (permalink / raw)
  To: Björn Töpel
  Cc: syzbot+c82697e3043781e08802, Alexei Starovoitov, Netdev,
	Björn Töpel, bpf, David Miller, Jesper Dangaard Brouer,
	Jakub Kicinski, John Fastabend, Jonathan Lemon, Martin KaFai Lau,
	LKML, Karlsson, Magnus, Song Liu, syzkaller-bugs, Xdp,
	Yonghong Song, hdanton

On 8/20/19 5:29 PM, Björn Töpel wrote:
> On Tue, 20 Aug 2019 at 16:30, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 8/20/19 12:04 PM, Björn Töpel wrote:
>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>
>>> The poll() implementation for AF_XDP sockets did not perform the
>>> proper state checks, prior accessing the socket umem. This patch fixes
>>> that by performing a xsk_is_bound() check.
>>>
>>> Suggested-by: Hillf Danton <hdanton@sina.com>
>>> Reported-by: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com
>>> Fixes: 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP rings")
>>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
>>> ---
>>>    net/xdp/xsk.c | 14 ++++++++++++--
>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
>>> index ee4428a892fa..08bed5e92af4 100644
>>> --- a/net/xdp/xsk.c
>>> +++ b/net/xdp/xsk.c
>>> @@ -356,13 +356,20 @@ static int xsk_generic_xmit(struct sock *sk, struct msghdr *m,
>>>        return err;
>>>    }
>>>
>>> +static bool xsk_is_bound(struct xdp_sock *xs)
>>> +{
>>> +     struct net_device *dev = READ_ONCE(xs->dev);
>>> +
>>> +     return dev && xs->state == XSK_BOUND;
>>> +}
>>> +
>>>    static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
>>>    {
>>>        bool need_wait = !(m->msg_flags & MSG_DONTWAIT);
>>>        struct sock *sk = sock->sk;
>>>        struct xdp_sock *xs = xdp_sk(sk);
>>>
>>> -     if (unlikely(!xs->dev))
>>> +     if (unlikely(!xsk_is_bound(xs)))
>>>                return -ENXIO;
>>>        if (unlikely(!(xs->dev->flags & IFF_UP)))
>>>                return -ENETDOWN;
>>> @@ -383,6 +390,9 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock,
>>>        struct net_device *dev = xs->dev;
>>>        struct xdp_umem *umem = xs->umem;
>>>
>>> +     if (unlikely(!xsk_is_bound(xs)))
>>> +             return mask;
>>> +
>>>        if (umem->need_wakeup)
>>>                dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
>>>                                                umem->need_wakeup);
>>> @@ -417,7 +427,7 @@ static void xsk_unbind_dev(struct xdp_sock *xs)
>>>    {
>>>        struct net_device *dev = xs->dev;
>>>
>>> -     if (!dev || xs->state != XSK_BOUND)
>>> +     if (!xsk_is_bound(xs))
>>>                return;
>>
>> I think I'm a bit confused by your READ_ONCE() usage. ;-/ I can see why you're
>> using it in xsk_is_bound() above, but then at the same time all the other callbacks
>> like xsk_poll() or xsk_unbind_dev() above have a struct net_device *dev = xs->dev
>> right before the test. Could you elaborate?
> 
> Yes, now I'm confused as well! Digging deeper... I believe there are a
> couple of places in xsk.c that do not have
> READ_ONCE/WRITE_ONCE-correctness. Various xdp_sock members are read
> lock-less outside the control plane mutex (mutex member of struct
> xdp_sock). This needs some re-work. I'll look into using the newly

Right, so even in above two cases, the compiler could have refetched, e.g.
dev variable could have first been NULL, but xsk_is_bound() later returns
true.

> introduced state member (with corresponding read/write barriers) for
> this.
> 
> I'll cook some patch(es) that address this, but first it sounds like I
> need to reread [1] two, or three times. At least. ;-)
> 
> 
> Thanks,
> Björn
> 
> 
> [1] https://lwn.net/Articles/793253/
> 
> 
>> Thanks,
>> Daniel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: general protection fault in xsk_poll
       [not found] <20190820033154.9112-1-hdanton@sina.com>
@ 2019-08-20  9:17 ` Björn Töpel
  0 siblings, 0 replies; 6+ messages in thread
From: Björn Töpel @ 2019-08-20  9:17 UTC (permalink / raw)
  To: Hillf Danton, syzbot
  Cc: ast, bpf, daniel, davem, hawk, jakub.kicinski, john.fastabend,
	jonathan.lemon, kafai, linux-kernel, magnus.karlsson, netdev,
	songliubraving, syzkaller-bugs, xdp-newbies, yhs

On 2019-08-20 05:31, Hillf Danton wrote:
> 
> On Mon, 19 Aug 2019 18:18:06 -0700
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit:    da657043 Add linux-next specific files for 20190819
>> git tree:       linux-next
>> console output: https://syzkaller.appspot.com/x/log.txt?x=16af124c600000
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=739a9b3ab3d8c770
>> dashboard link: https://syzkaller.appspot.com/bug?extid=c82697e3043781e08802
>> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=109e1922600000
>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1445bf02600000
>>
>> The bug was bisected to:
>>
>> commit 77cd0d7b3f257fd0e3096b4fdcff1a7d38e99e10
>> Author: Magnus Karlsson <magnus.karlsson@intel.com>
>> Date:   Wed Aug 14 07:27:17 2019 +0000
>>
>>       xsk: add support for need_wakeup flag in AF_XDP rings
>>
>> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=15e1ea4c600000
>> final crash:    https://syzkaller.appspot.com/x/report.txt?x=17e1ea4c600000
>> console output: https://syzkaller.appspot.com/x/log.txt?x=13e1ea4c600000
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+c82697e3043781e08802@syzkaller.appspotmail.com
>> Fixes: 77cd0d7b3f25 ("xsk: add support for need_wakeup flag in AF_XDP rings")
>>
>> kasan: CONFIG_KASAN_INLINE enabled
>> kasan: GPF could be caused by NULL-ptr deref or user memory access
>> general protection fault: 0000 [#1] PREEMPT SMP KASAN
>> CPU: 1 PID: 7959 Comm: syz-executor611 Not tainted 5.3.0-rc5-next-20190819 #68
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
>> Google 01/01/2011
>> RIP: 0010:xsk_poll+0x95/0x540 net/xdp/xsk.c:386
>> Code: 80 3c 02 00 0f 85 70 04 00 00 4c 8b a3 88 04 00 00 48 b8 00 00 00 00
>> 00 fc ff df 49 8d bc 24 96 00 00 00 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48
>> 89 fa 83 e2 07 38 d0 7f 08 84 c0 0f 85 bf 03 00 00
>> RSP: 0018:ffff8880926f7850 EFLAGS: 00010207
>> RAX: dffffc0000000000 RBX: ffff88809a141700 RCX: ffffffff859b07aa
>> RDX: 0000000000000012 RSI: ffffffff859b07c4 RDI: 0000000000000096
>> RBP: ffff8880926f7880 R08: ffff88809698a580 R09: ffffed1013428329
>> R10: ffffed1013428328 R11: ffff88809a141947 R12: 0000000000000000
>> R13: 0000000000000304 R14: ffff888095d4d840 R15: ffff888092bdd020
>> FS:  0000555557529880(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000020000280 CR3: 0000000098281000 CR4: 00000000001406e0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>>    sock_poll+0x15e/0x480 net/socket.c:1256
>>    vfs_poll include/linux/poll.h:90 [inline]
>>    do_pollfd fs/select.c:859 [inline]
>>    do_poll fs/select.c:907 [inline]
>>    do_sys_poll+0x7c2/0xde0 fs/select.c:1001
>>    __do_sys_ppoll fs/select.c:1101 [inline]
>>    __se_sys_ppoll fs/select.c:1081 [inline]
>>    __x64_sys_ppoll+0x259/0x310 fs/select.c:1081
>>    do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
>>    entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> RIP: 0033:0x440159
>> Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
>> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
>> ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
>> RSP: 002b:00007ffd9fbd16e8 EFLAGS: 00000246 ORIG_RAX: 000000000000010f
>> RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440159
>> RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000020000280
>> RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8
>> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004019e0
>> R13: 0000000000401a70 R14: 0000000000000000 R15: 0000000000000000
>> Modules linked in:
>> ---[ end trace da907175426b4065 ]---
> 
> Add umem check.
> 
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -381,9 +381,9 @@ static unsigned int xsk_poll(struct file
>   	struct sock *sk = sock->sk;
>   	struct xdp_sock *xs = xdp_sk(sk);
>   	struct net_device *dev = xs->dev;
> -	struct xdp_umem *umem = xs->umem;
> +	struct xdp_umem *umem = READ_ONCE(xs->umem);
>   
> -	if (umem->need_wakeup)
> +	if (umem && umem->need_wakeup)
>   		dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
>   						umem->need_wakeup);
>   
> --
> 

Thanks!

What do you think about making it a bit more generic, like:

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index ee4428a892fa..08bed5e92af4 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -356,13 +356,20 @@ static int xsk_generic_xmit(struct sock *sk, 
struct msghdr *m,
  	return err;
  }

+static bool xsk_is_bound(struct xdp_sock *xs)
+{
+	struct net_device *dev = READ_ONCE(xs->dev);
+
+	return dev && xs->state == XSK_BOUND;
+}
+
  static int xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t 
total_len)
  {
  	bool need_wait = !(m->msg_flags & MSG_DONTWAIT);
  	struct sock *sk = sock->sk;
  	struct xdp_sock *xs = xdp_sk(sk);

-	if (unlikely(!xs->dev))
+	if (unlikely(!xsk_is_bound(xs)))
  		return -ENXIO;
  	if (unlikely(!(xs->dev->flags & IFF_UP)))
  		return -ENETDOWN;
@@ -383,6 +390,9 @@ static unsigned int xsk_poll(struct file *file, 
struct socket *sock,
  	struct net_device *dev = xs->dev;
  	struct xdp_umem *umem = xs->umem;

+	if (unlikely(!xsk_is_bound(xs)))
+		return mask;
+
  	if (umem->need_wakeup)
  		dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
  						umem->need_wakeup);
@@ -417,7 +427,7 @@ static void xsk_unbind_dev(struct xdp_sock *xs)
  {
  	struct net_device *dev = xs->dev;

-	if (!dev || xs->state != XSK_BOUND)
+	if (!xsk_is_bound(xs))
  		return;

  	xs->state = XSK_UNBOUND;

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-08-20 21:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-20  1:18 general protection fault in xsk_poll syzbot
2019-08-20 10:04 ` [PATCH bpf-next] xsk: proper socket state check " Björn Töpel
2019-08-20 14:30   ` Daniel Borkmann
2019-08-20 15:29     ` Björn Töpel
2019-08-20 21:24       ` Daniel Borkmann
     [not found] <20190820033154.9112-1-hdanton@sina.com>
2019-08-20  9:17 ` general protection fault " Björn Töpel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).