1. Patchew
  2. linux
  3. [PATCH v2 0/2] implement PROCFS_SET_GROUPS ioctl
  4. View patch

[PATCH v2 1/2] procfs: avoid some usages of seq_file private data

Stas Sergeev posted 2 patches 2 weeks, 1 day ago
There is a newer version of this series
[PATCH v2 1/2] procfs: avoid some usages of seq_file private data
Posted by Stas Sergeev 2 weeks, 1 day ago 
seq_file private data carries the inode pointer here.
Replace
`struct inode *inode = m->private;`
with:
`struct inode *inode = file_inode(m->file);`
to avoid the reliance on private data.

This is needed so that `proc_single_show()` can be used by
custom fops that utilize seq_file private data for other things.
This is used in the next patch.

Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

CC: Eric Biederman <ebiederm@xmission.com>
CC: Andy Lutomirski <luto@kernel.org>
CC: Aleksa Sarai <cyphar@cyphar.com>
CC: Christian Brauner <brauner@kernel.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Jeff Layton <jlayton@kernel.org>
CC: Kees Cook <kees@kernel.org>
CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Felix Moessbauer <felix.moessbauer@siemens.com>
CC: Adrian Ratiu <adrian.ratiu@collabora.com>
CC: Casey Schaufler <casey@schaufler-ca.com>
CC: linux-kernel@vger.kernel.org
CC: linux-fsdevel@vger.kernel.org
CC: Jan Kara <jack@suse.cz>
CC: Chengming Zhou <chengming.zhou@linux.dev>
CC: Jens Axboe <axboe@kernel.dk>
CC: Oleg Nesterov <oleg@redhat.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
---
 fs/proc/base.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index b31283d81c52..015db8752a99 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -528,7 +528,7 @@ static int proc_pid_schedstat(struct seq_file *m, struct pid_namespace *ns,
 static int lstats_show_proc(struct seq_file *m, void *v)
 {
 	int i;
-	struct inode *inode = m->private;
+	struct inode *inode = file_inode(m->file);
 	struct task_struct *task = get_proc_task(inode);
 
 	if (!task)
@@ -800,7 +800,7 @@ static const struct inode_operations proc_def_inode_operations = {
 
 static int proc_single_show(struct seq_file *m, void *v)
 {
-	struct inode *inode = m->private;
+	struct inode *inode = file_inode(m->file);
 	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct pid *pid = proc_pid(inode);
 	struct task_struct *task;
@@ -1494,7 +1494,7 @@ static const struct file_operations proc_fail_nth_operations = {
  */
 static int sched_show(struct seq_file *m, void *v)
 {
-	struct inode *inode = m->private;
+	struct inode *inode = file_inode(m->file);
 	struct pid_namespace *ns = proc_pid_ns(inode->i_sb);
 	struct task_struct *p;
 
@@ -1546,7 +1546,7 @@ static const struct file_operations proc_pid_sched_operations = {
  */
 static int sched_autogroup_show(struct seq_file *m, void *v)
 {
-	struct inode *inode = m->private;
+	struct inode *inode = file_inode(m->file);
 	struct task_struct *p;
 
 	p = get_proc_task(inode);
@@ -1745,7 +1745,7 @@ static ssize_t comm_write(struct file *file, const char __user *buf,
 
 static int comm_show(struct seq_file *m, void *v)
 {
-	struct inode *inode = m->private;
+	struct inode *inode = file_inode(m->file);
 	struct task_struct *p;
 
 	p = get_proc_task(inode);
@@ -2641,7 +2641,7 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
 
 static int timerslack_ns_show(struct seq_file *m, void *v)
 {
-	struct inode *inode = m->private;
+	struct inode *inode = file_inode(m->file);
 	struct task_struct *p;
 	int err = 0;
 
-- 
2.47.0
Re: [PATCH v2 1/2] procfs: avoid some usages of seq_file private data
Posted by Kees Cook 2 weeks, 1 day ago 

On November 8, 2024 2:13:38 AM PST, Stas Sergeev <stsp2@yandex.ru> wrote:
>seq_file private data carries the inode pointer here.
>Replace
>`struct inode *inode = m->private;`
>with:
>`struct inode *inode = file_inode(m->file);`
>to avoid the reliance on private data.

Conceptually this seems good, though I'd expect to see the removal of _setting_ m->private too in this patch.

>This is needed so that `proc_single_show()` can be used by
>custom fops that utilize seq_file private data for other things.
>This is used in the next patch.

Now that next patch is pretty wild. I think using proc is totally wrong for managing uid/gid. If that's going to happen at all, I think it should be tied to pidfd which will already do the correct process lifetime management, etc.

-Kees

-- 
Kees Cook
Re: [PATCH v2 1/2] procfs: avoid some usages of seq_file private data
Posted by stsp 2 weeks, 1 day ago 
08.11.2024 18:32, Kees Cook пишет:
> On November 8, 2024 2:13:38 AM PST, Stas Sergeev <stsp2@yandex.ru> wrote:
>> seq_file private data carries the inode pointer here.
>> Replace
>> `struct inode *inode = m->private;`
>> with:
>> `struct inode *inode = file_inode(m->file);`
>> to avoid the reliance on private data.
> Conceptually this seems good, though I'd expect to see the removal of _setting_ m->private too in this patch.

Done and sent v3.

>> This is needed so that `proc_single_show()` can be used by
>> custom fops that utilize seq_file private data for other things.
>> This is used in the next patch.
> Now that next patch is pretty wild. I think using proc is totally wrong for managing uid/gid. If that's going to happen at all,

And if not - it would be good if someone
tells how to fix the actual problem then.
I think the closest thing was credfd discussed
here:
https://lkml2.uits.iu.edu/hypermail/linux/kernel/1403.3/01528.html
But /proc/self/status already carries creds,
so what else is it if not credfd? :)
I can't even think of what else the read()
syscall should return on an actual hypothetical
credfd - other than what it returns now when
reading /proc/self/status.


>   I think it should be tied to pidfd which will already do the correct process lifetime management, etc.
Please let me know the exact scheme
you have in mind so that I can try it out.
I don't see any obvious mapping of my
current proposal to pidfd, so I can't guess.
Re: [PATCH v2 1/2] procfs: avoid some usages of seq_file private data
Posted by stsp 2 weeks, 1 day ago 
08.11.2024 18:32, Kees Cook пишет:
>
> On November 8, 2024 2:13:38 AM PST, Stas Sergeev <stsp2@yandex.ru> wrote:
>> seq_file private data carries the inode pointer here.
>> Replace
>> `struct inode *inode = m->private;`
>> with:
>> `struct inode *inode = file_inode(m->file);`
>> to avoid the reliance on private data.
> Conceptually this seems good, though I'd expect to see the removal of _setting_ m->private too in this patch.

Sure I can try to do that, perhaps as
an unrelated patch.
Just got scared to post large patches
and deal with potential problems where
I didn't even mean to change anything.

>> This is needed so that `proc_single_show()` can be used by
>> custom fops that utilize seq_file private data for other things.
>> This is used in the next patch.
> Now that next patch is pretty wild. I think using proc is totally wrong for managing uid/gid. If that's going to happen at all, I think it should be tied to pidfd which will already do the correct process lifetime management, etc.
I did the POC with pidfd:

https://lore.kernel.org/lkml/20241101202657.468595-1-stsp2@yandex.ru/T/

For me it was just a random place to
hack a POC on. I then searched for something
more realistic and choose proc/status because
it already carries all the needed info inside.
So in this case it can be read from, validated,
then applied with ioctl.

How exactly do you foresee using pidfd?
I mean, unless I am misunderstanding the
pidfd intention, it is always opened by the
pid of another process. There is no way of
some process to "allow" others opening its
pid, or to even know they did. Is it possible
to use pidfd in such a way that the process
can grant his groups explicitly?
In my POC I had to do a totally silly thing
of opening the _own_ pid and then sending
the resulting fd with SCM_RIGHTS. I don't
think people would do something like that.
What technique do you have in the mind?

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.