proc: Don't allow empty /proc/PID/cmdline for user tasks

Kernel threads have empty /proc/PID/cmdline and some userland tools including ps(1) and older versions of systemd use this to detect kernel threads. However, any userland program can emulate the behavior by making its argvs unavailable and trick the affected tools into thinking that the task is a kernel thread. Linus's reproducer follows.

  #include <sys/prctl.h>
  #include <sys/mman.h>

int main(void)

  {
	  char empty[16384];
	  unsigned long ptr;

	  asm volatile("" :"=r" (ptr) : "0" (empty):"memory");
	  ptr = (ptr+4095) & ~4095;
	  munmap((void *)ptr, 32768);

	  sleep(1000);
	  return 0;

}

Compiling the above program into nullcmdline and running it on an unpatche kernel shows the following behavior.

  $ ./nullcmdline &
  [1] 2382031
  [devbig577 ~/tmp]$ hexdump -C /proc/2382031/comm
  00000000  6e 75 6c 6c 63 6d 64 6c  69 6e 65 0a              |nullcmdline.|
  0000000c
  $ hexdump -C /proc/2382031/cmdline
  $ ps 2382031
      PID TTY      STAT   TIME COMMAND
  2382031 pts/2    S      0:00 [nullcmdline]

The empty cmdline makes ps(1) think that nullcmdline is a kernel thread and put brackets around its name (comm), which is mostly a nuisance but it's possible that this confusion can lead to more harmful confusions.

This patch fixes the issue by making proc_pid_cmdline_read() never return empty string for user tasks. If the result is empty for whatever reason, comm string is returned. Even when the comm string is empty, it still returns the null termnation character. On a patched kernel, running the same command as above gives us.

  $ ./nullcmdline &
  [1] 2317
  [test ~]# hexdump -C /proc/2317/comm
  00000000  6e 75 6c 6c 63 6d 64 6c  69 6e 65 0a              |nullcmdline.|
  0000000c
  $ hexdump -C /proc/2317/cmdline
  00000000  6e 75 6c 6c 63 6d 64 6c  69 6e 65 00              |nullcmdline.|
  0000000c
  $ ps 2317
    PID TTY      STAT   TIME COMMAND
   2317 pts/0    S      0:00 nullcmdline

Note that cmdline is a dup of comm and ps(1) is no longer confused.

---
Hello,

Linus, this patch is somewhat different from the rest of workqueue changes and it could make sense to apply separately, so please feel free to apply directly. If you want it to be routed together with the other workqueue changes, please let me know.

Thanks.

 fs/proc/base.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

@@ -224,9 +224,10 @@ static ssize_t proc_pid_cmdline_read(str
 	if (!tsk)
 		return -ESRCH;
 	mm = get_task_mm(tsk);
-	put_task_struct(tsk);
-	if (!mm)
-		return 0;
+	if (!mm) {
+		rv = 0;
+		goto out_put_task;
+	}
 	/* Check if process spawned far enough to have cmdline. */
 	if (!mm->env_end) {
 		rv = 0;

@@ -367,8 +368,23 @@ out_free_page:
 	free_page((unsigned long)page);
 out_mmput:
 	mmput(mm);
+out_put_task:
+	/*
+	 * Some userland tools use empty cmdline to distinguish kthreads.
+	 * Avoid empty cmdline for user tasks by returning tsk->comm with
+	 * \0 termination when empty.
+	 */
+	if (*pos == 0 && rv == 0 && !(tsk->flags & PF_KTHREAD)) {
+		char tcomm[TASK_COMM_LEN];
+
+		get_task_comm(tcomm, tsk);
+		rv = min(strlen(tcomm) + 1, count);
+		if (copy_to_user(buf, tsk->comm, rv))
+			rv = -EFAULT;
+	}
 	if (rv > 0)
 		*pos += rv;
+	put_task_struct(tsk);
 	return rv;
 }
 

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

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

index 1a76d751cf3c..c4d963a12162 100644
@@ -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,