From: Joel Granados <j.granados@samsung.com>
To: Kuniyuki Iwashima <kuniyu@amazon.com>
Cc: <linux@weissschuh.net>, <davem@davemloft.net>,
<dmitry.torokhov@gmail.com>, <ebiederm@xmission.com>,
<edumazet@google.com>, <kuba@kernel.org>,
<linux-kernel@vger.kernel.org>, <mcgrof@kernel.org>,
<netdev@vger.kernel.org>, <pabeni@redhat.com>
Subject: Re: [PATCH] net: always initialize sysctl ownership
Date: Tue, 19 Mar 2024 14:21:58 +0100 [thread overview]
Message-ID: <20240319132158.37aitf2ysxqcd3bt@joelS2.panther.com> (raw)
In-Reply-To: <20240316003958.65385-1-kuniyu@amazon.com>
[-- Attachment #1: Type: text/plain, Size: 3824 bytes --]
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>
--
Joel Granados
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
next prev parent reply other threads:[~2024-03-19 13:22 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20240315162041eucas1p1b9254ef7e70f21b24b88d0999c34e9bd@eucas1p1.samsung.com>
2024-03-15 16:20 ` [PATCH] net: always initialize sysctl ownership Thomas Weißschuh
2024-03-16 0:39 ` Kuniyuki Iwashima
2024-03-19 13:21 ` Joel Granados [this message]
2024-03-19 14:47 ` Paolo Abeni
2024-03-19 13:07 ` Joel Granados
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240319132158.37aitf2ysxqcd3bt@joelS2.panther.com \
--to=j.granados@samsung.com \
--cc=davem@davemloft.net \
--cc=dmitry.torokhov@gmail.com \
--cc=ebiederm@xmission.com \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=kuniyu@amazon.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@weissschuh.net \
--cc=mcgrof@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.