| Message ID | 20241108101339.1560116-2-stsp2@yandex.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | implement PROCFS_SET_GROUPS ioctl | expand |
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
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?
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.
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;
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(-)