| Message ID | 20241101044828.55960-1-03zouyi09.25@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipv6: ip6_fib: fix possible null-pointer-dereference in ipv6_route_native_seq_show | expand |
> 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
> Check if fib6_nh is non-NULL before accessing fib6_nh->fib_nh_gw_family > in ipv6_route_native_seq_show() to prevent a null-pointer dereference. > Assign dev as dev = fib6_nh ? fib6_nh->fib_nh_dev : NULL to ensure safe > handling when nexthop_fib6_nh(rt->nh) returns NULL. Would you like to send another improved patch version (instead of a reply) according to a change review? Regards, Markus
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
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.
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 : "");
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(-)