Fix Smatch-detected error:
net/ipv6/route.c:3427 ip6_route_check_nh() error:
we previously assumed '_dev' could be null

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.

Signed-off-by: Purva Yeshi <purvayeshi550@gmail.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 */
+			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;
-- 
2.34.1

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@gmail.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

On 3/26/25 6:57 AM, Sabrina Dubroca wrote:
> 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).

And fib6_nh_init has:

        struct net_device *dev = NULL;
        struct inet6_dev *idev = NULL;

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

I do not believe it is a problem.