[ Apparently this thread wasn't on the lists, and I didn't even notice. So re-sending the patches ]
On Sat, Jul 13, 2019 at 9:16 PM Eric W. Biederman <ebied...@xmission.com> wrote: > Given that this fixes a regression and a bug. > > Acked-by: "Eric W. Biederman" <ebied...@xmission.com> I'd much rather start from scratch. Like the attached. Alexey Izbyshev has a third patch that then limits the setproctitle() case to only allow looking into the env[] area, which looks reasonable. Linus
From 5563a2fb39fe0ad42f239d2f583128d50155002b Mon Sep 17 00:00:00 2001 From: Linus Torvalds <torva...@linux-foundation.org> Date: Sat, 13 Jul 2019 13:40:13 -0700 Subject: [PATCH 1/2] /proc/<pid>/cmdline: remove all the special cases Start off with a clean slate that only reads exactly from arg_start to arg_end, without any oddities. We'll add back the setproctitle() special case very differently in the next commit. Signed-off-by: Linus Torvalds <torva...@linux-foundation.org> --- fs/proc/base.c | 71 ++++++-------------------------------------------- 1 file changed, 8 insertions(+), 63 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 255f6754c70d..8040f9d1cf07 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -212,7 +212,7 @@ static int proc_root_link(struct dentry *dentry, struct path *path) 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 arg_start, arg_end; unsigned long pos, len; char *page; @@ -223,36 +223,18 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf, spin_lock(&mm->arg_lock); arg_start = mm->arg_start; arg_end = mm->arg_end; - env_start = mm->env_start; - env_end = mm->env_end; spin_unlock(&mm->arg_lock); if (arg_start >= arg_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. - */ - if (env_start != arg_end || env_start >= env_end) - 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; - /* 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 = arg_start + *ppos; + if (pos < arg_start || pos >= arg_end) return 0; - - /* .. and we never go past env_end */ - if (env_end - pos < count) - count = env_end - pos; + if (count > arg_end - pos) + count = arg_end - pos; page = (char *)__get_free_page(GFP_KERNEL); if (!page) @@ -262,48 +244,11 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf, 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; -- 2.22.0.193.g083935f9a2
From 63dd14999c6b210fbe33f780fec53faefa867d95 Mon Sep 17 00:00:00 2001 From: Linus Torvalds <torva...@linux-foundation.org> Date: Sat, 13 Jul 2019 14:27:14 -0700 Subject: [PATCH 2/2] /proc/<pid>/cmdline: add back the setproctitle() special case This makes the setproctitle() special case very explicit indeed, and handles it with a separate helper function entirely. This makes the logic about when we use the string lengths etc much more obvious, and makes it easy to see what we do. [ Fixed for missing 'count' check noted by Alexey Izbyshev ] Signed-off-by: Linus Torvalds <torva...@linux-foundation.org> --- fs/proc/base.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 8040f9d1cf07..3ad3ff4cc12c 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -209,12 +209,54 @@ static int proc_root_link(struct dentry *dentry, struct path *path) 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, loff_t *ppos, + unsigned long arg_start) +{ + unsigned long pos = *ppos; + 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; unsigned long pos, len; - char *page; + char *page, c; /* Check if process spawned far enough to have cmdline. */ if (!mm->env_end) @@ -228,6 +270,16 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf, if (arg_start >= arg_end) 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, ppos, arg_start); + /* We're not going to care if "*ppos" has high bits set */ /* .. but we do check the result is in the proper range */ pos = arg_start + *ppos; -- 2.22.0.193.g083935f9a2