Re: [PATCH net v3 3/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()

[Simon Horman <horms@kernel.org>]

On Thu, Oct 10, 2024 at 11:07:44AM +0200, Stefan Wiehler wrote:
> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
> must be done under RCU or RTNL lock. Copy from user space must be
> performed beforehand as we are not allowed to sleep under RCU lock.
> 
> Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
> ---
> v3:
>   - split into separate patches
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
>   - rebase on top of net tree
>   - add Fixes tag
>   - refactor out paths
> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240605195355.363936-1-oss@malat.biz/
> ---
>  net/ipv6/ip6mr.c | 46 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index b18eb4ad21e4..415ba6f55a44 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -1961,10 +1961,7 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>  	struct mfc6_cache *c;
>  	struct net *net = sock_net(sk);
>  	struct mr_table *mrt;
> -
> -	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
> -	if (!mrt)
> -		return -ENOENT;
> +	int err;
>  
>  	switch (cmd) {
>  	case SIOCGETMIFCNT_IN6:
> @@ -1972,8 +1969,30 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>  			return -EFAULT;
>  		if (vr.mifi >= mrt->maxvif)
>  			return -EINVAL;

Hi Stefan,

mrt is now used uninitialised here.

> +		break;
> +	case SIOCGETSGCNT_IN6:
> +		if (copy_from_user(&sr, arg, sizeof(sr)))
> +			return -EFAULT;
> +		break;
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +
> +
> +	rcu_read_lock();
> +	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
> +	if (!mrt) {
> +		err = -ENOENT;
> +		goto out;
> +	}
> +
> +	switch (cmd) {
> +	case SIOCGETMIFCNT_IN6:
> +		if (vr.mifi >= mrt->maxvif) {
> +			err = -EINVAL;
> +			goto out;
> +		}
>  		vr.mifi = array_index_nospec(vr.mifi, mrt->maxvif);
> -		rcu_read_lock();
>  		vif = &mrt->vif_table[vr.mifi];
>  		if (VIF_EXISTS(mrt, vr.mifi)) {
>  			vr.icount = READ_ONCE(vif->pkt_in);

...

> @@ -2004,11 +2020,13 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>  				return -EFAULT;
>  			return 0;
>  		}
> -		rcu_read_unlock();
> -		return -EADDRNOTAVAIL;
> -	default:
> -		return -ENOIOCTLCMD;
> +		err = -EADDRNOTAVAIL;
> +		goto out;
>  	}
> +

I think that this out label should be used consistently once rcu_read_lock
has been taken. With this patch applied there seems to be one case on error
where rcu_read_unlock() before returning, and one case where it isn't
(which looks like it leaks the lock).

> +out:
> +	rcu_read_unlock();
> +	return err;
>  }
>  #endif

-- 
pw-bot: changes-requested