netdev - Re: [PATCH net-next 14/16] net: Add sk_bind_dev_if to task

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, 28 Jul 2015 10:11:13 -0600
From:	David Ahern <dsa@...ulusnetworks.com>
To:	Andy Lutomirski <luto@...capital.net>
CC:	gospo@...ulusnetworks.com,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	ddutt@...ulusnetworks.com,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Ingo Molnar <mingo@...nel.org>,
	Stephen Hemminger <stephen@...workplumber.org>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>,
	jtoppins@...ulusnetworks.com,
	Network Development <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Shrijeet Mukherjee <shm@...ulusnetworks.com>,
	svaidya@...cade.com, hadi@...atatu.com,
	nikolay@...ulusnetworks.com, roopa@...ulusnetworks.com
Subject: Re: [PATCH net-next 14/16] net: Add sk_bind_dev_if to task_struct

On 7/28/15 9:25 AM, Andy Lutomirski wrote:
> On Jul 27, 2015 11:33 AM, "David Ahern" <dsa@...ulusnetworks.com> wrote:
>>
>> Allow tasks to have a default device index for binding sockets. If set
>> the value is passed to all AF_INET/AF_INET6 sockets when they are created.
>>
>
> This is not intended to be a review of the concept.  I haven't thought
> about whether the concept is a good idea, broken by design, or
> whatever.  FWIW, if this were added to the kernel and didn't require
> excessive privilege, I'd probably use it.  (I still don't really
> understand why binding to a device requires privilege in the first
> place, but, again, I haven't thought about it very much.)

The intent here is to restrict a task to only sending and receiving 
packets from a single network device. The device can be single ethernet 
interface, a stacked device (e.g, bond) or in our case a VRF device 
which restricts a task to interfaces (and hence network paths) 
associated with the VRF.

>
>> +#ifdef CONFIG_NET
>> +       case PR_SET_SK_BIND_DEV_IF:
>> +       {
>> +               struct net_device *dev;
>> +               int idx = (int) arg2;
>> +
>> +               if (!capable(CAP_NET_ADMIN))
>> +                       return -EPERM;
>> +
>
> Can you either use ns_capable or add a comment as to why not?

will do.

>
> Also, please return -EINVAL if unused args are nonzero.

ok.

>
>> +               if (idx) {
>> +                       dev = dev_get_by_index(me->nsproxy->net_ns, idx);
>> +                       if (!dev)
>> +                               return -EINVAL;
>> +                       dev_put(dev);
>> +               }
>> +               me->sk_bind_dev_if = idx;
>> +               break;
>> +       }
>> +       case PR_GET_SK_BIND_DEV_IF:
>> +       {
>> +               struct task_struct *tsk;
>> +               int sk_bind_dev_if = -EINVAL;
>> +
>> +               rcu_read_lock();
>> +               tsk = find_task_by_vpid(arg2);
>> +               if (tsk)
>> +                       sk_bind_dev_if = tsk->sk_bind_dev_if;
>
> Why do you support different tasks here?  Could this use proc instead?

In this case we want to allow a separate process to determine if a task 
is restricted to a device.

>
> The same -EINVAL issue applies.
>
> Also, I think you need to hook setns and unshare to do something
> reasonable when the task is bound to a device.

ack on both.

Thanks for the review,
David
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html