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 08:25:17 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	David Ahern <dsa@...ulusnetworks.com>
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 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.)

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

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

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

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.

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