Received-SPF: pass (google.com: domain of linux-kernel+bounces-105012-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Kuniyuki Iwashima <kuniyu@amazon.com>
To: <linux@weissschuh.net>
CC: <davem@davemloft.net>, <dmitry.torokhov@gmail.com>,
	<ebiederm@xmission.com>, <edumazet@google.com>, <j.granados@samsung.com>,
	<kuba@kernel.org>, <linux-kernel@vger.kernel.org>, <mcgrof@kernel.org>,
	<netdev@vger.kernel.org>, <pabeni@redhat.com>, <kuniyu@amazon.com>
Subject: Re: [PATCH] net: always initialize sysctl ownership
Date: Fri, 15 Mar 2024 17:39:58 -0700
Message-ID: <20240316003958.65385-1-kuniyu@amazon.com>
In-Reply-To: <20240315-sysctl-net-ownership-v1-1-2b465555a292@weissschuh.net>
References: <20240315-sysctl-net-ownership-v1-1-2b465555a292@weissschuh.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

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>