* [PATCH] net: make net namespace sysctls belong to container's owner
@ 2016-08-02 23:19 Dmitry Torokhov
2016-08-08 21:08 ` Eric W. Biederman
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Torokhov @ 2016-08-02 23:19 UTC (permalink / raw)
To: David S. Miller; +Cc: Al Viro, Eric W. Biederman, linux-kernel, netdev
If net namespace is attached to a user namespace let's make container's
root owner of sysctls affecting said network namespace instead of global
root.
This also allows us to clean up net_ctl_permissions() because we do not
need to fudge permissions anymore for the container's owner since it now
owns the objects in question.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
This helps when running Android CTS in a container, but I think it makes
sense regardless.
fs/proc/proc_sysctl.c | 5 +++++
include/linux/sysctl.h | 4 ++++
net/sysctl_net.c | 27 ++++++++++++++++++---------
3 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5e57c3e..28f9085 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -430,6 +430,7 @@ static int sysctl_perm(struct ctl_table_header *head, struct ctl_table *table, i
static struct inode *proc_sys_make_inode(struct super_block *sb,
struct ctl_table_header *head, struct ctl_table *table)
{
+ struct ctl_table_root *root = head->root;
struct inode *inode;
struct proc_inode *ei;
@@ -457,6 +458,10 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
if (is_empty_dir(head))
make_empty_dir_inode(inode);
}
+
+ if (root->set_ownership)
+ root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
+
out:
return inode;
}
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index fa7bc29..55bec2f 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -25,6 +25,7 @@
#include <linux/rcupdate.h>
#include <linux/wait.h>
#include <linux/rbtree.h>
+#include <linux/uidgid.h>
#include <uapi/linux/sysctl.h>
/* For the /proc/sys support */
@@ -156,6 +157,9 @@ struct ctl_table_root {
struct ctl_table_set default_set;
struct ctl_table_set *(*lookup)(struct ctl_table_root *root,
struct nsproxy *namespaces);
+ void (*set_ownership)(struct ctl_table_header *head,
+ struct ctl_table *table,
+ kuid_t *uid, kgid_t *gid);
int (*permissions)(struct ctl_table_header *head, struct ctl_table *table);
};
diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index ed98c1f..ff68326 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -42,26 +42,35 @@ static int net_ctl_permissions(struct ctl_table_header *head,
struct ctl_table *table)
{
struct net *net = container_of(head->set, struct net, sysctls);
- kuid_t root_uid = make_kuid(net->user_ns, 0);
- kgid_t root_gid = make_kgid(net->user_ns, 0);
/* Allow network administrator to have same access as root. */
- if (ns_capable(net->user_ns, CAP_NET_ADMIN) ||
- uid_eq(root_uid, current_euid())) {
+ if (ns_capable(net->user_ns, CAP_NET_ADMIN)) {
int mode = (table->mode >> 6) & 7;
return (mode << 6) | (mode << 3) | mode;
}
- /* Allow netns root group to have the same access as the root group */
- if (in_egroup_p(root_gid)) {
- int mode = (table->mode >> 3) & 7;
- return (mode << 3) | mode;
- }
+
return table->mode;
}
+static void net_ctl_set_ownership(struct ctl_table_header *head,
+ struct ctl_table *table,
+ kuid_t *uid, kgid_t *gid)
+{
+ struct net *net = container_of(head->set, struct net, sysctls);
+
+ *uid = make_kuid(net->user_ns, 0);
+ if (!uid_valid(*uid))
+ *uid = GLOBAL_ROOT_UID;
+
+ *gid = make_kgid(net->user_ns, 0);
+ if (!gid_valid(*gid))
+ *gid = GLOBAL_ROOT_GID;
+}
+
static struct ctl_table_root net_sysctl_root = {
.lookup = net_ctl_header_lookup,
.permissions = net_ctl_permissions,
+ .set_ownership = net_ctl_set_ownership,
};
static int __net_init sysctl_net_init(struct net *net)
--
2.8.0.rc3.226.g39d4020
--
Dmitry
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] net: make net namespace sysctls belong to container's owner
2016-08-02 23:19 [PATCH] net: make net namespace sysctls belong to container's owner Dmitry Torokhov
@ 2016-08-08 21:08 ` Eric W. Biederman
2016-08-08 21:54 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Eric W. Biederman @ 2016-08-08 21:08 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: David S. Miller, Al Viro, linux-kernel, netdev
Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> If net namespace is attached to a user namespace let's make container's
> root owner of sysctls affecting said network namespace instead of global
> root.
>
> This also allows us to clean up net_ctl_permissions() because we do not
> need to fudge permissions anymore for the container's owner since it now
> owns the objects in question.
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Overall this seems reasonable. However I am not a fan of your error
handling.
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> This helps when running Android CTS in a container, but I think it makes
> sense regardless.
> +static void net_ctl_set_ownership(struct ctl_table_header *head,
> + struct ctl_table *table,
> + kuid_t *uid, kgid_t *gid)
> +{
> + struct net *net = container_of(head->set, struct net, sysctls);
> +
> + *uid = make_kuid(net->user_ns, 0);
> + if (!uid_valid(*uid))
> + *uid = GLOBAL_ROOT_UID;
> +
> + *gid = make_kgid(net->user_ns, 0);
> + if (!gid_valid(*gid))
> + *gid = GLOBAL_ROOT_GID;
This code should eiter be:
*uid = make_kuid(net->user_ns, 0);
*gid = make_kgid(net->user_ns, 0);
Or it should be:
tmp_uid = make_kuid(net->user_ns, 0);
if (uid_valid(tmp_uid))
*uid = tmp_uid;
tmp_gid = make_kgid(net->user_ns, 0);
if (gid_valid(tmp_gid))
*gid = tmp_gid;
It is just very fragile to assume to know what uid and gid
would be if this code fails.
As of v4.8-rc1 INVALID_UID and INVALID_GID can be set in inode->i_uid
and inode->i_gid without causing horrible vfs confusion (making the
first option viable), but I expect with the mention of Android you want
to backport this so I will ask that you ask to implement the error
handling that doesn't assume you know better than the generic code.
If you don't have a better value to set something to it really should be
left alone.
Eric
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net: make net namespace sysctls belong to container's owner
2016-08-08 21:54 ` Dmitry Torokhov
@ 2016-08-08 21:48 ` Eric W. Biederman
0 siblings, 0 replies; 4+ messages in thread
From: Eric W. Biederman @ 2016-08-08 21:48 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: David S. Miller, Al Viro, lkml, netdev
Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> On Mon, Aug 8, 2016 at 2:08 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>>
>>> If net namespace is attached to a user namespace let's make container's
>>> root owner of sysctls affecting said network namespace instead of global
>>> root.
>>>
>>> This also allows us to clean up net_ctl_permissions() because we do not
>>> need to fudge permissions anymore for the container's owner since it now
>>> owns the objects in question.
>>
>> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>
>> Overall this seems reasonable. However I am not a fan of your error
>> handling.
>>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> ---
>>>
>>> This helps when running Android CTS in a container, but I think it makes
>>> sense regardless.
>>
>>> +static void net_ctl_set_ownership(struct ctl_table_header *head,
>>> + struct ctl_table *table,
>>> + kuid_t *uid, kgid_t *gid)
>>> +{
>>> + struct net *net = container_of(head->set, struct net, sysctls);
>>> +
>>> + *uid = make_kuid(net->user_ns, 0);
>>> + if (!uid_valid(*uid))
>>> + *uid = GLOBAL_ROOT_UID;
>>> +
>>> + *gid = make_kgid(net->user_ns, 0);
>>> + if (!gid_valid(*gid))
>>> + *gid = GLOBAL_ROOT_GID;
>>
>> This code should eiter be:
>> *uid = make_kuid(net->user_ns, 0);
>> *gid = make_kgid(net->user_ns, 0);
>>
>> Or it should be:
>> tmp_uid = make_kuid(net->user_ns, 0);
>> if (uid_valid(tmp_uid))
>> *uid = tmp_uid;
>>
>> tmp_gid = make_kgid(net->user_ns, 0);
>> if (gid_valid(tmp_gid))
>> *gid = tmp_gid;
>>
>> It is just very fragile to assume to know what uid and gid
>> would be if this code fails.
>>
>> As of v4.8-rc1 INVALID_UID and INVALID_GID can be set in inode->i_uid
>> and inode->i_gid without causing horrible vfs confusion (making the
>> first option viable), but I expect with the mention of Android you want
>> to backport this so I will ask that you ask to implement the error
>> handling that doesn't assume you know better than the generic code.
>>
>> If you don't have a better value to set something to it really should be
>> left alone.
>
> OK, fair enough. I will adopt the 2nd option and will resubmit. I need
> to also test without net namespaces support (my other change blows up
> because we are getting half-initialized init_net structure when
> namespaces are disabled).
No rush. I will be out on vacation for the next couple of weeks.
Eric
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net: make net namespace sysctls belong to container's owner
2016-08-08 21:08 ` Eric W. Biederman
@ 2016-08-08 21:54 ` Dmitry Torokhov
2016-08-08 21:48 ` Eric W. Biederman
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Torokhov @ 2016-08-08 21:54 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: David S. Miller, Al Viro, lkml, netdev
On Mon, Aug 8, 2016 at 2:08 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>
>> If net namespace is attached to a user namespace let's make container's
>> root owner of sysctls affecting said network namespace instead of global
>> root.
>>
>> This also allows us to clean up net_ctl_permissions() because we do not
>> need to fudge permissions anymore for the container's owner since it now
>> owns the objects in question.
>
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Overall this seems reasonable. However I am not a fan of your error
> handling.
>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>
>> This helps when running Android CTS in a container, but I think it makes
>> sense regardless.
>
>> +static void net_ctl_set_ownership(struct ctl_table_header *head,
>> + struct ctl_table *table,
>> + kuid_t *uid, kgid_t *gid)
>> +{
>> + struct net *net = container_of(head->set, struct net, sysctls);
>> +
>> + *uid = make_kuid(net->user_ns, 0);
>> + if (!uid_valid(*uid))
>> + *uid = GLOBAL_ROOT_UID;
>> +
>> + *gid = make_kgid(net->user_ns, 0);
>> + if (!gid_valid(*gid))
>> + *gid = GLOBAL_ROOT_GID;
>
> This code should eiter be:
> *uid = make_kuid(net->user_ns, 0);
> *gid = make_kgid(net->user_ns, 0);
>
> Or it should be:
> tmp_uid = make_kuid(net->user_ns, 0);
> if (uid_valid(tmp_uid))
> *uid = tmp_uid;
>
> tmp_gid = make_kgid(net->user_ns, 0);
> if (gid_valid(tmp_gid))
> *gid = tmp_gid;
>
> It is just very fragile to assume to know what uid and gid
> would be if this code fails.
>
> As of v4.8-rc1 INVALID_UID and INVALID_GID can be set in inode->i_uid
> and inode->i_gid without causing horrible vfs confusion (making the
> first option viable), but I expect with the mention of Android you want
> to backport this so I will ask that you ask to implement the error
> handling that doesn't assume you know better than the generic code.
>
> If you don't have a better value to set something to it really should be
> left alone.
OK, fair enough. I will adopt the 2nd option and will resubmit. I need
to also test without net namespaces support (my other change blows up
because we are getting half-initialized init_net structure when
namespaces are disabled).
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-08-08 22:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-02 23:19 [PATCH] net: make net namespace sysctls belong to container's owner Dmitry Torokhov
2016-08-08 21:08 ` Eric W. Biederman
2016-08-08 21:54 ` Dmitry Torokhov
2016-08-08 21:48 ` Eric W. Biederman
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).