| Message ID | 20180603133546.28635-1-edumazet@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: ipv6: prevent use after free in ip6_route_mpath_notify() | expand |
On 6/3/18 7:35 AM, Eric Dumazet wrote: > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config *cfg, > > err_nh = NULL; > list_for_each_entry(nh, &rt6_nh_list, next) { > + dst_release(&rt_last->dst); > rt_last = nh->rt6_info; > + dst_hold(&rt_last->dst); > err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack); > /* save reference to first route for notification */ > if (!rt_notif && !err) > @@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg, > list_del(&nh->next); > kfree(nh); > } > - > + dst_release(&rt_last->dst); > return err; > } Since the rtnl lock is held, a successfully inserted route can not be removed until ip6_route_multipath_add finishes. This is a simpler change that works with net-next as well: diff --git a/net/ipv6/route.c b/net/ipv6/route.c index f4d61736c41a..1684197c189f 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -4263,11 +4263,12 @@ static int ip6_route_multipath_add(struct fib6_config *cfg, err_nh = NULL; list_for_each_entry(nh, &rt6_nh_list, next) { - rt_last = nh->rt6_info; err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack); /* save reference to first route for notification */ if (!rt_notif && !err) rt_notif = nh->rt6_info; + if (!err) + rt_last = nh->rt6_info; /* nh->rt6_info is used or freed at this point, reset to NULL*/ nh->rt6_info = NULL; Is there a reproducer for the syzbot case?
On 06/03/2018 07:01 AM, David Ahern wrote: > On 6/3/18 7:35 AM, Eric Dumazet wrote: >> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >> index f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889 100644 >> --- a/net/ipv6/route.c >> +++ b/net/ipv6/route.c >> @@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config *cfg, >> >> err_nh = NULL; >> list_for_each_entry(nh, &rt6_nh_list, next) { >> + dst_release(&rt_last->dst); >> rt_last = nh->rt6_info; >> + dst_hold(&rt_last->dst); >> err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack); >> /* save reference to first route for notification */ >> if (!rt_notif && !err) >> @@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg, >> list_del(&nh->next); >> kfree(nh); >> } >> - >> + dst_release(&rt_last->dst); >> return err; >> } > > Since the rtnl lock is held, a successfully inserted route can not be > removed until ip6_route_multipath_add finishes. This is a simpler change > that works with net-next as well: Your patch changes the intent of your original commit. It seems you wanted rt_last to point to the last attempted insertion, not the last successful one ? Or have I misunderstood, and not only we had a use-after-free, but also a semantic error ? > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index f4d61736c41a..1684197c189f 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -4263,11 +4263,12 @@ static int ip6_route_multipath_add(struct > fib6_config *cfg, > > err_nh = NULL; > list_for_each_entry(nh, &rt6_nh_list, next) { > - rt_last = nh->rt6_info; > err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack); > /* save reference to first route for notification */ > if (!rt_notif && !err) > rt_notif = nh->rt6_info; > + if (!err) > + rt_last = nh->rt6_info; > > /* nh->rt6_info is used or freed at this point, reset to > NULL*/ > nh->rt6_info = NULL; > > > Is there a reproducer for the syzbot case? Not yet.
On 6/3/18 8:01 AM, David Ahern wrote:
> Is there a reproducer for the syzbot case?
One reproducer is to insert a route and then add a multipath route that
has a duplicate nexthop.e.g,:
ip -6 ro add vrf red 2001:db8:101::/64 nexthop via 2001:db8:1::2
ip -6 ro append vrf red 2001:db8:101::/64 nexthop via 2001:db8:1::4
nexthop via 2001:db8:1::2
Current net and next-next generates the trace; with the fix I proposed I
don't see it on either branch and I do see the expected notifications to
userspace.
On 6/3/18 8:31 AM, Eric Dumazet wrote: > > > On 06/03/2018 07:01 AM, David Ahern wrote: >> On 6/3/18 7:35 AM, Eric Dumazet wrote: >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >>> index f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889 100644 >>> --- a/net/ipv6/route.c >>> +++ b/net/ipv6/route.c >>> @@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config *cfg, >>> >>> err_nh = NULL; >>> list_for_each_entry(nh, &rt6_nh_list, next) { >>> + dst_release(&rt_last->dst); >>> rt_last = nh->rt6_info; >>> + dst_hold(&rt_last->dst); >>> err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack); >>> /* save reference to first route for notification */ >>> if (!rt_notif && !err) >>> @@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg, >>> list_del(&nh->next); >>> kfree(nh); >>> } >>> - >>> + dst_release(&rt_last->dst); >>> return err; >>> } >> >> Since the rtnl lock is held, a successfully inserted route can not be >> removed until ip6_route_multipath_add finishes. This is a simpler change >> that works with net-next as well: > > Your patch changes the intent of your original commit. > > It seems you wanted rt_last to point to the last attempted insertion, > not the last successful one ? The note in ip6_route_mpath_notify explains it: /* if this is an APPEND route, then rt points to the first route * inserted and rt_last points to last route inserted. Userspace > > Or have I misunderstood, and not only we had a use-after-free, but also > a semantic error ? It was a mistake to set rt_last before checking err. So the use-after-free exposed the semantic error.
On 06/03/2018 07:46 AM, David Ahern wrote: > It was a mistake to set rt_last before checking err. So the > use-after-free exposed the semantic error. > SGTM, please send the formal patch then, thanks !
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config *cfg, err_nh = NULL; list_for_each_entry(nh, &rt6_nh_list, next) { + dst_release(&rt_last->dst); rt_last = nh->rt6_info; + dst_hold(&rt_last->dst); err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack); /* save reference to first route for notification */ if (!rt_notif && !err) @@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg, list_del(&nh->next); kfree(nh); } - + dst_release(&rt_last->dst); return err; }