| Message ID | 20240315-sysctl-net-ownership-v1-1-2b465555a292@weissschuh.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: always initialize sysctl ownership | expand |
From: Thomas Weißschuh <linux@weissschuh.net> Date: Fri, 15 Mar 2024 17:20:31 +0100 > The sysctl core does not initialize these fields when the set_ownership > callback is present. > So always do it in the callback. Could you add few more words here to explain what the problem is like commit 5ec27ec735ba ("fs/proc/proc_sysctl.c: fix the default values of i_uid/i_gid on /proc/sys inodes."). BTW, it seems that we can change the value even if the uid/gid is invalid unlike the issue mentioned in 5ec27ec735ba. So, the problem is just the invalid uid/gid exposed to userspace or am I missing something ? ---8<--- [root@localhost ~]# unshare --user --keep-caps [nobody@localhost ~]$ id uid=65534(nobody) gid=65534(nobody) groups=65534(nobody) [nobody@localhost ~]$ unshare --net --keep-caps [nobody@localhost ~]$ stat /proc/sys/net/core/somaxconn File: /proc/sys/net/core/somaxconn Size: 0 Blocks: 0 IO Block: 1024 regular empty file Device: 13h/19d Inode: 3726 Links: 1 Access: (0644/-rw-r--r--) Uid: (65534/ nobody) Gid: (65534/ nobody) Access: 2024-03-16 00:28:51.937789000 +0000 Modify: 2024-03-16 00:28:51.937789000 +0000 Change: 2024-03-16 00:28:51.937789000 +0000 Birth: - [nobody@localhost ~]$ cat /proc/sys/net/core/somaxconn 4096 [nobody@localhost ~]$ echo 2048 > /proc/sys/net/core/somaxconn [nobody@localhost ~]$ cat /proc/sys/net/core/somaxconn 2048 ---8<--- > > Fixes: e79c6a4fc923 ("net: make net namespace sysctls belong to container's owner") > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > net/sysctl_net.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/net/sysctl_net.c b/net/sysctl_net.c > index 051ed5f6fc93..03e320ddacc9 100644 > --- a/net/sysctl_net.c > +++ b/net/sysctl_net.c > @@ -62,12 +62,10 @@ static void net_ctl_set_ownership(struct ctl_table_header *head, > kgid_t ns_root_gid; > > ns_root_uid = make_kuid(net->user_ns, 0); > - if (uid_valid(ns_root_uid)) > - *uid = ns_root_uid; > + *uid = uid_valid(ns_root_uid) ? ns_root_uid : GLOBAL_ROOT_UID; > > ns_root_gid = make_kgid(net->user_ns, 0); > - if (gid_valid(ns_root_gid)) > - *gid = ns_root_gid; > + *gid = gid_valid(ns_root_gid) ? ns_root_gid : GLOBAL_ROOT_GID; > } How about setting the default in proc_sys_make_inode() instead ? because the default value configured by new_inode() is not appropriate for procfs, IIUC. Thanks! > > static struct ctl_table_root net_sysctl_root = { > > --- > base-commit: e5eb28f6d1afebed4bb7d740a797d0390bd3a357 > change-id: 20240315-sysctl-net-ownership-bc4e17eaeea6 > > Best regards, > -- > Thomas Weißschuh <linux@weissschuh.net>
On Fri, Mar 15, 2024 at 05:20:31PM +0100, Thomas Weißschuh wrote: > The sysctl core does not initialize these fields when the set_ownership > callback is present. This is a bit ambiguous as these values get set in the new_inode(sb) call. What is really happening is that the GLOBAL_ROOT_[U|G]ID is not getting the correct default. > So always do it in the callback. > > Fixes: e79c6a4fc923 ("net: make net namespace sysctls belong to container's owner") I think this is incorrect and here is why: 1. At the point of committing this code, the default behavior was dictated by the new_inode call. Which calculated the value from super_block *sb. This was Aug 10 2016 (approx). 2. The issue comes when a new default behavior is added by setting GLOBAL_ROOT_[U|G]ID in 5ec27ec735ba0 ("fs/proc/proc_sysctl.c: fix the default values of i_uid/i_gid on /proc/sys inodes"). This commit was in 2019 and missed adjusting net_ctl_set_ownership. > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > net/sysctl_net.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/net/sysctl_net.c b/net/sysctl_net.c > index 051ed5f6fc93..03e320ddacc9 100644 > --- a/net/sysctl_net.c > +++ b/net/sysctl_net.c > @@ -62,12 +62,10 @@ static void net_ctl_set_ownership(struct ctl_table_header *head, > kgid_t ns_root_gid; > > ns_root_uid = make_kuid(net->user_ns, 0); > - if (uid_valid(ns_root_uid)) > - *uid = ns_root_uid; > + *uid = uid_valid(ns_root_uid) ? ns_root_uid : GLOBAL_ROOT_UID; > > ns_root_gid = make_kgid(net->user_ns, 0); > - if (gid_valid(ns_root_gid)) > - *gid = ns_root_gid; > + *gid = gid_valid(ns_root_gid) ? ns_root_gid : GLOBAL_ROOT_GID; > } > > static struct ctl_table_root net_sysctl_root = { > > --- > base-commit: e5eb28f6d1afebed4bb7d740a797d0390bd3a357 > change-id: 20240315-sysctl-net-ownership-bc4e17eaeea6 > > Best regards, > -- > Thomas Weißschuh <linux@weissschuh.net> >
On Fri, Mar 15, 2024 at 05:39:58PM -0700, Kuniyuki Iwashima wrote: > From: Thomas Weißschuh <linux@weissschuh.net> > Date: Fri, 15 Mar 2024 17:20:31 +0100 > > The sysctl core does not initialize these fields when the set_ownership > > callback is present. > > So always do it in the callback. > > Could you add few more words here to explain what the problem is > like commit 5ec27ec735ba ("fs/proc/proc_sysctl.c: fix the default > values of i_uid/i_gid on /proc/sys inodes."). I second this proposal. There is more to the story than a simple setting of default values. IMO, you should mention how the missbehavior was introduced in 5ec27ec735ba0 (fs/proc/proc_sysctl.c: fix the default values of i_uid/i_gid on /proc/sys inodes.) > > BTW, it seems that we can change the value even if the uid/gid is > invalid unlike the issue mentioned in 5ec27ec735ba. Indeed and I think it is because net_ctl_permissions set the mode in the same way as test_perm does. Therefore regardless of the tests against GLOBAL_ROOT_[U|G]ID being true or false mode gets right shifted by 6 and 3. > > So, the problem is just the invalid uid/gid exposed to userspace > or am I missing something ? That seems to be the case. I think that should just be GLOBAL_ROOT_[G|U]ID. Please correct me if I'm wrong. > > ---8<--- > [root@localhost ~]# unshare --user --keep-caps > [nobody@localhost ~]$ id > uid=65534(nobody) gid=65534(nobody) groups=65534(nobody) > [nobody@localhost ~]$ unshare --net --keep-caps > [nobody@localhost ~]$ stat /proc/sys/net/core/somaxconn > File: /proc/sys/net/core/somaxconn > Size: 0 Blocks: 0 IO Block: 1024 regular empty file > Device: 13h/19d Inode: 3726 Links: 1 > Access: (0644/-rw-r--r--) Uid: (65534/ nobody) Gid: (65534/ nobody) > Access: 2024-03-16 00:28:51.937789000 +0000 > Modify: 2024-03-16 00:28:51.937789000 +0000 > Change: 2024-03-16 00:28:51.937789000 +0000 > Birth: - > [nobody@localhost ~]$ cat /proc/sys/net/core/somaxconn > 4096 > [nobody@localhost ~]$ echo 2048 > /proc/sys/net/core/somaxconn > [nobody@localhost ~]$ cat /proc/sys/net/core/somaxconn > 2048 > ---8<--- > > > > > > Fixes: e79c6a4fc923 ("net: make net namespace sysctls belong to container's owner") > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > --- > > net/sysctl_net.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/net/sysctl_net.c b/net/sysctl_net.c > > index 051ed5f6fc93..03e320ddacc9 100644 > > --- a/net/sysctl_net.c > > +++ b/net/sysctl_net.c > > @@ -62,12 +62,10 @@ static void net_ctl_set_ownership(struct ctl_table_header *head, > > kgid_t ns_root_gid; > > > > ns_root_uid = make_kuid(net->user_ns, 0); > > - if (uid_valid(ns_root_uid)) > > - *uid = ns_root_uid; > > + *uid = uid_valid(ns_root_uid) ? ns_root_uid : GLOBAL_ROOT_UID; > > > > ns_root_gid = make_kgid(net->user_ns, 0); > > - if (gid_valid(ns_root_gid)) > > - *gid = ns_root_gid; > > + *gid = gid_valid(ns_root_gid) ? ns_root_gid : GLOBAL_ROOT_GID; > > } > > How about setting the default in proc_sys_make_inode() instead ? > because the default value configured by new_inode() is not > appropriate for procfs, IIUC. Are you proposing something like this in proc_sys_make_inode/ inode->i_uid = GLOBAL_ROOT_UID; inode->i_gid = GLOBAL_ROOT_GID; if (root->set_ownership) root->set_ownership(head, table, &indoe->i_uid, &inode->i_gid) ? > > Thanks! > > > > > static struct ctl_table_root net_sysctl_root = { > > > > --- > > base-commit: e5eb28f6d1afebed4bb7d740a797d0390bd3a357 > > change-id: 20240315-sysctl-net-ownership-bc4e17eaeea6 > > > > Best regards, > > -- > > Thomas Weißschuh <linux@weissschuh.net>
On Fri, 2024-03-15 at 17:39 -0700, Kuniyuki Iwashima wrote: > on Fri, 15 Mar 2024 17:20:31 +0100 Thomas Weißschuh <linux@weissschuh.net wrote: > > > diff --git a/net/sysctl_net.c b/net/sysctl_net.c > > index 051ed5f6fc93..03e320ddacc9 100644 > > --- a/net/sysctl_net.c > > +++ b/net/sysctl_net.c > > @@ -62,12 +62,10 @@ static void net_ctl_set_ownership(struct ctl_table_header *head, > > kgid_t ns_root_gid; > > > > ns_root_uid = make_kuid(net->user_ns, 0); > > - if (uid_valid(ns_root_uid)) > > - *uid = ns_root_uid; > > + *uid = uid_valid(ns_root_uid) ? ns_root_uid : GLOBAL_ROOT_UID; > > > > ns_root_gid = make_kgid(net->user_ns, 0); > > - if (gid_valid(ns_root_gid)) > > - *gid = ns_root_gid; > > + *gid = gid_valid(ns_root_gid) ? ns_root_gid : GLOBAL_ROOT_GID; > > } > > How about setting the default in proc_sys_make_inode() instead ? > because the default value configured by new_inode() is not I also think that could be a better option, as the caller is already providing default values in some cases. Cheers, Paolo
diff --git a/net/sysctl_net.c b/net/sysctl_net.c index 051ed5f6fc93..03e320ddacc9 100644 --- a/net/sysctl_net.c +++ b/net/sysctl_net.c @@ -62,12 +62,10 @@ static void net_ctl_set_ownership(struct ctl_table_header *head, kgid_t ns_root_gid; ns_root_uid = make_kuid(net->user_ns, 0); - if (uid_valid(ns_root_uid)) - *uid = ns_root_uid; + *uid = uid_valid(ns_root_uid) ? ns_root_uid : GLOBAL_ROOT_UID; ns_root_gid = make_kgid(net->user_ns, 0); - if (gid_valid(ns_root_gid)) - *gid = ns_root_gid; + *gid = gid_valid(ns_root_gid) ? ns_root_gid : GLOBAL_ROOT_GID; } static struct ctl_table_root net_sysctl_root = {
The sysctl core does not initialize these fields when the set_ownership callback is present. So always do it in the callback. Fixes: e79c6a4fc923 ("net: make net namespace sysctls belong to container's owner") Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- net/sysctl_net.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) --- base-commit: e5eb28f6d1afebed4bb7d740a797d0390bd3a357 change-id: 20240315-sysctl-net-ownership-bc4e17eaeea6 Best regards,