Merge branch 'proc-cmdline' (/proc/<pid>/cmdline fixes)
authorLinus Torvalds <torvalds@linux-foundation.org>
Tue, 16 Jul 2019 17:37:27 +0000 (10:37 -0700)
committerLinus Torvalds <torvalds@linux-foundation.org>
Tue, 16 Jul 2019 17:37:27 +0000 (10:37 -0700)
This fixes two problems reported with the cmdline simplification and
cleanup last year:

 - the setproctitle() special cases didn't quite match the original
   semantics, and it can be noticeable:

      https://lore.kernel.org/lkml/alpine.LNX.2.21.1904052326230.3249@kich.toxcorp.com/

 - it could leak an uninitialized byte from the temporary buffer under
   the right (wrong) circustances:

      https://lore.kernel.org/lkml/20190712160913.17727-1-izbyshev@ispras.ru/

It rewrites the logic entirely, splitting it into two separate commits
(and two separate functions) for the two different cases ("unedited
cmdline" vs "setproctitle() has been used to change the command line").

* proc-cmdline:
  /proc/<pid>/cmdline: add back the setproctitle() special case
  /proc/<pid>/cmdline: remove all the special cases

1  2 
fs/proc/base.c

diff --combined fs/proc/base.c
@@@ -209,12 -209,53 +209,53 @@@ static int proc_root_link(struct dentr
        return result;
  }
  
+ /*
+  * If the user used setproctitle(), we just get the string from
+  * user space at arg_start, and limit it to a maximum of one page.
+  */
+ static ssize_t get_mm_proctitle(struct mm_struct *mm, char __user *buf,
+                               size_t count, unsigned long pos,
+                               unsigned long arg_start)
+ {
+       char *page;
+       int ret, got;
+       if (pos >= PAGE_SIZE)
+               return 0;
+       page = (char *)__get_free_page(GFP_KERNEL);
+       if (!page)
+               return -ENOMEM;
+       ret = 0;
+       got = access_remote_vm(mm, arg_start, page, PAGE_SIZE, FOLL_ANON);
+       if (got > 0) {
+               int len = strnlen(page, got);
+               /* Include the NUL character if it was found */
+               if (len < got)
+                       len++;
+               if (len > pos) {
+                       len -= pos;
+                       if (len > count)
+                               len = count;
+                       len -= copy_to_user(buf, page+pos, len);
+                       if (!len)
+                               len = -EFAULT;
+                       ret = len;
+               }
+       }
+       free_page((unsigned long)page);
+       return ret;
+ }
  static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
                              size_t count, loff_t *ppos)
  {
        unsigned long arg_start, arg_end, env_start, env_end;
        unsigned long pos, len;
-       char *page;
+       char *page, c;
  
        /* Check if process spawned far enough to have cmdline. */
        if (!mm->env_end)
                return 0;
  
        /*
-        * We have traditionally allowed the user to re-write
-        * the argument strings and overflow the end result
-        * into the environment section. But only do that if
-        * the environment area is contiguous to the arguments.
+        * We allow setproctitle() to overwrite the argument
+        * strings, and overflow past the original end. But
+        * only when it overflows into the environment area.
         */
-       if (env_start != arg_end || env_start >= env_end)
+       if (env_start != arg_end || env_end < env_start)
                env_start = env_end = arg_end;
-       /* .. and limit it to a maximum of one page of slop */
-       if (env_end >= arg_end + PAGE_SIZE)
-               env_end = arg_end + PAGE_SIZE - 1;
+       len = env_end - arg_start;
  
        /* We're not going to care if "*ppos" has high bits set */
-       pos = arg_start + *ppos;
-       /* .. but we do check the result is in the proper range */
-       if (pos < arg_start || pos >= env_end)
+       pos = *ppos;
+       if (pos >= len)
                return 0;
+       if (count > len - pos)
+               count = len - pos;
+       if (!count)
+               return 0;
+       /*
+        * Magical special case: if the argv[] end byte is not
+        * zero, the user has overwritten it with setproctitle(3).
+        *
+        * Possible future enhancement: do this only once when
+        * pos is 0, and set a flag in the 'struct file'.
+        */
+       if (access_remote_vm(mm, arg_end-1, &c, 1, FOLL_ANON) == 1 && c)
+               return get_mm_proctitle(mm, buf, count, pos, arg_start);
  
-       /* .. and we never go past env_end */
-       if (env_end - pos < count)
-               count = env_end - pos;
+       /*
+        * For the non-setproctitle() case we limit things strictly
+        * to the [arg_start, arg_end[ range.
+        */
+       pos += arg_start;
+       if (pos < arg_start || pos >= arg_end)
+               return 0;
+       if (count > arg_end - pos)
+               count = arg_end - pos;
  
        page = (char *)__get_free_page(GFP_KERNEL);
        if (!page)
        while (count) {
                int got;
                size_t size = min_t(size_t, PAGE_SIZE, count);
-               long offset;
-               /*
-                * Are we already starting past the official end?
-                * We always include the last byte that is *supposed*
-                * to be NUL
-                */
-               offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
  
-               got = access_remote_vm(mm, pos - offset, page, size + offset, FOLL_ANON);
-               if (got <= offset)
+               got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
+               if (got <= 0)
                        break;
-               got -= offset;
-               /* Don't walk past a NUL character once you hit arg_end */
-               if (pos + got >= arg_end) {
-                       int n = 0;
-                       /*
-                        * If we started before 'arg_end' but ended up
-                        * at or after it, we start the NUL character
-                        * check at arg_end-1 (where we expect the normal
-                        * EOF to be).
-                        *
-                        * NOTE! This is smaller than 'got', because
-                        * pos + got >= arg_end
-                        */
-                       if (pos < arg_end)
-                               n = arg_end - pos - 1;
-                       /* Cut off at first NUL after 'n' */
-                       got = n + strnlen(page+n, offset+got-n);
-                       if (got < offset)
-                               break;
-                       got -= offset;
-                       /* Include the NUL if it existed */
-                       if (got < size)
-                               got++;
-               }
-               got -= copy_to_user(buf, page+offset, got);
+               got -= copy_to_user(buf, page, got);
                if (unlikely(!got)) {
                        if (!len)
                                len = -EFAULT;
@@@ -532,7 -550,8 +550,7 @@@ static int proc_oom_score(struct seq_fi
        unsigned long totalpages = totalram_pages() + total_swap_pages;
        unsigned long points = 0;
  
 -      points = oom_badness(task, NULL, NULL, totalpages) *
 -                                      1000 / totalpages;
 +      points = oom_badness(task, totalpages) * 1000 / totalpages;
        seq_printf(m, "%lu\n", points);
  
        return 0;
@@@ -1961,12 -1980,9 +1979,12 @@@ static int map_files_d_revalidate(struc
                goto out;
  
        if (!dname_to_vma_addr(dentry, &vm_start, &vm_end)) {
 -              down_read(&mm->mmap_sem);
 -              exact_vma_exists = !!find_exact_vma(mm, vm_start, vm_end);
 -              up_read(&mm->mmap_sem);
 +              status = down_read_killable(&mm->mmap_sem);
 +              if (!status) {
 +                      exact_vma_exists = !!find_exact_vma(mm, vm_start,
 +                                                          vm_end);
 +                      up_read(&mm->mmap_sem);
 +              }
        }
  
        mmput(mm);
@@@ -2012,11 -2028,8 +2030,11 @@@ static int map_files_get_link(struct de
        if (rc)
                goto out_mmput;
  
 +      rc = down_read_killable(&mm->mmap_sem);
 +      if (rc)
 +              goto out_mmput;
 +
        rc = -ENOENT;
 -      down_read(&mm->mmap_sem);
        vma = find_exact_vma(mm, vm_start, vm_end);
        if (vma && vma->vm_file) {
                *path = vma->vm_file->f_path;
@@@ -2112,11 -2125,7 +2130,11 @@@ static struct dentry *proc_map_files_lo
        if (!mm)
                goto out_put_task;
  
 -      down_read(&mm->mmap_sem);
 +      result = ERR_PTR(-EINTR);
 +      if (down_read_killable(&mm->mmap_sem))
 +              goto out_put_mm;
 +
 +      result = ERR_PTR(-ENOENT);
        vma = find_exact_vma(mm, vm_start, vm_end);
        if (!vma)
                goto out_no_vma;
  
  out_no_vma:
        up_read(&mm->mmap_sem);
 +out_put_mm:
        mmput(mm);
  out_put_task:
        put_task_struct(task);
@@@ -2170,12 -2178,7 +2188,12 @@@ proc_map_files_readdir(struct file *fil
        mm = get_task_mm(task);
        if (!mm)
                goto out_put_task;
 -      down_read(&mm->mmap_sem);
 +
 +      ret = down_read_killable(&mm->mmap_sem);
 +      if (ret) {
 +              mmput(mm);
 +              goto out_put_task;
 +      }
  
        nr_files = 0;
  
@@@ -3076,9 -3079,6 +3094,9 @@@ static const struct pid_entry tgid_base
  #ifdef CONFIG_STACKLEAK_METRICS
        ONE("stack_depth", S_IRUGO, proc_stack_depth),
  #endif
 +#ifdef CONFIG_PROC_PID_ARCH_STATUS
 +      ONE("arch_status", S_IRUGO, proc_pid_arch_status),
 +#endif
  };
  
  static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
@@@ -3466,9 -3466,6 +3484,9 @@@ static const struct pid_entry tid_base_
  #ifdef CONFIG_LIVEPATCH
        ONE("patch_state",  S_IRUSR, proc_pid_patch_state),
  #endif
 +#ifdef CONFIG_PROC_PID_ARCH_STATUS
 +      ONE("arch_status", S_IRUGO, proc_pid_arch_status),
 +#endif
  };
  
  static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx)