* [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms
@ 2013-06-12 4:44 Gao feng
2013-06-12 4:44 ` [PATCH RESEND 2/2] neigh: disallow un-init_net to change thresh of neigh Gao feng
2013-06-12 7:33 ` [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms Eric W. Biederman
0 siblings, 2 replies; 6+ messages in thread
From: Gao feng @ 2013-06-12 4:44 UTC (permalink / raw)
To: davem; +Cc: ebiederm, netdev, Gao feng
Though we don't export the /proc/sys/net/ipv[4,6]/neigh/default/
directory to the un-init_net, but we can still use cmd such as
"ip ntable change name arp_cache locktime 129" to change the locktime
of default neigh_parms.
This patch disallows the un-init_net to find out the neigh_table.parms.
So the un-init_net will failed to influence the init_net.
Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
net/core/neighbour.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 5c56b21..e4027ff 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1418,26 +1418,29 @@ static inline struct neigh_parms *lookup_neigh_parms(struct neigh_table *tbl,
struct neigh_parms *p;
for (p = &tbl->parms; p; p = p->next) {
- if ((p->dev && p->dev->ifindex == ifindex && net_eq(neigh_parms_net(p), net)) ||
- (!p->dev && !ifindex))
+ if (p->dev && p->dev->ifindex == ifindex &&
+ net_eq(neigh_parms_net(p), net))
return p;
+
+ if (!p->dev && !ifindex) {
+ if (net_eq(net, &init_net))
+ return p;
+ else
+ return ERR_PTR(-EPERM);
+ }
}
- return NULL;
+ return ERR_PTR(-ENOENT);
}
struct neigh_parms *neigh_parms_alloc(struct net_device *dev,
struct neigh_table *tbl)
{
- struct neigh_parms *p, *ref;
+ struct neigh_parms *p;
struct net *net = dev_net(dev);
const struct net_device_ops *ops = dev->netdev_ops;
- ref = lookup_neigh_parms(tbl, net, 0);
- if (!ref)
- return NULL;
-
- p = kmemdup(ref, sizeof(*p), GFP_KERNEL);
+ p = kmemdup(&tbl->parms, sizeof(*p), GFP_KERNEL);
if (p) {
p->tbl = tbl;
atomic_set(&p->refcnt, 1);
@@ -1999,8 +2002,8 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh)
ifindex = nla_get_u32(tbp[NDTPA_IFINDEX]);
p = lookup_neigh_parms(tbl, net, ifindex);
- if (p == NULL) {
- err = -ENOENT;
+ if (IS_ERR(p)) {
+ err = PTR_ERR(p);
goto errout_tbl_lock;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH RESEND 2/2] neigh: disallow un-init_net to change thresh of neigh
2013-06-12 4:44 [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms Gao feng
@ 2013-06-12 4:44 ` Gao feng
2013-06-12 7:33 ` [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms Eric W. Biederman
1 sibling, 0 replies; 6+ messages in thread
From: Gao feng @ 2013-06-12 4:44 UTC (permalink / raw)
To: davem; +Cc: ebiederm, netdev, Gao feng
thresh and interval are global resources,
only init net can change them.
Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
net/core/neighbour.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index e4027ff..8feb382 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2056,6 +2056,12 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh)
}
}
+ err = -EPERM;
+ if ((tb[NDTA_THRESH1] || tb[NDTA_THRESH2] ||
+ tb[NDTA_THRESH3] || tb[NDTA_GC_INTERVAL]) &&
+ !net_eq(net, &init_net))
+ goto errout_tbl_lock;
+
if (tb[NDTA_THRESH1])
tbl->gc_thresh1 = nla_get_u32(tb[NDTA_THRESH1]);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms
2013-06-12 4:44 [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms Gao feng
2013-06-12 4:44 ` [PATCH RESEND 2/2] neigh: disallow un-init_net to change thresh of neigh Gao feng
@ 2013-06-12 7:33 ` Eric W. Biederman
2013-06-13 1:17 ` Gao feng
1 sibling, 1 reply; 6+ messages in thread
From: Eric W. Biederman @ 2013-06-12 7:33 UTC (permalink / raw)
To: Gao feng; +Cc: davem, netdev
Gao feng <gaofeng@cn.fujitsu.com> writes:
> Though we don't export the /proc/sys/net/ipv[4,6]/neigh/default/
> directory to the un-init_net, but we can still use cmd such as
> "ip ntable change name arp_cache locktime 129" to change the locktime
> of default neigh_parms.
>
> This patch disallows the un-init_net to find out the neigh_table.parms.
> So the un-init_net will failed to influence the init_net.
Interesting...
The problem these two patches seek to address seems legit.
However I disagree with the way you are handling this.
Outside of the initial network namespace we should return -ENOENT
instead of -EPERM. Which would match how we handle sysctls, and I think
missing neigh table values. Just not making these global values visible
seems wise.
The alternative is to use the proper permission test which is
capable(CAP_SYS_ADMIN) (instead of testing network namespaces) and
return -EPERM if that fails. Which would allow processes in other
network namespaces to change the value if they could otherwise change
the value.
Namespaces are about visibility and there is no need to make an
exception here.
Eric
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> ---
> net/core/neighbour.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 5c56b21..e4027ff 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1418,26 +1418,29 @@ static inline struct neigh_parms *lookup_neigh_parms(struct neigh_table *tbl,
> struct neigh_parms *p;
>
> for (p = &tbl->parms; p; p = p->next) {
> - if ((p->dev && p->dev->ifindex == ifindex && net_eq(neigh_parms_net(p), net)) ||
> - (!p->dev && !ifindex))
> + if (p->dev && p->dev->ifindex == ifindex &&
> + net_eq(neigh_parms_net(p), net))
> return p;
> +
> + if (!p->dev && !ifindex) {
> + if (net_eq(net, &init_net))
> + return p;
> + else
> + return ERR_PTR(-EPERM);
> + }
> }
>
> - return NULL;
> + return ERR_PTR(-ENOENT);
> }
>
> struct neigh_parms *neigh_parms_alloc(struct net_device *dev,
> struct neigh_table *tbl)
> {
> - struct neigh_parms *p, *ref;
> + struct neigh_parms *p;
> struct net *net = dev_net(dev);
> const struct net_device_ops *ops = dev->netdev_ops;
>
> - ref = lookup_neigh_parms(tbl, net, 0);
> - if (!ref)
> - return NULL;
> -
> - p = kmemdup(ref, sizeof(*p), GFP_KERNEL);
> + p = kmemdup(&tbl->parms, sizeof(*p), GFP_KERNEL);
> if (p) {
> p->tbl = tbl;
> atomic_set(&p->refcnt, 1);
> @@ -1999,8 +2002,8 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh)
> ifindex = nla_get_u32(tbp[NDTPA_IFINDEX]);
>
> p = lookup_neigh_parms(tbl, net, ifindex);
> - if (p == NULL) {
> - err = -ENOENT;
> + if (IS_ERR(p)) {
> + err = PTR_ERR(p);
> goto errout_tbl_lock;
> }
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms
2013-06-12 7:33 ` [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms Eric W. Biederman
@ 2013-06-13 1:17 ` Gao feng
2013-06-13 1:27 ` Eric W. Biederman
0 siblings, 1 reply; 6+ messages in thread
From: Gao feng @ 2013-06-13 1:17 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: davem, netdev
On 06/12/2013 03:33 PM, Eric W. Biederman wrote:
> Gao feng <gaofeng@cn.fujitsu.com> writes:
>
>> Though we don't export the /proc/sys/net/ipv[4,6]/neigh/default/
>> directory to the un-init_net, but we can still use cmd such as
>> "ip ntable change name arp_cache locktime 129" to change the locktime
>> of default neigh_parms.
>>
>> This patch disallows the un-init_net to find out the neigh_table.parms.
>> So the un-init_net will failed to influence the init_net.
>
> Interesting...
>
> The problem these two patches seek to address seems legit.
>
> However I disagree with the way you are handling this.
>
> Outside of the initial network namespace we should return -ENOENT
> instead of -EPERM. Which would match how we handle sysctls, and I think
> missing neigh table values. Just not making these global values visible
> seems wise.
>
Ok, it seems more reasonable.
> The alternative is to use the proper permission test which is
> capable(CAP_SYS_ADMIN) (instead of testing network namespaces) and
> return -EPERM if that fails. Which would allow processes in other
> network namespaces to change the value if they could otherwise change
> the value.
>
So you mean the uninitial net namespace can't see these values but it
can change them? it's too strange.
And the thresh/interval are both under default/ too, if we return -ENOENT
for other items, we should also return -ENOENT for them instead of the -EPERM.
> Namespaces are about visibility and there is no need to make an
> exception here.
>
> Eric
>
>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>> ---
>> net/core/neighbour.c | 25 ++++++++++++++-----------
>> 1 file changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>> index 5c56b21..e4027ff 100644
>> --- a/net/core/neighbour.c
>> +++ b/net/core/neighbour.c
>> @@ -1418,26 +1418,29 @@ static inline struct neigh_parms *lookup_neigh_parms(struct neigh_table *tbl,
>> struct neigh_parms *p;
>>
>> for (p = &tbl->parms; p; p = p->next) {
>> - if ((p->dev && p->dev->ifindex == ifindex && net_eq(neigh_parms_net(p), net)) ||
>> - (!p->dev && !ifindex))
>> + if (p->dev && p->dev->ifindex == ifindex &&
>> + net_eq(neigh_parms_net(p), net))
>> return p;
>> +
>> + if (!p->dev && !ifindex) {
>> + if (net_eq(net, &init_net))
>> + return p;
>> + else
>> + return ERR_PTR(-EPERM);
>> + }
>> }
>>
>> - return NULL;
>> + return ERR_PTR(-ENOENT);
>> }
>>
>> struct neigh_parms *neigh_parms_alloc(struct net_device *dev,
>> struct neigh_table *tbl)
>> {
>> - struct neigh_parms *p, *ref;
>> + struct neigh_parms *p;
>> struct net *net = dev_net(dev);
>> const struct net_device_ops *ops = dev->netdev_ops;
>>
>> - ref = lookup_neigh_parms(tbl, net, 0);
>> - if (!ref)
>> - return NULL;
>> -
>> - p = kmemdup(ref, sizeof(*p), GFP_KERNEL);
>> + p = kmemdup(&tbl->parms, sizeof(*p), GFP_KERNEL);
>> if (p) {
>> p->tbl = tbl;
>> atomic_set(&p->refcnt, 1);
>> @@ -1999,8 +2002,8 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh)
>> ifindex = nla_get_u32(tbp[NDTPA_IFINDEX]);
>>
>> p = lookup_neigh_parms(tbl, net, ifindex);
>> - if (p == NULL) {
>> - err = -ENOENT;
>> + if (IS_ERR(p)) {
>> + err = PTR_ERR(p);
>> goto errout_tbl_lock;
>> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms
2013-06-13 1:17 ` Gao feng
@ 2013-06-13 1:27 ` Eric W. Biederman
2013-06-13 3:44 ` Gao feng
0 siblings, 1 reply; 6+ messages in thread
From: Eric W. Biederman @ 2013-06-13 1:27 UTC (permalink / raw)
To: Gao feng; +Cc: davem, netdev
Gao feng <gaofeng@cn.fujitsu.com> writes:
> On 06/12/2013 03:33 PM, Eric W. Biederman wrote:
>> Gao feng <gaofeng@cn.fujitsu.com> writes:
>>
>>> Though we don't export the /proc/sys/net/ipv[4,6]/neigh/default/
>>> directory to the un-init_net, but we can still use cmd such as
>>> "ip ntable change name arp_cache locktime 129" to change the locktime
>>> of default neigh_parms.
>>>
>>> This patch disallows the un-init_net to find out the neigh_table.parms.
>>> So the un-init_net will failed to influence the init_net.
>>
>> Interesting...
>>
>> The problem these two patches seek to address seems legit.
>>
>> However I disagree with the way you are handling this.
>>
>> Outside of the initial network namespace we should return -ENOENT
>> instead of -EPERM. Which would match how we handle sysctls, and I think
>> missing neigh table values. Just not making these global values visible
>> seems wise.
>>
>
> Ok, it seems more reasonable.
>
>> The alternative is to use the proper permission test which is
>> capable(CAP_SYS_ADMIN) (instead of testing network namespaces) and
>> return -EPERM if that fails. Which would allow processes in other
>> network namespaces to change the value if they could otherwise change
>> the value.
>>
>
> So you mean the uninitial net namespace can't see these values but it
> can change them? it's too strange.
Sorry I was saying that if you don't want to hide the values the
permissions and (-EPERM) should track the user namespace not the network
namespace.
> And the thresh/interval are both under default/ too, if we return -ENOENT
> for other items, we should also return -ENOENT for them instead of the
> -EPERM.
Yes. Let's return hide the global values and just return -ENOENT for
everything. That seems simplest.
Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms
2013-06-13 1:27 ` Eric W. Biederman
@ 2013-06-13 3:44 ` Gao feng
0 siblings, 0 replies; 6+ messages in thread
From: Gao feng @ 2013-06-13 3:44 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: davem, netdev
On 06/13/2013 09:27 AM, Eric W. Biederman wrote:
> Gao feng <gaofeng@cn.fujitsu.com> writes:
>
>> On 06/12/2013 03:33 PM, Eric W. Biederman wrote:
>>> Gao feng <gaofeng@cn.fujitsu.com> writes:
>>>
>>>> Though we don't export the /proc/sys/net/ipv[4,6]/neigh/default/
>>>> directory to the un-init_net, but we can still use cmd such as
>>>> "ip ntable change name arp_cache locktime 129" to change the locktime
>>>> of default neigh_parms.
>>>>
>>>> This patch disallows the un-init_net to find out the neigh_table.parms.
>>>> So the un-init_net will failed to influence the init_net.
>>>
>>> Interesting...
>>>
>>> The problem these two patches seek to address seems legit.
>>>
>>> However I disagree with the way you are handling this.
>>>
>>> Outside of the initial network namespace we should return -ENOENT
>>> instead of -EPERM. Which would match how we handle sysctls, and I think
>>> missing neigh table values. Just not making these global values visible
>>> seems wise.
>>>
>>
>> Ok, it seems more reasonable.
>>
>>> The alternative is to use the proper permission test which is
>>> capable(CAP_SYS_ADMIN) (instead of testing network namespaces) and
>>> return -EPERM if that fails. Which would allow processes in other
>>> network namespaces to change the value if they could otherwise change
>>> the value.
>>>
>>
>> So you mean the uninitial net namespace can't see these values but it
>> can change them? it's too strange.
>
> Sorry I was saying that if you don't want to hide the values the
> permissions and (-EPERM) should track the user namespace not the network
> namespace.
>
>> And the thresh/interval are both under default/ too, if we return -ENOENT
>> for other items, we should also return -ENOENT for them instead of the
>> -EPERM.
>
> Yes. Let's return hide the global values and just return -ENOENT for
> everything. That seems simplest.
>
Get it, thanks.
BTW, do you think we need to prevent the default parms being leaked to container?
we can use cmd "ip ntable show name arp_cache" get the thresh and so on in container
now.
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-13 3:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-12 4:44 [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms Gao feng
2013-06-12 4:44 ` [PATCH RESEND 2/2] neigh: disallow un-init_net to change thresh of neigh Gao feng
2013-06-12 7:33 ` [PATCH RESEND 1/2] neigh: only allow init_net to change the default neigh_parms Eric W. Biederman
2013-06-13 1:17 ` Gao feng
2013-06-13 1:27 ` Eric W. Biederman
2013-06-13 3:44 ` Gao feng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).