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

[Stefan Wiehler <stefan.wiehler@nokia.com>]

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

Thanks, that was an accident, it should have stayed where it is.

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

In the remaining two return paths we need to release the RCU lock before
calling copy_to_user(), so unfortunately we cannot use a common out label.

Kind regards,

Stefan