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]

Message-ID: <CALx6S35wKOHw+tMap3-6H9m7YmPrRH1=YaQGy=0OxJpWvBPapQ@mail.gmail.com>
Date:	Tue, 28 Jul 2015 10:12:26 -0700
From:	Tom Herbert <tom@...bertland.com>
To:	David Ahern <dsa@...ulusnetworks.com>
Cc:	Andy Lutomirski <luto@...capital.net>, 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 Prabhu <roopa@...ulusnetworks.com>,
	Blake Matheny <bmatheny@...com>,
	Alex Gartrell <agartrell@...com>
Subject: Re: [PATCH net-next 14/16] net: Add sk_bind_dev_if to task_struct

On Tue, Jul 28, 2015 at 9:11 AM, David Ahern <dsa@...ulusnetworks.com> wrote:
> 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.
>
We are also intending to implement similar functionality for ILA to
restrict tasks (probably from cgroup) to binding to it's assigned
addresses. This seems most easily accomplished by adding a binding
interface which is only checked at bind time. After binding, the a
connection should be processed no differently than any others,
additional plumbing in the data path for network name spaces just
seems like overhead.

Tom

>>
>>> +#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
--
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