[PATCH net-next 1/4]netns: fdb: allow unprivileged users to add/del fdb entries

[PATCH net-next 1/4]netns: fdb: allow unprivileged users to add/del fdb entries

[Gao feng]

Right now,only ixgdb,macvlan,vxlan and bridge implement
fdb_add/fdb_del operations.

these operations only operate the private data of net
device. So allowing the unprivileged users who creates
the userns and netns to add/del fdb entries will do no
harm to other netns.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 net/core/rtnetlink.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 9a419b0..c1e4db6 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2058,9 +2058,6 @@ static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
 	u8 *addr;
 	int err;
 
-	if (!capable(CAP_NET_ADMIN))
-		return -EPERM;
-
 	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL);
 	if (err < 0)
 		return err;
@@ -2127,9 +2124,6 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
 	int err = -EINVAL;
 	__u8 *addr;
 
-	if (!capable(CAP_NET_ADMIN))
-		return -EPERM;
-
 	if (nlmsg_len(nlh) < sizeof(*ndm))
 		return -EINVAL;
 
-- 
1.7.11.7

[PATCH net-next 2/4] netns: ebtable: allow unprivileged users to operate ebtables

[Gao feng]

ebt_table is a private resource of netns, operating ebtables
in one netns will not affect other netns, we can allow the
creator user of userns and netns to change the ebtables.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 net/bridge/netfilter/ebtables.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 5fe2ff3..8d493c9 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1472,16 +1472,17 @@ static int do_ebt_set_ctl(struct sock *sk,
 	int cmd, void __user *user, unsigned int len)
 {
 	int ret;
+	struct net *net = sock_net(sk);
 
-	if (!capable(CAP_NET_ADMIN))
+	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 
 	switch(cmd) {
 	case EBT_SO_SET_ENTRIES:
-		ret = do_replace(sock_net(sk), user, len);
+		ret = do_replace(net, user, len);
 		break;
 	case EBT_SO_SET_COUNTERS:
-		ret = update_counters(sock_net(sk), user, len);
+		ret = update_counters(net, user, len);
 		break;
 	default:
 		ret = -EINVAL;
@@ -1494,14 +1495,15 @@ static int do_ebt_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
 	int ret;
 	struct ebt_replace tmp;
 	struct ebt_table *t;
+	struct net *net = sock_net(sk);
 
-	if (!capable(CAP_NET_ADMIN))
+	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 
 	if (copy_from_user(&tmp, user, sizeof(tmp)))
 		return -EFAULT;
 
-	t = find_table_lock(sock_net(sk), tmp.name, &ret, &ebt_mutex);
+	t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
 	if (!t)
 		return ret;
 
@@ -2279,16 +2281,17 @@ static int compat_do_ebt_set_ctl(struct sock *sk,
 		int cmd, void __user *user, unsigned int len)
 {
 	int ret;
+	struct net *net = sock_net(sk);
 
-	if (!capable(CAP_NET_ADMIN))
+	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 
 	switch (cmd) {
 	case EBT_SO_SET_ENTRIES:
-		ret = compat_do_replace(sock_net(sk), user, len);
+		ret = compat_do_replace(net, user, len);
 		break;
 	case EBT_SO_SET_COUNTERS:
-		ret = compat_update_counters(sock_net(sk), user, len);
+		ret = compat_update_counters(net, user, len);
 		break;
 	default:
 		ret = -EINVAL;
@@ -2302,8 +2305,9 @@ static int compat_do_ebt_get_ctl(struct sock *sk, int cmd,
 	int ret;
 	struct compat_ebt_replace tmp;
 	struct ebt_table *t;
+	struct net *net = sock_net(sk);
 
-	if (!capable(CAP_NET_ADMIN))
+	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 
 	/* try real handler in case userland supplied needed padding */
@@ -2314,7 +2318,7 @@ static int compat_do_ebt_get_ctl(struct sock *sk, int cmd,
 	if (copy_from_user(&tmp, user, sizeof(tmp)))
 		return -EFAULT;
 
-	t = find_table_lock(sock_net(sk), tmp.name, &ret, &ebt_mutex);
+	t = find_table_lock(net, tmp.name, &ret, &ebt_mutex);
 	if (!t)
 		return ret;
 
-- 
1.7.11.7

[PATCH net-next 3/4] netns: bridge: allow unprivileged users add/delete mdb entry

[Gao feng]

since the mdb table is belong to bridge device,and the
bridge device can only be seen in one netns.
So it's safe to allow unprivileged user which is the
creator of userns and netns to modify the mdb table.

Signed-off-by: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 net/bridge/br_mdb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index acc9f4c..38991e0 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -272,9 +272,6 @@ static int br_mdb_parse(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net_device *dev;
 	int err;
 
-	if (!capable(CAP_NET_ADMIN))
-		return -EPERM;
-
 	err = nlmsg_parse(nlh, sizeof(*bpm), tb, MDBA_SET_ENTRY, NULL);
 	if (err < 0)
 		return err;
-- 
1.7.11.7

[PATCH net-next 4/4] netns: bond: allow unprivileged users to control bond device

[Gao feng]

reduce the permission check of bond device's ioctl.
allow the userns root to control the bond device.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 drivers/net/bonding/bond_main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b38c9bf..2239937 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3612,6 +3612,7 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
 	struct ifslave k_sinfo;
 	struct ifslave __user *u_sinfo = NULL;
 	struct mii_ioctl_data *mii = NULL;
+	struct net *net;
 	int res = 0;
 
 	pr_debug("bond_ioctl: master=%s, cmd=%d\n", bond_dev->name, cmd);
@@ -3678,10 +3679,12 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
 		break;
 	}
 
-	if (!capable(CAP_NET_ADMIN))
+	net = dev_net(bond_dev);
+
+	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 
-	slave_dev = dev_get_by_name(dev_net(bond_dev), ifr->ifr_slave);
+	slave_dev = dev_get_by_name(net, ifr->ifr_slave);
 
 	pr_debug("slave_dev=%p:\n", slave_dev);
 
-- 
1.7.11.7

Re: [PATCH net-next 3/4] netns: bridge: allow unprivileged users add/delete mdb entry

[Matt Helsley]

On Fri, Feb 01, 2013 at 10:30:59AM +0800, Gao feng wrote:
> since the mdb table is belong to bridge device,and the
> bridge device can only be seen in one netns.
> So it's safe to allow unprivileged user which is the
> creator of userns and netns to modify the mdb table.
> 
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> ---
>  net/bridge/br_mdb.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index acc9f4c..38991e0 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -272,9 +272,6 @@ static int br_mdb_parse(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	struct net_device *dev;
>  	int err;
> 
> -	if (!capable(CAP_NET_ADMIN))
> -		return -EPERM;
> -

I'm wondering why this doesn't follow the:

...
- if (!capable(CAP_NET_ADMIN))
+ if (!ns_capable(net->user_ns, CAP_NET_ADMIN))

pattern like the rest of the changes you provided. Perhaps I'm
neglecting something but it looks wrong to remove the CAP_NET_ADMIN
check entirely.

Cheers,
	-Matt Helsley

Re: [PATCH net-next 3/4] netns: bridge: allow unprivileged users add/delete mdb entry

[Gao feng]

On 2013/02/01 11:46, Matt Helsley wrote:
> On Fri, Feb 01, 2013 at 10:30:59AM +0800, Gao feng wrote:
>> since the mdb table is belong to bridge device,and the
>> bridge device can only be seen in one netns.
>> So it's safe to allow unprivileged user which is the
>> creator of userns and netns to modify the mdb table.
>>
>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>> ---
>>  net/bridge/br_mdb.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
>> index acc9f4c..38991e0 100644
>> --- a/net/bridge/br_mdb.c
>> +++ b/net/bridge/br_mdb.c
>> @@ -272,9 +272,6 @@ static int br_mdb_parse(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  	struct net_device *dev;
>>  	int err;
>>
>> -	if (!capable(CAP_NET_ADMIN))
>> -		return -EPERM;
>> -
> 
> I'm wondering why this doesn't follow the:
> 
> ...
> - if (!capable(CAP_NET_ADMIN))
> + if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> 
> pattern like the rest of the changes you provided. Perhaps I'm
> neglecting something but it looks wrong to remove the CAP_NET_ADMIN
> check entirely.
> 

rtnetlink_rcv_msg has done this job,in commit dfc47ef8639facd77210e74be831943c2fdd9c74
Eric change capable to ns_capable in rtnetlink_rcv_msg and Push capable(CAP_NET_ADMIN)
into the rtnl methods.So we only need to do is remove this capable in br_mdb_parse.

Thanks!

Re: [PATCH net-next 3/4] netns: bridge: allow unprivileged users add/delete mdb entry

[Matt Helsley]

On Fri, Feb 01, 2013 at 11:59:03AM +0800, Gao feng wrote:
> On 2013/02/01 11:46, Matt Helsley wrote:
> > On Fri, Feb 01, 2013 at 10:30:59AM +0800, Gao feng wrote:
> >> since the mdb table is belong to bridge device,and the
> >> bridge device can only be seen in one netns.
> >> So it's safe to allow unprivileged user which is the
> >> creator of userns and netns to modify the mdb table.
> >>
> >> Signed-off-by: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> >> ---
> >>  net/bridge/br_mdb.c | 3 ---
> >>  1 file changed, 3 deletions(-)
> >>
> >> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> >> index acc9f4c..38991e0 100644
> >> --- a/net/bridge/br_mdb.c
> >> +++ b/net/bridge/br_mdb.c
> >> @@ -272,9 +272,6 @@ static int br_mdb_parse(struct sk_buff *skb, struct nlmsghdr *nlh,
> >>  	struct net_device *dev;
> >>  	int err;
> >>
> >> -	if (!capable(CAP_NET_ADMIN))
> >> -		return -EPERM;
> >> -
> > 
> > I'm wondering why this doesn't follow the:
> > 
> > ...
> > - if (!capable(CAP_NET_ADMIN))
> > + if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> > 
> > pattern like the rest of the changes you provided. Perhaps I'm
> > neglecting something but it looks wrong to remove the CAP_NET_ADMIN
> > check entirely.
> > 
> 
> rtnetlink_rcv_msg has done this job,in commit dfc47ef8639facd77210e74be831943c2fdd9c74
> Eric change capable to ns_capable in rtnetlink_rcv_msg and Push capable(CAP_NET_ADMIN)
> into the rtnl methods.So we only need to do is remove this capable in br_mdb_parse.
> 
> Thanks!

OK, thanks!

Cheers,
	-Matt

Re: [PATCH net-next 1/4]netns: fdb: allow unprivileged users to add/del fdb entries

[David Miller]

From: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Date: Fri, 1 Feb 2013 10:30:57 +0800

> Right now,only ixgdb,macvlan,vxlan and bridge implement
> fdb_add/fdb_del operations.
> 
> these operations only operate the private data of net
> device. So allowing the unprivileged users who creates
> the userns and netns to add/del fdb entries will do no
> harm to other netns.
> 
> Signed-off-by: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Applied.

Re: [PATCH net-next 2/4] netns: ebtable: allow unprivileged users to operate ebtables

[David Miller]

From: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Date: Fri, 1 Feb 2013 10:30:58 +0800

> ebt_table is a private resource of netns, operating ebtables
> in one netns will not affect other netns, we can allow the
> creator user of userns and netns to change the ebtables.
> 
> Signed-off-by: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Applied.

Re: [PATCH net-next 3/4] netns: bridge: allow unprivileged users add/delete mdb entry

[David Miller]

From: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Date: Fri, 1 Feb 2013 10:30:59 +0800

> since the mdb table is belong to bridge device,and the
> bridge device can only be seen in one netns.
> So it's safe to allow unprivileged user which is the
> creator of userns and netns to modify the mdb table.
> 
> Signed-off-by: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Applied.

Re: [PATCH net-next 4/4] netns: bond: allow unprivileged users to control bond device

[David Miller]

From: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Date: Fri, 1 Feb 2013 10:31:00 +0800

> reduce the permission check of bond device's ioctl.
> allow the userns root to control the bond device.
> 
> Signed-off-by: Gao feng <gaofeng-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Applied.