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

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

[David Ahern]

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(+)

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;
 
-- 
2.25.1

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

[Ido Schimmel]

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,

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

[David Ahern]

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,

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

[Ido Schimmel]

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

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

[Paolo Abeni]

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