| Message ID | 20221005181257.8897-1-dsahern@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ipv4: Handle attempt to delete multipath route when fib_info contains an nh reference | expand |
On Wed, Oct 05, 2022 at 12:12:57PM -0600, David Ahern wrote: > Gwangun Jung reported a slab-out-of-bounds access in fib_nh_match: > fib_nh_match+0xf98/0x1130 linux-6.0-rc7/net/ipv4/fib_semantics.c:961 > fib_table_delete+0x5f3/0xa40 linux-6.0-rc7/net/ipv4/fib_trie.c:1753 > inet_rtm_delroute+0x2b3/0x380 linux-6.0-rc7/net/ipv4/fib_frontend.c:874 > > Separate nexthop objects are mutually exclusive with the legacy > multipath spec. Fix fib_nh_match to return if the config for the > to be deleted route contains a multipath spec while the fib_info > is using a nexthop object. Cool bug... Managed to reproduce with: # ip nexthop add id 1 blackhole # ip route add 192.0.2.0/24 nhid 1 # ip route del 192.0.2.0/24 nexthop via 198.51.100.1 nexthop via 198.51.100.2 Maybe add to tools/testing/selftests/net/fib_nexthops.sh ? Checked IPv6 and I don't think we can hit it there, but I will double check tomorrow morning. > > Fixes: 493ced1ac47c ("ipv4: Allow routes to use nexthop objects") > Reported-by: Gwangun Jung <exsociety@gmail.com> > Signed-off-by: David Ahern <dsahern@kernel.org> > --- > net/ipv4/fib_semantics.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c > index 2dc97583d279..17caa73f57e6 100644 > --- a/net/ipv4/fib_semantics.c > +++ b/net/ipv4/fib_semantics.c > @@ -926,6 +926,10 @@ int fib_nh_match(struct net *net, struct fib_config *cfg, struct fib_info *fi, > if (!cfg->fc_mp) > return 0; > > + /* multipath spec and nexthop id are mutually exclusive */ > + if (fi->nh) > + return 1; > + > rtnh = cfg->fc_mp; > remaining = cfg->fc_mp_len; There is already such a check above for the non-multipath check, maybe we can just move it up to cover both cases? Something like: diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c index 2dc97583d279..e9a7f70a54df 100644 --- a/net/ipv4/fib_semantics.c +++ b/net/ipv4/fib_semantics.c @@ -888,13 +888,13 @@ int fib_nh_match(struct net *net, struct fib_config *cfg, struct fib_info *fi, return 1; } + /* cannot match on nexthop object attributes */ + if (fi->nh) + return 1; + if (cfg->fc_oif || cfg->fc_gw_family) { struct fib_nh *nh; - /* cannot match on nexthop object attributes */ - if (fi->nh) - return 1; - nh = fib_info_nh(fi, 0); if (cfg->fc_encap) { if (fib_encap_match(net, cfg->fc_encap_type,
On 10/5/22 1:08 PM, Ido Schimmel wrote: > On Wed, Oct 05, 2022 at 12:12:57PM -0600, David Ahern wrote: >> Gwangun Jung reported a slab-out-of-bounds access in fib_nh_match: >> fib_nh_match+0xf98/0x1130 linux-6.0-rc7/net/ipv4/fib_semantics.c:961 >> fib_table_delete+0x5f3/0xa40 linux-6.0-rc7/net/ipv4/fib_trie.c:1753 >> inet_rtm_delroute+0x2b3/0x380 linux-6.0-rc7/net/ipv4/fib_frontend.c:874 >> >> Separate nexthop objects are mutually exclusive with the legacy >> multipath spec. Fix fib_nh_match to return if the config for the >> to be deleted route contains a multipath spec while the fib_info >> is using a nexthop object. > > Cool bug... Managed to reproduce with: > > # ip nexthop add id 1 blackhole > # ip route add 192.0.2.0/24 nhid 1 > # ip route del 192.0.2.0/24 nexthop via 198.51.100.1 nexthop via 198.51.100.2 that's what I did as well. > > Maybe add to tools/testing/selftests/net/fib_nexthops.sh ? I have one in my tree, but in my tests nothing blew up or threw an error message. It requires KASAN to be enabled otherwise the test does not trigger anything. > > Checked IPv6 and I don't think we can hit it there, but I will double > check tomorrow morning. > >> >> Fixes: 493ced1ac47c ("ipv4: Allow routes to use nexthop objects") >> Reported-by: Gwangun Jung <exsociety@gmail.com> >> Signed-off-by: David Ahern <dsahern@kernel.org> >> --- >> net/ipv4/fib_semantics.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> > > There is already such a check above for the non-multipath check, maybe > we can just move it up to cover both cases? Something like: > > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c > index 2dc97583d279..e9a7f70a54df 100644 > --- a/net/ipv4/fib_semantics.c > +++ b/net/ipv4/fib_semantics.c > @@ -888,13 +888,13 @@ int fib_nh_match(struct net *net, struct fib_config *cfg, struct fib_info *fi, > return 1; > } > > + /* cannot match on nexthop object attributes */ > + if (fi->nh) > + return 1; that should work as well. I went with the simplest change that would definitely not have a negative impact on backports. > + > if (cfg->fc_oif || cfg->fc_gw_family) { > struct fib_nh *nh; > > - /* cannot match on nexthop object attributes */ > - if (fi->nh) > - return 1; > - > nh = fib_info_nh(fi, 0); > if (cfg->fc_encap) { > if (fib_encap_match(net, cfg->fc_encap_type,
On Wed, Oct 05, 2022 at 01:27:59PM -0600, David Ahern wrote: > On 10/5/22 1:08 PM, Ido Schimmel wrote: > > On Wed, Oct 05, 2022 at 12:12:57PM -0600, David Ahern wrote: > >> Gwangun Jung reported a slab-out-of-bounds access in fib_nh_match: > >> fib_nh_match+0xf98/0x1130 linux-6.0-rc7/net/ipv4/fib_semantics.c:961 > >> fib_table_delete+0x5f3/0xa40 linux-6.0-rc7/net/ipv4/fib_trie.c:1753 > >> inet_rtm_delroute+0x2b3/0x380 linux-6.0-rc7/net/ipv4/fib_frontend.c:874 > >> > >> Separate nexthop objects are mutually exclusive with the legacy > >> multipath spec. Fix fib_nh_match to return if the config for the > >> to be deleted route contains a multipath spec while the fib_info > >> is using a nexthop object. > > > > Cool bug... Managed to reproduce with: > > > > # ip nexthop add id 1 blackhole > > # ip route add 192.0.2.0/24 nhid 1 > > # ip route del 192.0.2.0/24 nexthop via 198.51.100.1 nexthop via 198.51.100.2 > > that's what I did as well. :) > > > > > Maybe add to tools/testing/selftests/net/fib_nexthops.sh ? > > I have one in my tree, but in my tests nothing blew up or threw an error > message. It requires KASAN to be enabled otherwise the test does not > trigger anything. That's fine. At least our team is running this test as part of regression on a variety of machines, some of which run a debug kernel with KASAN enabled. The system knows to fail a test if a splat was emitted to the kernel log. > > > > > Checked IPv6 and I don't think we can hit it there, but I will double > > check tomorrow morning. > > > >> > >> Fixes: 493ced1ac47c ("ipv4: Allow routes to use nexthop objects") > >> Reported-by: Gwangun Jung <exsociety@gmail.com> > >> Signed-off-by: David Ahern <dsahern@kernel.org> > >> --- > >> net/ipv4/fib_semantics.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > > > > > There is already such a check above for the non-multipath check, maybe > > we can just move it up to cover both cases? Something like: > > > > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c > > index 2dc97583d279..e9a7f70a54df 100644 > > --- a/net/ipv4/fib_semantics.c > > +++ b/net/ipv4/fib_semantics.c > > @@ -888,13 +888,13 @@ int fib_nh_match(struct net *net, struct fib_config *cfg, struct fib_info *fi, > > return 1; > > } > > > > + /* cannot match on nexthop object attributes */ > > + if (fi->nh) > > + return 1; > > that should work as well. I went with the simplest change that would > definitely not have a negative impact on backports. Ha, I see this hunk was added by 6bf92d70e690b. Given how overzealous the AUTOSEL bot is, I don't expect this fix to be missing from stable kernels. If you also blame 6bf92d70e690b (given it was apparently incomplete), then it makes it clear to anyone doing the backport that 6bf92d70e690b is needed as well. I prefer having the check at the beginning because a) It would have avoided this bug b) It directly follows the 'cfg->fc_nh_id' check, making it clear that we never match if nexthop ID was not specified, but we got a FIB info with a nexthop object. > > > > + > > if (cfg->fc_oif || cfg->fc_gw_family) { > > struct fib_nh *nh; > > > > - /* cannot match on nexthop object attributes */ > > - if (fi->nh) > > - return 1; > > - > > nh = fib_info_nh(fi, 0); > > if (cfg->fc_encap) { > > if (fib_encap_match(net, cfg->fc_encap_type, >
On Thu, 2022-10-06 at 09:49 +0300, Ido Schimmel wrote: > On Wed, Oct 05, 2022 at 01:27:59PM -0600, David Ahern wrote: > > On 10/5/22 1:08 PM, Ido Schimmel wrote: > > > On Wed, Oct 05, 2022 at 12:12:57PM -0600, David Ahern wrote: > > > > Gwangun Jung reported a slab-out-of-bounds access in fib_nh_match: > > > > fib_nh_match+0xf98/0x1130 linux-6.0-rc7/net/ipv4/fib_semantics.c:961 > > > > fib_table_delete+0x5f3/0xa40 linux-6.0-rc7/net/ipv4/fib_trie.c:1753 > > > > inet_rtm_delroute+0x2b3/0x380 linux-6.0-rc7/net/ipv4/fib_frontend.c:874 > > > > > > > > Separate nexthop objects are mutually exclusive with the legacy > > > > multipath spec. Fix fib_nh_match to return if the config for the > > > > to be deleted route contains a multipath spec while the fib_info > > > > is using a nexthop object. > > > > > > Cool bug... Managed to reproduce with: > > > > > > # ip nexthop add id 1 blackhole > > > # ip route add 192.0.2.0/24 nhid 1 > > > # ip route del 192.0.2.0/24 nexthop via 198.51.100.1 nexthop via 198.51.100.2 > > > > that's what I did as well. > > :) > > > > > > > > > Maybe add to tools/testing/selftests/net/fib_nexthops.sh ? > > > > I have one in my tree, but in my tests nothing blew up or threw an error > > message. It requires KASAN to be enabled otherwise the test does not > > trigger anything. > > That's fine. At least our team is running this test as part of > regression on a variety of machines, some of which run a debug kernel > with KASAN enabled. The system knows to fail a test if a splat was > emitted to the kernel log. > > > > > > > > > Checked IPv6 and I don't think we can hit it there, but I will double > > > check tomorrow morning. > > > > > > > > > > > Fixes: 493ced1ac47c ("ipv4: Allow routes to use nexthop objects") > > > > Reported-by: Gwangun Jung <exsociety@gmail.com> > > > > Signed-off-by: David Ahern <dsahern@kernel.org> > > > > --- > > > > net/ipv4/fib_semantics.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > > > There is already such a check above for the non-multipath check, maybe > > > we can just move it up to cover both cases? Something like: > > > > > > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c > > > index 2dc97583d279..e9a7f70a54df 100644 > > > --- a/net/ipv4/fib_semantics.c > > > +++ b/net/ipv4/fib_semantics.c > > > @@ -888,13 +888,13 @@ int fib_nh_match(struct net *net, struct fib_config *cfg, struct fib_info *fi, > > > return 1; > > > } > > > > > > + /* cannot match on nexthop object attributes */ > > > + if (fi->nh) > > > + return 1; > > > > that should work as well. I went with the simplest change that would > > definitely not have a negative impact on backports. > > Ha, I see this hunk was added by 6bf92d70e690b. Given how overzealous > the AUTOSEL bot is, I don't expect this fix to be missing from stable > kernels. If you also blame 6bf92d70e690b (given it was apparently > incomplete), then it makes it clear to anyone doing the backport that > 6bf92d70e690b is needed as well. > > I prefer having the check at the beginning because a) It would have > avoided this bug b) It directly follows the 'cfg->fc_nh_id' check, > making it clear that we never match if nexthop ID was not specified, but > we got a FIB info with a nexthop object. I also think this other option is better, and I think the backport effort will be mostly unaffected: a kernel needing 6bf92d70e690b but not the above fix would be quite a strange/completely unexpected subject for stable backport. Could you please consider a v2 moving the check upwards? Thanks! Paolo
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c index 2dc97583d279..17caa73f57e6 100644 --- a/net/ipv4/fib_semantics.c +++ b/net/ipv4/fib_semantics.c @@ -926,6 +926,10 @@ int fib_nh_match(struct net *net, struct fib_config *cfg, struct fib_info *fi, if (!cfg->fc_mp) return 0; + /* multipath spec and nexthop id are mutually exclusive */ + if (fi->nh) + return 1; + rtnh = cfg->fc_mp; remaining = cfg->fc_mp_len;
Gwangun Jung reported a slab-out-of-bounds access in fib_nh_match: fib_nh_match+0xf98/0x1130 linux-6.0-rc7/net/ipv4/fib_semantics.c:961 fib_table_delete+0x5f3/0xa40 linux-6.0-rc7/net/ipv4/fib_trie.c:1753 inet_rtm_delroute+0x2b3/0x380 linux-6.0-rc7/net/ipv4/fib_frontend.c:874 Separate nexthop objects are mutually exclusive with the legacy multipath spec. Fix fib_nh_match to return if the config for the to be deleted route contains a multipath spec while the fib_info is using a nexthop object. Fixes: 493ced1ac47c ("ipv4: Allow routes to use nexthop objects") Reported-by: Gwangun Jung <exsociety@gmail.com> Signed-off-by: David Ahern <dsahern@kernel.org> --- net/ipv4/fib_semantics.c | 4 ++++ 1 file changed, 4 insertions(+)