Re: [PATCH net] ipv4: Handle attempt to delete multipath route when fib_info contains an nh reference

[Paolo Abeni <pabeni@redhat.com>]

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