Re: [PATCH] proc: Don't allow empty /proc/PID/cmdline for user tasks

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On Wed, May 16, 2018 at 6:21 PM Tejun Heo <tj@kernel.org> wrote:

> This patch fixes the issue by making proc_pid_cmdline_read() never
> return empty string for user tasks.

Ugh.

That function really is too damn ugly, and this just makes it worse.

Can we please split things up a bit before uglifying the code further?

IOW, the first step should be something like the attached patch, which
splits up all the tsk/mm error cases.

Then your patch could just be a trivial

     if (!*pos && !ret)
         ret = get_comm_cmdline(tsk, buf, count, pos);

in that new (and much simpler) proc_pid_cmdline_read() just before the
put_task_struct.

Hmm?

NOTE! This patch is *entirely* untested, but it builds and the conversion
was pretty much entirely mechanical.

And yes, the "get_mm_cmdline()" function is still too damn ugly, and should
still be cleaned up more, but it's at least _slightly_ simpler than it used
to be, and the new logic wouldn't go into that horrible thing.

                Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2382 bytes --]

 fs/proc/base.c | 64 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 25 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1a76d751cf3c..c4d963a12162 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -205,11 +205,9 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
 	return result;
 }
 
-static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
-				     size_t _count, loff_t *pos)
+static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
+			      size_t _count, loff_t *pos)
 {
-	struct task_struct *tsk;
-	struct mm_struct *mm;
 	char *page;
 	unsigned long count = _count;
 	unsigned long arg_start, arg_end, env_start, env_end;
@@ -218,26 +216,13 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 	char c;
 	ssize_t rv;
 
-	BUG_ON(*pos < 0);
-
-	tsk = get_proc_task(file_inode(file));
-	if (!tsk)
-		return -ESRCH;
-	mm = get_task_mm(tsk);
-	put_task_struct(tsk);
-	if (!mm)
-		return 0;
 	/* Check if process spawned far enough to have cmdline. */
-	if (!mm->env_end) {
-		rv = 0;
-		goto out_mmput;
-	}
+	if (!mm->env_end)
+		return 0;
 
 	page = (char *)__get_free_page(GFP_KERNEL);
-	if (!page) {
-		rv = -ENOMEM;
-		goto out_mmput;
-	}
+	if (!page)
+		return -ENOMEM;
 
 	down_read(&mm->mmap_sem);
 	arg_start = mm->arg_start;
@@ -365,13 +350,42 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 
 out_free_page:
 	free_page((unsigned long)page);
-out_mmput:
-	mmput(mm);
-	if (rv > 0)
-		*pos += rv;
 	return rv;
 }
 
+static ssize_t get_task_cmdline(struct task_struct *tsk, char __user *buf,
+				size_t count, loff_t *pos)
+{
+	struct mm_struct *mm;
+	ssize_t ret;
+
+	mm = get_task_mm(tsk);
+	if (!mm)
+		return 0;
+
+	ret = get_mm_cmdline(mm, buf, count, pos);
+	mmput(mm);
+	return ret;
+}
+
+static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
+				     size_t count, loff_t *pos)
+{
+	struct task_struct *tsk;
+	ssize_t ret;
+
+	BUG_ON(*pos < 0);
+
+	tsk = get_proc_task(file_inode(file));
+	if (!tsk)
+		return -ESRCH;
+	ret = get_task_cmdline(tsk, buf, count, pos);
+	put_task_struct(tsk);
+	if (ret > 0)
+		*pos += ret;
+	return ret;
+}
+
 static const struct file_operations proc_pid_cmdline_ops = {
 	.read	= proc_pid_cmdline_read,
 	.llseek	= generic_file_llseek,