Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Jul 2023 13:49:39 +0200
From:   Christian Brauner <brauner@kernel.org>
To:     Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
Cc:     Willy Tarreau <w@1wt.eu>, Shuah Khan <shuah@kernel.org>,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, Zhangjin Wu <falcon@tinylab.org>,
        Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH 2/2] proc: use generic setattr() for /proc/$PID/net
Message-ID: <20230713-abitur-alliierte-93460a210b1b@brauner>
References: <20230624-proc-net-setattr-v1-0-73176812adee@weissschuh.net>
 <20230624-proc-net-setattr-v1-2-73176812adee@weissschuh.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230624-proc-net-setattr-v1-2-73176812adee@weissschuh.net>
Precedence: bulk

On Sat, Jun 24, 2023 at 12:30:47PM +0200, Thomas Weißschuh wrote:
> All other files in /proc/$PID/ use proc_setattr().
> 
> Not using it allows the usage of chmod() on /proc/$PID/net, even on
> other processes owned by the same user.
> The same would probably also be true for other attributes to be changed.
> 
> As this technically represents an ABI change it is not marked for
> stable so any unlikely regressions are caught during a full release cycle.
> 
> Fixes: e9720acd728a ("[NET]: Make /proc/net a symlink on /proc/self/net (v3)")
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  fs/proc/proc_net.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
> index a0c0419872e3..78f9e6b469c0 100644
> --- a/fs/proc/proc_net.c
> +++ b/fs/proc/proc_net.c
> @@ -321,6 +321,7 @@ static int proc_tgid_net_getattr(struct mnt_idmap *idmap,
>  const struct inode_operations proc_net_inode_operations = {
>  	.lookup		= proc_tgid_net_lookup,
>  	.getattr	= proc_tgid_net_getattr,
> +	.setattr        = proc_setattr,
>  };
>  
>  static int proc_tgid_net_readdir(struct file *file, struct dir_context *ctx)

So your concern really is specifically about /proc/$pid/net itself as
that's owned by the user and thus the user itself can chmod it and thus
also restrict access for other processess running with the same uid:

chmod 0000 /proc/1234/net
ls -al /proc/self/net
ls: cannot open directory '/proc/self/net/': Permission denied

Yeah, it's not a huge deal but it's arguably a bug especially since the
original commit from 2006 that introduced proc_setattr() was clear that
it should apply to anything beneath /proc/<pid>/ owned by the user.

So I agree and we should probably try and have the same behavior for
/proc/$pid/net as well. We can see if that breaks something.