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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 23 Nov 2021 13:09:54 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Nikolay Aleksandrov <razor@...ckwall.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        dsahern@...il.com, Nikolay Aleksandrov <nikolay@...dia.com>,
        stable@...r.kernel.org
Subject: Re: [PATCH net] net: nexthop: fix null pointer dereference when IPv6
 is not enabled

On Tue, Nov 23, 2021 at 12:27:19PM +0200, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov <nikolay@...dia.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
>