linux-kernel - Re: [PATCH] proc: Fix the issue of 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]

Message-ID: <ajipijba74lvxh2qqyxbxtbmlqil2smsuxayym5ipbmjdysxq2@stvu4kt62yzu>
Date: Mon, 24 Mar 2025 18:48:35 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Penglei Jiang <superman.xpt@...il.com>
Cc: brauner@...nel.org, akpm@...ux-foundation.org, 
	lorenzo.stoakes@...cle.com, tglx@...utronix.de, jlayton@...nel.org, viro@...iv.linux.org.uk, 
	felix.moessbauer@...mens.com, adrian.ratiu@...labora.com, xu.xin16@....com.cn, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	syzbot+f9238a0a31f9b5603fef@...kaller.appspotmail.com, syzbot+02e64be5307d72e9c309@...kaller.appspotmail.com
Subject: Re: [PATCH] proc: Fix the issue of proc_mem_open returning NULL

On Mon, Mar 24, 2025 at 09:23:53AM -0700, Penglei Jiang wrote:
> The following functions call proc_mem_open but do not handle the case
> where it returns NULL:
> 
>   __mem_open in fs/proc/base.c
>   proc_maps_open in fs/proc/task_mmu.c
>   smaps_rollup_open in fs/proc/task_mmu.c
>   pagemap_open in fs/proc/task_mmu.c
>   maps_open in fs/proc/task_nommu.c
> 
> The following reported bugs may be related to this issue:
> 
>   https://lore.kernel.org/all/000000000000f52642060d4e3750@google.com
>   https://lore.kernel.org/all/0000000000001bc4a00612d9a7f4@google.com
> 
> Fix:
> 
> Modify proc_mem_open to return an error code in case of errors, instead
> of returning NULL.
> 

The rw routines associated with these consumers explictly NULL check
mm, which becomes redundant with the patch.

While I find it fishy that returning NULL was ever a thing to begin
with, it is unclear to me if it can be easily changed now from
userspace-visible behavior standpoint.

I think the best way forward for the time being is to add the missing
NULL checks instead.

> Signed-off-by: Penglei Jiang <superman.xpt@...il.com>
> ---
>  fs/proc/base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index cd89e956c322..b5e7317cf0dc 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -840,7 +840,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>  	put_task_struct(task);
>  
>  	if (IS_ERR(mm))
> -		return mm == ERR_PTR(-ESRCH) ? NULL : mm;
> +		return mm;
>  
>  	/* ensure this mm_struct can't be freed */
>  	mmgrab(mm);
> -- 
> 2.17.1
>