linux-kernel - Re: [PATCH v6 1/2] proc: pass file instead of inode to proc_mem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 17 Jun 2024 11:47:03 +0100
From: "Adrian Ratiu" <adrian.ratiu@...labora.com>
To: "Christian Brauner" <brauner@...nel.org>
Cc: linux-fsdevel@...r.kernel.org, linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org, linux-doc@...r.kernel.org, kernel@...labora.com, gbiv@...gle.com, ryanbeltran@...gle.com, inglorion@...gle.com, ajordanr@...gle.com, jorgelo@...omium.org, "Jann Horn" <jannh@...gle.com>, "Kees Cook" <keescook@...omium.org>, "Jeff Xu" <jeffxu@...gle.com>, "Kees Cook" <kees@...nel.org>
Subject: Re: [PATCH v6 1/2] proc: pass file instead of 
 inode to proc_mem_open

On Monday, June 17, 2024 11:48 EEST, Christian Brauner <brauner@...nel.org> wrote:

> On Thu, Jun 13, 2024 at 04:39:36PM GMT, Adrian Ratiu wrote:
> > The file struct is required in proc_mem_open() so its
> > f_mode can be checked when deciding whether to allow or
> > deny /proc/*/mem open requests via the new read/write
> > and foll_force restriction mechanism.
> > 
> > Thus instead of directly passing the inode to the fun,
> > we pass the file and get the inode inside it.
> > 
> > Cc: Jann Horn <jannh@...gle.com>
> > Cc: Kees Cook <keescook@...omium.org>
> > Cc: Christian Brauner <brauner@...nel.org>
> > Cc: Jeff Xu <jeffxu@...gle.com>
> > Signed-off-by: Adrian Ratiu <adrian.ratiu@...labora.com>
> > Reviewed-by: Kees Cook <kees@...nel.org>
> > ---
> 
> I've tentatively applies this patch to #vfs.procfs.
> One comment, one question:
> 
> > No changes in v6
> > ---
> >  fs/proc/base.c       | 6 +++---
> >  fs/proc/internal.h   | 2 +-
> >  fs/proc/task_mmu.c   | 6 +++---
> >  fs/proc/task_nommu.c | 2 +-
> >  4 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 72a1acd03675..4c607089f66e 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -794,9 +794,9 @@ static const struct file_operations proc_single_file_operations = {
> >  };
> >  
> >  
> > -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
> > +struct mm_struct *proc_mem_open(struct file  *file, unsigned int mode)
> >  {
> > -	struct task_struct *task = get_proc_task(inode);
> > +	struct task_struct *task = get_proc_task(file->f_inode);
> 
> Comment: This should use file_inode(file) but I've just fixed that when I
> applied.
> 
> Question: Is this an equivalent transformation. So is the inode that was
> passed to proc_mem_open() always the same inode as file_inode(file)?

Thank you!

Yes, the inode associated with the file struct should be always the same
while the file is opened, so the link set during the top-level mem_open()
callback should still hold while it itself calls into its sub-functions like
proc_mem_open().