| Message ID | 1502795391.4936.60.camel@edumazet-glaptop3.roam.corp.google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 15 Aug 2017 04:09:51 -0700 > From: Eric Dumazet <edumazet@google.com> > > Based on a syzkaller report [1], I found that a per cpu allocation > failure in snmp6_alloc_dev() would then lead to NULL dereference in > ip6_route_dev_notify(). > > It seems this is a very old bug, thus no Fixes tag in this submission. > > Let's add in6_dev_put_clear() helper, as we will probably use > it elsewhere (once available/present in net-next) ... > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> Applied and queued up for -stable, thanks Eric.
On Tue, Aug 15, 2017 at 4:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > Based on a syzkaller report [1], I found that a per cpu allocation > failure in snmp6_alloc_dev() would then lead to NULL dereference in > ip6_route_dev_notify(). > > It seems this is a very old bug, thus no Fixes tag in this submission. It should be introduced by my commit which introduces these in6_dev_put(). > > Let's add in6_dev_put_clear() helper, as we will probably use > it elsewhere (once available/present in net-next) > I don't understand why not just check ->rt6i_idev against NULL for -net? Ideally, we should let notifiers handle this, when we fail in one of those notifiers, we should only rollback those succeeded rather than all of them, but this may require more changes. Thanks.
On Wed, 2017-08-16 at 11:50 -0700, Cong Wang wrote: > On Tue, Aug 15, 2017 at 4:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > > > > Based on a syzkaller report [1], I found that a per cpu allocation > > failure in snmp6_alloc_dev() would then lead to NULL dereference in > > ip6_route_dev_notify(). > > > > It seems this is a very old bug, thus no Fixes tag in this submission. > > > It should be introduced by my commit which introduces these > in6_dev_put(). > Sorry, which commit exactly ? I got the issue on 4.3 up to latest 4.12
On Wed, 2017-08-16 at 12:15 -0700, Eric Dumazet wrote: > On Wed, 2017-08-16 at 11:50 -0700, Cong Wang wrote: > > On Tue, Aug 15, 2017 at 4:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > From: Eric Dumazet <edumazet@google.com> > > > > > > Based on a syzkaller report [1], I found that a per cpu allocation > > > failure in snmp6_alloc_dev() would then lead to NULL dereference in > > > ip6_route_dev_notify(). > > > > > > It seems this is a very old bug, thus no Fixes tag in this submission. > > > > > > It should be introduced by my commit which introduces these > > in6_dev_put(). > > > > Sorry, which commit exactly ? > > I got the issue on 4.3 up to latest 4.12 Oh you're right, this was because I had backported 242d3a49a2a1a71d8eb9f953db1bcaa9d698ce00 into my trees a while back... So the bug was only added in 4.12
On Wed, Aug 16, 2017 at 12:37 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Wed, 2017-08-16 at 12:15 -0700, Eric Dumazet wrote: >> On Wed, 2017-08-16 at 11:50 -0700, Cong Wang wrote: >> > On Tue, Aug 15, 2017 at 4:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: >> > > From: Eric Dumazet <edumazet@google.com> >> > > >> > > Based on a syzkaller report [1], I found that a per cpu allocation >> > > failure in snmp6_alloc_dev() would then lead to NULL dereference in >> > > ip6_route_dev_notify(). >> > > >> > > It seems this is a very old bug, thus no Fixes tag in this submission. >> > >> > >> > It should be introduced by my commit which introduces these >> > in6_dev_put(). >> > >> >> Sorry, which commit exactly ? >> >> I got the issue on 4.3 up to latest 4.12 > > Oh you're right, this was because I had backported > 242d3a49a2a1a71d8eb9f953db1bcaa9d698ce00 into my trees a while back... > > So the bug was only added in 4.12 Yes this is correct.
diff --git a/include/net/addrconf.h b/include/net/addrconf.h index 6df79e96a780..f44ff2476758 100644 --- a/include/net/addrconf.h +++ b/include/net/addrconf.h @@ -336,6 +336,16 @@ static inline void in6_dev_put(struct inet6_dev *idev) in6_dev_finish_destroy(idev); } +static inline void in6_dev_put_clear(struct inet6_dev **pidev) +{ + struct inet6_dev *idev = *pidev; + + if (idev) { + in6_dev_put(idev); + *pidev = NULL; + } +} + static inline void __in6_dev_put(struct inet6_dev *idev) { refcount_dec(&idev->refcnt); diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 99d4727f2b18..94d6a13d47f0 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -3721,10 +3721,10 @@ static int ip6_route_dev_notify(struct notifier_block *this, /* NETDEV_UNREGISTER could be fired for multiple times by * netdev_wait_allrefs(). Make sure we only call this once. */ - in6_dev_put(net->ipv6.ip6_null_entry->rt6i_idev); + in6_dev_put_clear(&net->ipv6.ip6_null_entry->rt6i_idev); #ifdef CONFIG_IPV6_MULTIPLE_TABLES - in6_dev_put(net->ipv6.ip6_prohibit_entry->rt6i_idev); - in6_dev_put(net->ipv6.ip6_blk_hole_entry->rt6i_idev); + in6_dev_put_clear(&net->ipv6.ip6_prohibit_entry->rt6i_idev); + in6_dev_put_clear(&net->ipv6.ip6_blk_hole_entry->rt6i_idev); #endif }