From: Dan Rosenberg <drosenberg@vsecurity.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
jmorris@namei.org, eugeneteo@kernel.org, kees.cook@canonical.com,
mingo@elte.hu, davem@davemloft.net
Subject: Re: [PATCH v2] kptr_restrict for hiding kernel pointers from unprivileged users
Date: Fri, 17 Dec 2010 20:12:39 -0500 [thread overview]
Message-ID: <1292634759.9764.26.camel@Dan> (raw)
In-Reply-To: <20101217164431.08f3e730.akpm@linux-foundation.org>
> The changelog doesn't describe why CONFIG_SECURITY_KPTR_RESTRICT
> exists, nor why the kptr_restrict sysctl exists. I can kinda guess why
> this was done, but it would be much better if your reasoning was
> present here.
>
> And I'd question whether we need CONFIG_SECURITY_KPTR_RESTRICT at all.
> Disabling it saves no memory. Its presence just increases the level of
> incompatibility between different vendor's kernels and potentially
> doubles the number of kernels which distros must ship (which they of
> course won't do). It might be better to add a kptr_restrict=1 kernel boot
> option (although people sometimes have problems with boot options in
> embedded environments).
>
> All that being said, distro initscripts can just set the sysctl to the
> desired value before any non-root process has even started, but this
> apparently is far too hard for them :(
>
> Finally, the changelog and the documentation changes don't tell us the
> full /proc path to the kptr_restrict pseudo-file. That would be useful
> info. Seems that it's /proc/sys/kernel/kptr_restrict?
>
I'll send a clean-up patch tomorrow fixing the documentation issues.
I'm also willing to take more feedback on the need for a config - this
was the approach that was recommended to me recently with
dmesg_restrict, but I also understand your reasoning.
> >
> > ...
> >
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -936,6 +936,8 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
> > return string(buf, end, uuid, spec);
> > }
> >
> > +int kptr_restrict = CONFIG_SECURITY_KPTR_RESTRICT;
> > +
> > /*
> > * Show a '%p' thing. A kernel extension is that the '%p' is followed
> > * by an extra set of alphanumeric characters that are extended format
> > @@ -979,6 +981,7 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
> > * Implements a "recursive vsnprintf".
> > * Do not use this feature without some mechanism to verify the
> > * correctness of the format string and va_list arguments.
> > + * - 'K' For a kernel pointer that should be hidden from unprivileged users
> > *
> > * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
> > * function pointers are really function descriptors, which contain a
> > @@ -1035,6 +1038,21 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> > return buf + vsnprintf(buf, end - buf,
> > ((struct va_format *)ptr)->fmt,
> > *(((struct va_format *)ptr)->va));
> > + case 'K':
> > + if (kptr_restrict) {
> > + if (in_irq() || in_serving_softirq() || in_nmi())
> > + WARN(1, "%%pK used in interrupt context.\n");
> > +
> > + else if (capable(CAP_SYSLOG))
> > + break;
>
> And the reason why it's unusable in interrupt context is that we can't
> meaningfully check CAP_SYSLOG from interrupt.
>
> Fair enough, but this does restrict %pK's usefulness.
>
> I think I'd be more comfortable with a WARN_ONCE here. If someone
> screws up then we don't want to spew thousands of repeated warnings at
> our poor users - one will do.
>
>
Agreed.
>
> So what's next? We need to convert 1,000,000 %p callsites to use %pK?
> That'll be fun. Please consider adding a new checkpatch rule which
> detects %p and asks people whether they should have used %pK.
The goal of this format specifier is specifically for pointers that are
exposed to unprivileged users. I agree that hiding all kernel pointers
would be nice, but I don't expect the angry masses to ever agree to
that. For now, I'll isolate specific cases, especially in /proc, that
are clear risks in terms of information leakage. I'll also be skipping
over pointers written to the syslog, since I think hiding that
information is dmesg_restrict's job.
Thanks,
Dan
next prev parent reply other threads:[~2010-12-18 1:12 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-12-11 0:05 [PATCH v2] kptr_restrict for hiding kernel pointers from unprivileged users Dan Rosenberg
2010-12-11 0:11 ` Kees Cook
2010-12-18 0:44 ` Andrew Morton
2010-12-18 1:12 ` Dan Rosenberg [this message]
2010-12-18 1:22 ` Andrew Morton
2010-12-18 5:22 ` Dan Rosenberg
2010-12-18 0:53 ` Andrew Morton
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=1292634759.9764.26.camel@Dan \
--to=drosenberg@vsecurity.com \
--cc=akpm@linux-foundation.org \
--cc=davem@davemloft.net \
--cc=eugeneteo@kernel.org \
--cc=jmorris@namei.org \
--cc=kees.cook@canonical.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=netdev@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).