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 10:48:15 +0200
From: Christian Brauner <brauner@...nel.org>
To: Adrian Ratiu <adrian.ratiu@...labora.com>
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 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)?

>  	struct mm_struct *mm = ERR_PTR(-ESRCH);
>  
>  	if (task) {
> @@ -816,7 +816,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>  
>  static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
>  {
> -	struct mm_struct *mm = proc_mem_open(inode, mode);
> +	struct mm_struct *mm = proc_mem_open(file, mode);
>  
>  	if (IS_ERR(mm))
>  		return PTR_ERR(mm);
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index a71ac5379584..d38b2eea40d1 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -295,7 +295,7 @@ struct proc_maps_private {
>  #endif
>  } __randomize_layout;
>  
> -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode);
> +struct mm_struct *proc_mem_open(struct file *file, unsigned int mode);
>  
>  extern const struct file_operations proc_pid_maps_operations;
>  extern const struct file_operations proc_pid_numa_maps_operations;
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index f8d35f993fe5..fe3b2182b0aa 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -210,7 +210,7 @@ static int proc_maps_open(struct inode *inode, struct file *file,
>  		return -ENOMEM;
>  
>  	priv->inode = inode;
> -	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +	priv->mm = proc_mem_open(file, PTRACE_MODE_READ);
>  	if (IS_ERR(priv->mm)) {
>  		int err = PTR_ERR(priv->mm);
>  
> @@ -1030,7 +1030,7 @@ static int smaps_rollup_open(struct inode *inode, struct file *file)
>  		goto out_free;
>  
>  	priv->inode = inode;
> -	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +	priv->mm = proc_mem_open(file, PTRACE_MODE_READ);
>  	if (IS_ERR(priv->mm)) {
>  		ret = PTR_ERR(priv->mm);
>  
> @@ -1754,7 +1754,7 @@ static int pagemap_open(struct inode *inode, struct file *file)
>  {
>  	struct mm_struct *mm;
>  
> -	mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +	mm = proc_mem_open(file, PTRACE_MODE_READ);
>  	if (IS_ERR(mm))
>  		return PTR_ERR(mm);
>  	file->private_data = mm;
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index bce674533000..a8ab182a4ed1 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -259,7 +259,7 @@ static int maps_open(struct inode *inode, struct file *file,
>  		return -ENOMEM;
>  
>  	priv->inode = inode;
> -	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +	priv->mm = proc_mem_open(file, PTRACE_MODE_READ);
>  	if (IS_ERR(priv->mm)) {
>  		int err = PTR_ERR(priv->mm);
>  
> -- 
> 2.44.2
>