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

[Ido Schimmel <idosch@idosch.org>]

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,
>