| Message ID | 20250326105215.23853-1-purvayeshi550@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipv6: Fix NULL dereference in ipv6_route_check_nh | expand |
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;
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.
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;
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(-)