lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 06 Oct 2022 09:29:36 +0200 From: Paolo Abeni <pabeni@...hat.com> To: Ido Schimmel <idosch@...sch.org>, David Ahern <dsahern@...nel.org> Cc: kuba@...nel.org, davem@...emloft.net, netdev@...r.kernel.org, Gwangun Jung <exsociety@...il.com> Subject: Re: [PATCH net] ipv4: Handle attempt to delete multipath route when fib_info contains an nh reference 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@...il.com> > > > > Signed-off-by: David Ahern <dsahern@...nel.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
Powered by blists - more mailing lists