[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z-P5vvrdA5MHMW_o@krikkit>
Date: Wed, 26 Mar 2025 13:57:34 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Purva Yeshi <purvayeshi550@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>,
David Ahern <dsahern@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: ipv6: Fix NULL dereference in ipv6_route_check_nh
2025-03-26, 16:22:15 +0530, Purva Yeshi wrote:
> Fix Smatch-detected error:
> net/ipv6/route.c:3427 ip6_route_check_nh() error:
> we previously assumed '_dev' could be null
I don't think this can actually happen. ip6_route_check_nh only gets
called via fib6_nh_init -> ip6_validate_gw -> ip6_route_check_nh, and
ip6_validate_gw unconditionally does dev = *_dev. Which is fine,
because its only caller (fib6_nh_init) passes &dev, so that can't be
NULL (and same for idev).
> Ensure _dev and idev are checked for NULL before dereferencing in
> ip6_route_check_nh. Assign NULL explicitly when fib_nh_dev is NULL
> to prevent unintended dereferences.
That's a separate issue (if it's really possible - I haven't checked)
than the smatch report you're quoting above. And if it is, it would
deserve a Fixes tag for the commit introducing this code.
>
> Signed-off-by: Purva Yeshi <purvayeshi550@...il.com>
> ---
> net/ipv6/route.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index ef2d23a1e3d5..ad5b3098eba0 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3424,9 +3424,20 @@ static int ip6_route_check_nh(struct net *net,
> if (dev != res.nh->fib_nh_dev)
> err = -EHOSTUNREACH;
> } else {
> - *_dev = dev = res.nh->fib_nh_dev;
> - netdev_hold(dev, dev_tracker, GFP_ATOMIC);
> - *idev = in6_dev_get(dev);
> + if (res.nh->fib_nh_dev) { /* Ensure fib_nh_dev is valid */
I don't think any of these comments are particularly helpful. It's
pretty clear that you're checking for NULL/setting NULL in all those
cases.
> + dev = res.nh->fib_nh_dev;
> +
> + if (_dev) /* Only assign if _dev is not NULL */
> + *_dev = dev;
> +
> + netdev_hold(dev, dev_tracker, GFP_ATOMIC);
> + *idev = in6_dev_get(dev);
> + } else {
> + if (_dev)
> + *_dev = NULL; /* Explicitly set NULL */
> + if (idev)
> + *idev = NULL; /* Explicitly set NULL */
> + }
> }
>
> return err;
--
Sabrina
Powered by blists - more mailing lists