linux-kernel - [PATCH V2] 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: <20250325041448.43211-1-superman.xpt@gmail.com>
Date: Mon, 24 Mar 2025 21:14:48 -0700
From: Penglei Jiang <superman.xpt@...il.com>
To: mjguzik@...il.com
Cc: adrian.ratiu@...labora.com,
	akpm@...ux-foundation.org,
	brauner@...nel.org,
	felix.moessbauer@...mens.com,
	jlayton@...nel.org,
	linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	lorenzo.stoakes@...cle.com,
	superman.xpt@...il.com,
	syzbot+02e64be5307d72e9c309@...kaller.appspotmail.com,
	syzbot+f9238a0a31f9b5603fef@...kaller.appspotmail.com,
	tglx@...utronix.de,
	viro@...iv.linux.org.uk,
	xu.xin16@....com.cn
Subject: [PATCH V2] proc: Fix the issue of proc_mem_open returning NULL

Hi, all

On Mon, 24 Mar 2025 18:48:35 +0100, Mateusz Guzik wrote:
> 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
> >

Mateusz Guzik provides valuable suggestions.

Complete the missing NULL checks.

Signed-off-by: Penglei Jiang <superman.xpt@...il.com>
---
 fs/proc/base.c       |  4 ++--
 fs/proc/task_mmu.c   | 12 ++++++------
 fs/proc/task_nommu.c |  4 ++--
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index cd89e956c322..d898cec3ee71 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -854,8 +854,8 @@ static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
 {
 	struct mm_struct *mm = proc_mem_open(inode, mode);
 
-	if (IS_ERR(mm))
-		return PTR_ERR(mm);
+	if (IS_ERR_OR_NULL(mm))
+		return mm ? PTR_ERR(mm) : -ESRCH;
 
 	file->private_data = mm;
 	return 0;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f02cd362309a..14d1d8d3e432 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -212,8 +212,8 @@ static int proc_maps_open(struct inode *inode, struct file *file,
 
 	priv->inode = inode;
 	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
-	if (IS_ERR(priv->mm)) {
-		int err = PTR_ERR(priv->mm);
+	if (IS_ERR_OR_NULL(priv->mm)) {
+		int err = priv->mm ? PTR_ERR(priv->mm) : -ESRCH;
 
 		seq_release_private(inode, file);
 		return err;
@@ -1312,8 +1312,8 @@ static int smaps_rollup_open(struct inode *inode, struct file *file)
 
 	priv->inode = inode;
 	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
-	if (IS_ERR(priv->mm)) {
-		ret = PTR_ERR(priv->mm);
+	if (IS_ERR_OR_NULL(priv->mm)) {
+		ret = priv->mm ? PTR_ERR(priv->mm) : -ESRCH;
 
 		single_release(inode, file);
 		goto out_free;
@@ -2045,8 +2045,8 @@ static int pagemap_open(struct inode *inode, struct file *file)
 	struct mm_struct *mm;
 
 	mm = proc_mem_open(inode, PTRACE_MODE_READ);
-	if (IS_ERR(mm))
-		return PTR_ERR(mm);
+	if (IS_ERR_OR_NULL(mm))
+		return mm ? PTR_ERR(mm) : -ESRCH;
 	file->private_data = mm;
 	return 0;
 }
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index bce674533000..59bfd61d653a 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -260,8 +260,8 @@ static int maps_open(struct inode *inode, struct file *file,
 
 	priv->inode = inode;
 	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
-	if (IS_ERR(priv->mm)) {
-		int err = PTR_ERR(priv->mm);
+	if (IS_ERR_OR_NULL(priv->mm)) {
+		int err = priv->mm ? PTR_ERR(priv->mm) : -ESRCH;
 
 		seq_release_private(inode, file);
 		return err;
-- 
2.17.1