Re: [PATCH net] net: nexthop: fix null pointer dereference when IPv6 is not enabled

[Ido Schimmel <idosch@idosch.org>]

On Tue, Nov 23, 2021 at 12:27:19PM +0200, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov <nikolay@nvidia.com>
> 
> When we try to add an IPv6 nexthop and IPv6 is not enabled
> (!CONFIG_IPV6) we'll hit a NULL pointer dereference[1] in the error path
> of nh_create_ipv6() due to calling ipv6_stub->fib6_nh_release. The bug
> has been present since the beginning of IPv6 nexthop gateway support.
> Commit 1aefd3de7bc6 ("ipv6: Add fib6_nh_init and release to stubs") tells
> us that only fib6_nh_init has a dummy stub because fib6_nh_release should
> not be called if fib6_nh_init returns an error, but the commit below added
> a call to ipv6_stub->fib6_nh_release in its error path. To fix it return
> the dummy stub's -EAFNOSUPPORT error directly without calling
> ipv6_stub->fib6_nh_release in nh_create_ipv6()'s error path.

[...]

> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index a69a9e76f99f..5dbd4b5505eb 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -2565,11 +2565,15 @@ static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
>  	/* sets nh_dev if successful */
>  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>  				      extack);
> -	if (err)
> +	if (err) {
> +		/* IPv6 is not enabled, don't call fib6_nh_release */
> +		if (err == -EAFNOSUPPORT)
> +			goto out;
>  		ipv6_stub->fib6_nh_release(fib6_nh);

Is the call actually necessary? If fib6_nh_init() failed, then I believe
it should clean up after itself and not rely on fib6_nh_release().

> -	else
> +	} else {
>  		nh->nh_flags = fib6_nh->fib_nh_flags;
> -
> +	}
> +out:
>  	return err;
>  }
>  
> -- 
> 2.31.1
>