From: Kees Cook <firstname.lastname@example.org> To: Jann Horn <email@example.com> Cc: Alexey Dobriyan <firstname.lastname@example.org>, Ken Chen <email@example.com>, kernel list <firstname.lastname@example.org>, "email@example.com" <firstname.lastname@example.org>, Will Deacon <email@example.com>, Laura Abbott <firstname.lastname@example.org>, Andy Lutomirski <email@example.com>, Security Officers <firstname.lastname@example.org>, Catalin Marinas <email@example.com>, Josh Poimboeuf <firstname.lastname@example.org>, Thomas Gleixner <email@example.com>, Ingo Molnar <firstname.lastname@example.org>, "H . Peter Anvin" <email@example.com>, Linux API <firstname.lastname@example.org> Subject: Re: [PATCH] proc: restrict kernel stack dumps to root Date: Thu, 13 Sep 2018 07:39:43 -0700 [thread overview] Message-ID: <CAGXu5jLLC6dp7T5AmKur0dioc767vvJFms8wj9syLBvQzcYQpg@mail.gmail.com> (raw) In-Reply-To: <CAG48ez22djpKdtYDTMn0MS=2m_QAq983O0-oXyVGiu3ju2HwXg@mail.gmail.com> On Thu, Sep 13, 2018 at 4:55 AM, Jann Horn <email@example.com> wrote: > On Thu, Sep 13, 2018 at 12:28 AM Kees Cook <firstname.lastname@example.org> wrote: >> >> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <email@example.com> wrote: >> > +linux-api, I guess >> > >> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <firstname.lastname@example.org> wrote: >> >> >> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root >> >> in order to prevent a local attacker from exploiting racy stack unwinding >> >> to leak kernel task stack contents. >> >> See the added comment for a longer rationale. >> >> >> >> There don't seem to be any users of this userspace API that can't >> >> gracefully bail out if reading from the file fails. Therefore, I believe >> >> that this change is unlikely to break things. >> >> In the case that this patch does end up needing a revert, the next-best >> >> solution might be to fake a single-entry stack based on wchan. >> >> >> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack") >> >> Cc: email@example.com >> >> Signed-off-by: Jann Horn <firstname.lastname@example.org> >> >> --- >> >> fs/proc/base.c | 14 ++++++++++++++ >> >> 1 file changed, 14 insertions(+) >> >> >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> >> index ccf86f16d9f0..7e9f07bf260d 100644 >> >> --- a/fs/proc/base.c >> >> +++ b/fs/proc/base.c >> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns, >> >> unsigned long *entries; >> >> int err; >> >> >> >> + /* >> >> + * The ability to racily run the kernel stack unwinder on a running task >> >> + * and then observe the unwinder output is scary; while it is useful for >> >> + * debugging kernel issues, it can also allow an attacker to leak kernel >> >> + * stack contents. >> >> + * Doing this in a manner that is at least safe from races would require >> >> + * some work to ensure that the remote task can not be scheduled; and >> >> + * even then, this would still expose the unwinder as local attack >> >> + * surface. >> >> + * Therefore, this interface is restricted to root. >> >> + */ >> >> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN)) >> >> + return -EACCES; >> >> In the past, we've avoided hard errors like this in favor of just >> censoring the output. Do we want to be more cautious here? (i.e. >> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;) > > In my mind, this is different because it's a place where we don't have > to selectively censor output while preserving parts of it, and it's a > place where, as Laura said, it's useful to make lack of privileges > clearly visible because that informs users that they may have to retry > with more privileges. > > Of course, if you have an example of software that actually breaks due > to this, I'll change it. But I looked at the three things in Debian > codesearch that seem to use it, and from what I can tell, they all > bail out cleanly when the read fails. I prefer -EACCESS too, but I thought I'd mention the alternative. So, I guess: Reviewed-by: Kees Cook <email@example.com> :) -Kees -- Kees Cook Pixel Security
next prev parent reply other threads:[~2018-09-13 19:49 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-09-11 18:39 Jann Horn 2018-09-12 15:29 ` Jann Horn 2018-09-12 22:27 ` Kees Cook 2018-09-12 22:47 ` Laura Abbott 2018-09-13 11:55 ` Jann Horn 2018-09-13 14:39 ` Kees Cook [this message] 2018-09-27 1:19 ` Jann Horn 2018-09-27 2:03 ` Kees Cook
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAGXu5jLLC6dp7T5AmKur0dioc767vvJFms8wj9syLBvQzcYQpg@mail.gmail.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH] proc: restrict kernel stack dumps to root' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
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).