ipv6: ip6_fib: fix possible null-pointer-dereference in ipv6_route_native_seq_show

[PATCH] ipv6: ip6_fib: fix possible null-pointer-dereference in ipv6_route_native_seq_show

Posted by Yi Zou 3 weeks, 2 days ago

In the ipv6_route_native_seq_show function, the fib6_nh variable
is assigned the value from nexthop_fib6_nh(rt->nh), which could
return NULL. This creates a risk of a null-pointer-dereference
when accessing fib6_nh->fib_nh_gw_family. This can be resolved by
checking if fib6_nh is non-NULL before accessing fib6_nh->fib_nh_gw_family
 and assign dev using dev = fib6_nh ? fib6_nh->fib_nh_dev : NULL;
to prevent null-pointer dereference errors.

Signed-off-by: Yi Zou <03zouyi09.25@gmail.com>
---
 net/ipv6/ip6_fib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index eb111d20615c..6632ab65d206 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -2555,14 +2555,14 @@ static int ipv6_route_native_seq_show(struct seq_file *seq, void *v)
 #else
 	seq_puts(seq, "00000000000000000000000000000000 00 ");
 #endif
-	if (fib6_nh->fib_nh_gw_family) {
+	if (fib6_nh && fib6_nh->fib_nh_gw_family) {
 		flags |= RTF_GATEWAY;
 		seq_printf(seq, "%pi6", &fib6_nh->fib_nh_gw6);
 	} else {
 		seq_puts(seq, "00000000000000000000000000000000");
 	}
 
-	dev = fib6_nh->fib_nh_dev;
+	dev = fib6_nh ? fib6_nh->fib_nh_dev : NULL;
 	seq_printf(seq, " %08x %08x %08x %08x %8s\n",
 		   rt->fib6_metric, refcount_read(&rt->fib6_ref), 0,
 		   flags, dev ? dev->name : "");
-- 
2.44.0

Re: [PATCH] ipv6: ip6_fib: fix possible null-pointer-dereference in ipv6_route_native_seq_show

Posted by Paolo Abeni 2 weeks, 5 days ago


On 11/1/24 05:48, Yi Zou wrote:
> In the ipv6_route_native_seq_show function, the fib6_nh variable
> is assigned the value from nexthop_fib6_nh(rt->nh), which could
> return NULL. This creates a risk of a null-pointer-dereference
> when accessing fib6_nh->fib_nh_gw_family. This can be resolved by
> checking if fib6_nh is non-NULL before accessing fib6_nh->fib_nh_gw_family
>  and assign dev using dev = fib6_nh ? fib6_nh->fib_nh_dev : NULL;
> to prevent null-pointer dereference errors.
> 
> Signed-off-by: Yi Zou <03zouyi09.25@gmail.com>

Please send a new revision, including a the target tree in the subj
prefix - in this case 'net' and a suitable Fixes tag.

/P

Re: [PATCH] ipv6: ip6_fib: fix possible null-pointer-dereference in ipv6_route_native_seq_show

Posted by David Ahern 2 weeks, 5 days ago

On 11/5/24 5:28 AM, Paolo Abeni wrote:
> 
> 
> On 11/1/24 05:48, Yi Zou wrote:
>> In the ipv6_route_native_seq_show function, the fib6_nh variable
>> is assigned the value from nexthop_fib6_nh(rt->nh), which could
>> return NULL. This creates a risk of a null-pointer-dereference
>> when accessing fib6_nh->fib_nh_gw_family. This can be resolved by
>> checking if fib6_nh is non-NULL before accessing fib6_nh->fib_nh_gw_family
>>  and assign dev using dev = fib6_nh ? fib6_nh->fib_nh_dev : NULL;
>> to prevent null-pointer dereference errors.
>>
>> Signed-off-by: Yi Zou <03zouyi09.25@gmail.com>
> 
> Please send a new revision, including a the target tree in the subj
> prefix - in this case 'net' and a suitable Fixes tag.
> 
> /P
> 

I would also like to understand why you believe NULL can really happen -
excluding memory corruption or custom patches to a kernel. If you look
at the make up of nexthop_fib6_nh it is defensive around bugs elsewhere
(nhsel > number of nexthops) and future changes (support for ipv6
nexthops that are not IPV6 addresses).

That comment applies to all of these patches around nexthop_fib6_nh
possibly returning NULL.

Re: [PATCH] ipv6: ip6_fib: fix possible null-pointer-dereference in ipv6_route_native_seq_show

Posted by Markus Elfring 3 weeks, 2 days ago

> In the ipv6_route_native_seq_show function, the fib6_nh variable
> is assigned the value from nexthop_fib6_nh(rt->nh), which could
> return NULL. This creates a risk of a null-pointer-dereference
> when accessing fib6_nh->fib_nh_gw_family. This can be resolved by
> checking if fib6_nh is non-NULL before accessing fib6_nh->fib_nh_gw_family
> and assign dev using dev = fib6_nh ? fib6_nh->fib_nh_dev : NULL;
> to prevent null-pointer dereference errors.
…

* Please choose an imperative wording for an improved change description.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc5#n94

* Would you like to append parentheses to function names?

* I suggest to omit the word “possible” from the summary phrase.

* How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc5#n145


Regards,
Markus