netdev - Re: [PATCH net-next] tun: Do SIOCGSKNS out of rtnl

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]

Message-ID: <44b2f8b6-7152-8ba6-f26d-d08ed820f980@virtuozzo.com>
Date:   Wed, 9 May 2018 12:00:05 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     Jason Wang <jasowang@...hat.com>, davem@...emloft.net,
        edumazet@...gle.com, mst@...hat.com, brouer@...hat.com,
        peterpenkov96@...il.com, sd@...asysnail.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] tun: Do SIOCGSKNS out of rtnl_lock()

Hi, Jason,

On 09.05.2018 10:18, Jason Wang wrote:
> 
> 
> On 2018年05月09日 00:21, Kirill Tkhai wrote:
>> Since net ns of tun device is assigned on the device creation,
>> and it never changes, we do not need to use any lock to get it
>> from alive tun.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@...tuozzo.com>
>> ---
>>   drivers/net/tun.c |   18 +++++++-----------
>>   1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index d3c04ab9752a..44d4f3d25350 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -2850,10 +2850,10 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>>                   unsigned long arg, int ifreq_len)
>>   {
>>       struct tun_file *tfile = file->private_data;
>> +    struct net *net = sock_net(&tfile->sk);
>>       struct tun_struct *tun;
>>       void __user* argp = (void __user*)arg;
>>       struct ifreq ifr;
>> -    struct net *net;
>>       kuid_t owner;
>>       kgid_t group;
>>       int sndbuf;
>> @@ -2877,14 +2877,18 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>>            */
>>           return put_user(IFF_TUN | IFF_TAP | TUN_FEATURES,
>>                   (unsigned int __user*)argp);
>> -    } else if (cmd == TUNSETQUEUE)
>> +    } else if (cmd == TUNSETQUEUE) {
>>           return tun_set_queue(file, &ifr);
>> +    } else if (cmd == SIOCGSKNS) {
> 
> Not for this patch, reusing socket ioctl cmd is probably not good though they were probably not intersected (see ioctl-number.txt). We probably need to introduce TUN specific ioctls for SIOCGSKNS and SIOCGIFHWADDR and warn for socket ones.

The most of socket ioctl cmds use 0x8900 type:

#define SOCK_IOC_TYPE   0x89

while tun cmd is 5400 ('T'). They should not intersect.

The only exceptions are

#define SIOCINQ         FIONREAD
#define SIOCOUTQ        TIOCOUTQ

#define TIOCOUTQ        0x5411
#define FIONREAD        0x541B

But they can't intersect even with exceptions, since tun nr starts from 200:

#define TUNSETNOCSUM  _IOW('T', 200, int) 

and 200 > 0x1b (==27).

I implemented SIOCGSKNS cmd in the same style as older socket cmds were used.
I'm not sure, we can remove existing SIOCGIFHWADDR, since they are already used.
If we add a warn, which time will we able to remove them? Some old software may
use it, and in case of the program isn't developed any more, nobody can fix this
warnings, even if he/she sees them..

Kirill

> 
>> +        if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
>> +            return -EPERM;
>> +        return open_related_ns(&net->ns, get_net_ns);
>> +    }
>>         ret = 0;
>>       rtnl_lock();
>>         tun = tun_get(tfile);
>> -    net = sock_net(&tfile->sk);
>>       if (cmd == TUNSETIFF) {
>>           ret = -EEXIST;
>>           if (tun)
>> @@ -2914,14 +2918,6 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>>           tfile->ifindex = ifindex;
>>           goto unlock;
>>       }
>> -    if (cmd == SIOCGSKNS) {
>> -        ret = -EPERM;
>> -        if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
>> -            goto unlock;
>> -
>> -        ret = open_related_ns(&net->ns, get_net_ns);
>> -        goto unlock;
>> -    }
>>         ret = -EBADFD;
>>       if (!tun)
>>
>