From: William Roberts <[email protected]>
Some out-of-tree modules do not use %pK and just use %p, as it's
the common C paradigm for printing pointers. Because of this,
kptr_restrict has no affect on the output and thus, no way to
contain the kernel address leak.
Introduce kptr_restrict level 3 that causes the kernel to
treat %p as if it was %pK and thus always prints zeros.
Sample Output:
kptr_restrict == 2:
p: 00000000604369f4
pK: 0000000000000000
kptr_restrict == 3:
p: 0000000000000000
pK: 0000000000000000
Signed-off-by: William Roberts <[email protected]>
---
Documentation/sysctl/kernel.txt | 3 ++
kernel/sysctl.c | 3 +-
lib/vsprintf.c | 107 ++++++++++++++++++++++++----------------
3 files changed, 69 insertions(+), 44 deletions(-)
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index ffab8b5..bca72a0 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -393,6 +393,9 @@ values to unprivileged users is a concern.
When kptr_restrict is set to (2), kernel pointers printed using
%pK will be replaced with 0's regardless of privileges.
+When kptr_restrict is set to (3), kernel pointers printed using
+%p and %pK will be replaced with 0's regardless of privileges.
+
==============================================================
kstack_depth_to_print: (X86 only)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index a377bfa..0d4e4af 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -128,6 +128,7 @@ static unsigned long one_ul = 1;
static int one_hundred = 100;
static int one_thousand = 1000;
#ifdef CONFIG_PRINTK
+static int three = 3;
static int ten_thousand = 10000;
#endif
#ifdef CONFIG_PERF_EVENTS
@@ -847,7 +848,7 @@ static struct ctl_table kern_table[] = {
.mode = 0644,
.proc_handler = proc_dointvec_minmax_sysadmin,
.extra1 = &zero,
- .extra2 = &two,
+ .extra2 = &three,
},
#endif
{
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 0967771..371cfab 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1472,6 +1472,56 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
int kptr_restrict __read_mostly;
+static inline void *cleanse_pointer(void *ptr, struct printf_spec spec,
+ char *buf, char *end, int default_width)
+{
+ switch (kptr_restrict) {
+ case 0:
+ /* Always print %p values */
+ break;
+ case 1: {
+ const struct cred *cred;
+
+ /*
+ * kptr_restrict==1 cannot be used in IRQ context
+ * because its test for CAP_SYSLOG would be meaningless.
+ */
+ if (in_irq() || in_serving_softirq() || in_nmi()) {
+ if (spec.field_width == -1)
+ spec.field_width = default_width;
+ return string(buf, end, "pK-error", spec);
+ }
+
+ /*
+ * Only print the real pointer value if the current
+ * process has CAP_SYSLOG and is running with the
+ * same credentials it started with. This is because
+ * access to files is checked at open() time, but %p
+ * checks permission at read() time. We don't want to
+ * leak pointer values if a binary opens a file using
+ * %pK and then elevates privileges before reading it.
+ */
+ cred = current_cred();
+ if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+ !uid_eq(cred->euid, cred->uid) ||
+ !gid_eq(cred->egid, cred->gid))
+ ptr = NULL;
+ break;
+ }
+ case 2: /* restrict only %pK */
+ case 3: /* restrict all non-extensioned %p and %pK */
+ default:
+ ptr = NULL;
+ break;
+ }
+ return ptr;
+}
+
+static inline int kptr_restrict_always_cleanse(void)
+{
+ return kptr_restrict == 3;
+}
+
/*
* Show a '%p' thing. A kernel extension is that the '%p' is followed
* by an extra set of alphanumeric characters that are extended format
@@ -1569,6 +1619,9 @@ int kptr_restrict __read_mostly;
* Note: The difference between 'S' and 'F' is that on ia64 and ppc64
* function pointers are really function descriptors, which contain a
* pointer to the real address.
+ *
+ * Note: That for kptr_restrict set to 3, %p and %pK have the same
+ * meaning.
*/
static noinline_for_stack
char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -1576,7 +1629,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
{
const int default_width = 2 * sizeof(void *);
- if (!ptr && *fmt != 'K') {
+ if (!ptr && *fmt != 'K' && !kptr_restrict_always_cleanse()) {
/*
* Print (null) with the same width as a pointer so it makes
* tabular output look nice.
@@ -1657,48 +1710,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
va_end(va);
return buf;
}
- case 'K':
- switch (kptr_restrict) {
- case 0:
- /* Always print %pK values */
- break;
- case 1: {
- const struct cred *cred;
-
- /*
- * kptr_restrict==1 cannot be used in IRQ context
- * because its test for CAP_SYSLOG would be meaningless.
- */
- if (in_irq() || in_serving_softirq() || in_nmi()) {
- if (spec.field_width == -1)
- spec.field_width = default_width;
- return string(buf, end, "pK-error", spec);
- }
-
- /*
- * Only print the real pointer value if the current
- * process has CAP_SYSLOG and is running with the
- * same credentials it started with. This is because
- * access to files is checked at open() time, but %pK
- * checks permission at read() time. We don't want to
- * leak pointer values if a binary opens a file using
- * %pK and then elevates privileges before reading it.
- */
- cred = current_cred();
- if (!has_capability_noaudit(current, CAP_SYSLOG) ||
- !uid_eq(cred->euid, cred->uid) ||
- !gid_eq(cred->egid, cred->gid))
- ptr = NULL;
- break;
- }
- case 2:
- default:
- /* Always print 0's for %pK */
- ptr = NULL;
- break;
- }
- break;
-
case 'N':
return netdev_bits(buf, end, ptr, fmt);
case 'a':
@@ -1718,6 +1729,16 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
case 'G':
return flags_string(buf, end, ptr, fmt);
+ default:
+ /*
+ * plain %p, no extension, check if we should always cleanse and
+ * treat like %pK.
+ */
+ if (!kptr_restrict_always_cleanse())
+ break;
+ case 'K':
+ /* always check whether or not to cleanse kernel addresses */
+ ptr = cleanse_pointer(ptr, spec, buf, end, default_width);
}
spec.flags |= SMALL;
if (spec.field_width == -1) {
--
1.9.1
On Wed, Oct 5, 2016 at 11:04 AM, <[email protected]> wrote:
> From: William Roberts <[email protected]>
>
> Some out-of-tree modules do not use %pK and just use %p, as it's
> the common C paradigm for printing pointers. Because of this,
> kptr_restrict has no affect on the output and thus, no way to
> contain the kernel address leak.
Solving this is certainly a good idea -- I'm all for finding a solid solution.
> Introduce kptr_restrict level 3 that causes the kernel to
> treat %p as if it was %pK and thus always prints zeros.
I'm worried that this could break kernel internals where %p is being
used and not exposed to userspace. Maybe those situations don't
exist...
Regardless, I would rather do what Grsecurity has done in this area,
and whitelist known-safe values instead. For example, they have %pP
for approved pointers, and %pX for approved
dereference_function_descriptor() output. Everything else is censored
if it is a value in kernel memory and destined for a user-space memory
buffer:
if ((unsigned long)ptr > TASK_SIZE && *fmt != 'P' && *fmt !=
'X' && *fmt != 'K' && is_usercopy_object(buf)) {
printk(KERN_ALERT "grsec: kernel infoleak detected!
Please report this log to [email protected].\n");
dump_stack();
ptr = NULL;
}
The "is_usercopy_object()" test is something we can add, which is
testing for a new SLAB flag that is used to mark slab caches as either
used by user-space or not, which is done also through whitelisting.
(For more details on this, see:
http://www.openwall.com/lists/kernel-hardening/2016/06/08/10)
Would you have time/interest to add the slab flags and
is_usercopy_object()? The hardened usercopy part of the slab
whitelisting can be separate, since it likely needs a different
usercopy interface to sanely integrate with upstream.
-Kees
--
Kees Cook
Nexus Security
On Wed, Oct 05 2016, [email protected] wrote:
> From: William Roberts <[email protected]>
>
> Some out-of-tree modules do not use %pK and just use %p, as it's
> the common C paradigm for printing pointers. Because of this,
> kptr_restrict has no affect on the output and thus, no way to
> contain the kernel address leak.
>
> Introduce kptr_restrict level 3 that causes the kernel to
> treat %p as if it was %pK and thus always prints zeros.
>
> Sample Output:
> kptr_restrict == 2:
> p: 00000000604369f4
> pK: 0000000000000000
>
> kptr_restrict == 3:
> p: 0000000000000000
> pK: 0000000000000000
>
> Signed-off-by: William Roberts <[email protected]>
> ---
> Documentation/sysctl/kernel.txt | 3 ++
> kernel/sysctl.c | 3 +-
> lib/vsprintf.c | 107 ++++++++++++++++++++++++----------------
That's a lot of changed lines. Why isn't this just
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1719,6 +1719,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
case 'G':
return flags_string(buf, end, ptr, fmt);
}
+ if (kptr_restrict == 3)
+ ptr = NULL;
spec.flags |= SMALL;
if (spec.field_width == -1) {
spec.field_width = default_width;
?
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Kees
> Cook
> Sent: Wednesday, October 5, 2016 3:34 PM
> To: Roberts, William C <[email protected]>
> Cc: [email protected]; Jonathan Corbet <[email protected]>;
> [email protected]; LKML <[email protected]>; Nick
> Desaulniers <[email protected]>; Dave Weinstein <[email protected]>
> Subject: Re: [PATCH] printk: introduce kptr_restrict level 3
>
> On Wed, Oct 5, 2016 at 11:04 AM, <[email protected]> wrote:
> > From: William Roberts <[email protected]>
> >
> > Some out-of-tree modules do not use %pK and just use %p, as it's the
> > common C paradigm for printing pointers. Because of this,
> > kptr_restrict has no affect on the output and thus, no way to contain
> > the kernel address leak.
>
> Solving this is certainly a good idea -- I'm all for finding a solid solution.
>
> > Introduce kptr_restrict level 3 that causes the kernel to treat %p as
> > if it was %pK and thus always prints zeros.
>
> I'm worried that this could break kernel internals where %p is being used and not
> exposed to userspace. Maybe those situations don't exist...
Not saying they don't I didn't find any.
>
> Regardless, I would rather do what Grsecurity has done in this area, and whitelist
> known-safe values instead. For example, they have %pP for approved pointers,
> and %pX for approved
> dereference_function_descriptor() output. Everything else is censored if it is a
> value in kernel memory and destined for a user-space memory
> buffer:
>
> if ((unsigned long)ptr > TASK_SIZE && *fmt != 'P' && *fmt != 'X' && *fmt !=
> 'K' && is_usercopy_object(buf)) {
> printk(KERN_ALERT "grsec: kernel infoleak detected!
> Please report this log to [email protected].\n");
> dump_stack();
> ptr = NULL;
> }
>
> The "is_usercopy_object()" test is something we can add, which is testing for a
> new SLAB flag that is used to mark slab caches as either used by user-space or
> not, which is done also through whitelisting.
> (For more details on this, see:
> http://www.openwall.com/lists/kernel-hardening/2016/06/08/10)
>
> Would you have time/interest to add the slab flags and is_usercopy_object()?
> The hardened usercopy part of the slab whitelisting can be separate, since it likely
> needs a different usercopy interface to sanely integrate with upstream.
I could likely take this on. I would need to read up on the links and have a better concept
of what it is.
>
> -Kees
>
> --
> Kees Cook
> Nexus Security
> -----Original Message-----
> From: Rasmus Villemoes [mailto:[email protected]]
> Sent: Wednesday, October 5, 2016 4:53 PM
> To: Roberts, William C <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH] printk: introduce kptr_restrict level 3
>
> On Wed, Oct 05 2016, [email protected] wrote:
>
> > From: William Roberts <[email protected]>
> >
> > Some out-of-tree modules do not use %pK and just use %p, as it's the
> > common C paradigm for printing pointers. Because of this,
> > kptr_restrict has no affect on the output and thus, no way to contain
> > the kernel address leak.
> >
> > Introduce kptr_restrict level 3 that causes the kernel to treat %p as
> > if it was %pK and thus always prints zeros.
> >
> > Sample Output:
> > kptr_restrict == 2:
> > p: 00000000604369f4
> > pK: 0000000000000000
> >
> > kptr_restrict == 3:
> > p: 0000000000000000
> > pK: 0000000000000000
> >
> > Signed-off-by: William Roberts <[email protected]>
> > ---
> > Documentation/sysctl/kernel.txt | 3 ++
> > kernel/sysctl.c | 3 +-
> > lib/vsprintf.c | 107 ++++++++++++++++++++++++----------------
>
> That's a lot of changed lines. Why isn't this just
I moved the nested case into a static local function, I thought it was easier to read than the existing
nested switches. The other reason was so we didn't have kptr_restrict littering that code
and it was contained within the default and K values of the switch.
>
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1719,6 +1719,8 @@ char *pointer(const char *fmt, char *buf, char *end,
> void *ptr,
> case 'G':
> return flags_string(buf, end, ptr, fmt);
> }
> + if (kptr_restrict == 3)
> + ptr = NULL;
> spec.flags |= SMALL;
> if (spec.field_width == -1) {
> spec.field_width = default_width;
>
> ?
On Wed, Oct 05, 2016 at 02:04:46PM -0400, [email protected] wrote:
> From: William Roberts <[email protected]>
>
> Some out-of-tree modules do not use %pK and just use %p, as it's
> the common C paradigm for printing pointers. Because of this,
> kptr_restrict has no affect on the output and thus, no way to
> contain the kernel address leak.
So what? We a) don't care about out of tree modules and b) you could
just triviall fix them up if you care.
No need to bloat the kernel with crap like this.
> -----Original Message-----
> From: Christoph Hellwig [mailto:[email protected]]
> Sent: Thursday, October 6, 2016 9:32 AM
> To: Roberts, William C <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH] printk: introduce kptr_restrict level 3
>
> On Wed, Oct 05, 2016 at 02:04:46PM -0400, [email protected] wrote:
> > From: William Roberts <[email protected]>
> >
> > Some out-of-tree modules do not use %pK and just use %p, as it's the
> > common C paradigm for printing pointers. Because of this,
> > kptr_restrict has no affect on the output and thus, no way to contain
> > the kernel address leak.
>
> So what? We a) don't care about out of tree modules and b) you could just triviall
> fix them up if you care.
Out of tree modules still affect core kernel security. I would also bet money, that somewhere
In-tree someone has put a %p when they wanted a %pK. So this method is just quite error
prone. We currently have a blacklist approach versus whitelist.
>
> No need to bloat the kernel with crap like this.
It's unconstructive comments like this that do the whole community harm. Notice how
responses from Kees Cook were aimed at finding a different solution to the problem and were
very constructive. As far as "bloating" goes, it really didn't change a whole lot, most of it was moved
lines, and adds maybe a few lines of code.
On Thu, Oct 06, 2016 at 01:47:47PM +0000, Roberts, William C wrote:
> Out of tree modules still affect core kernel security.
So don't use them.
> I would also bet money, that somewhere
> In-tree someone has put a %p when they wanted a %pK.
So fix them.
> So this method is just quite error
> prone. We currently have a blacklist approach versus whitelist.
Or fix the entire thing, get rid of %pK and always protect %p if you
can show that it doesn't break anything.
But stop posting patches with bullshit arguments like out of tree
modules.
On Thu, Oct 06, 2016 at 01:47:47PM +0000, Roberts, William C wrote:
> > -----Original Message-----
> > From: Christoph Hellwig [mailto:[email protected]]
> > Sent: Thursday, October 6, 2016 9:32 AM
> > To: Roberts, William C <[email protected]>
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]; [email protected]
> > Subject: Re: [PATCH] printk: introduce kptr_restrict level 3
> >
> > On Wed, Oct 05, 2016 at 02:04:46PM -0400, [email protected] wrote:
> > > From: William Roberts <[email protected]>
> > >
> > > Some out-of-tree modules do not use %pK and just use %p, as it's the
> > > common C paradigm for printing pointers. Because of this,
> > > kptr_restrict has no affect on the output and thus, no way to contain
> > > the kernel address leak.
> >
> > So what? We a) don't care about out of tree modules and b) you could just triviall
> > fix them up if you care.
>
> Out of tree modules still affect core kernel security. I would also bet money, that somewhere
> In-tree someone has put a %p when they wanted a %pK. So this method is just quite error
> prone. We currently have a blacklist approach versus whitelist.
grep says you have a point:
$ grep -IR 'seq_printf.*%p[^FfSsBRrhbMmIiEUVKNadCDgG].*&'
drivers/dma/qcom/hidma_dbg.c: seq_printf(s, "dev_trca=%p\n", &dmadev->dev_trca);
drivers/dma/qcom/hidma_dbg.c: seq_printf(s, "dev_evca=%p\n", &dmadev->dev_evca);
$ grep -IR 'pr_info.*%p[^FfSsBRrhbMmIiEUVKNadCDgG].*&'
drivers/misc/lkdtm_heap.c: pr_info("Allocated memory %p-%p\n", base, &base[offset * 2]);
$ grep -IR 'pr_err.*%p[^FfSsBRrhbMmIiEUVKNadCDgG].*&'
drivers/net/ethernet/qlogic/qlge/qlge_dbg.c: pr_err("rx_ring->cqicb = %p\n", &rx_ring->cqicb);
And these are just trivially greppable, low-hanging-fruit ones.
With somewhat broader greps, there seem to be lots more, but they'd
require manual review.
And in total, there are 13578 matches for %p[^FfSsBRrhbMmIiEUVKNadCDgG]
throughout the kernel. Reviewing all of those manually would suck.
On Thu, Oct 06, 2016 at 04:05:53PM +0200, Jann Horn wrote:
> On Thu, Oct 06, 2016 at 01:47:47PM +0000, Roberts, William C wrote:
> > > -----Original Message-----
> > > From: Christoph Hellwig [mailto:[email protected]]
> > > Sent: Thursday, October 6, 2016 9:32 AM
> > > To: Roberts, William C <[email protected]>
> > > Cc: [email protected]; [email protected]; linux-
> > > [email protected]; [email protected]
> > > Subject: Re: [PATCH] printk: introduce kptr_restrict level 3
> > >
> > > On Wed, Oct 05, 2016 at 02:04:46PM -0400, [email protected] wrote:
> > > > From: William Roberts <[email protected]>
> > > >
> > > > Some out-of-tree modules do not use %pK and just use %p, as it's the
> > > > common C paradigm for printing pointers. Because of this,
> > > > kptr_restrict has no affect on the output and thus, no way to contain
> > > > the kernel address leak.
> > >
> > > So what? We a) don't care about out of tree modules and b) you could just triviall
> > > fix them up if you care.
> >
> > Out of tree modules still affect core kernel security. I would also bet money, that somewhere
> > In-tree someone has put a %p when they wanted a %pK. So this method is just quite error
> > prone. We currently have a blacklist approach versus whitelist.
>
> grep says you have a point:
>
> $ grep -IR 'seq_printf.*%p[^FfSsBRrhbMmIiEUVKNadCDgG].*&'
> drivers/dma/qcom/hidma_dbg.c: seq_printf(s, "dev_trca=%p\n", &dmadev->dev_trca);
> drivers/dma/qcom/hidma_dbg.c: seq_printf(s, "dev_evca=%p\n", &dmadev->dev_evca);
>
> $ grep -IR 'pr_info.*%p[^FfSsBRrhbMmIiEUVKNadCDgG].*&'
> drivers/misc/lkdtm_heap.c: pr_info("Allocated memory %p-%p\n", base, &base[offset * 2]);
>
> $ grep -IR 'pr_err.*%p[^FfSsBRrhbMmIiEUVKNadCDgG].*&'
> drivers/net/ethernet/qlogic/qlge/qlge_dbg.c: pr_err("rx_ring->cqicb = %p\n", &rx_ring->cqicb);
>
> And these are just trivially greppable, low-hanging-fruit ones.
> With somewhat broader greps, there seem to be lots more, but they'd
> require manual review.
(Although, of course, most matches for seq_printf are in debugfs
files or stuff that's only enabled with some DEBUG config option
or so. But there are also e.g. pr_warn() users of %p that are not
in debug code.)
> -----Original Message-----
> From: Christoph Hellwig [mailto:[email protected]]
> Sent: Thursday, October 6, 2016 9:56 AM
> To: Roberts, William C <[email protected]>
> Cc: Christoph Hellwig <[email protected]>; kernel-
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH] printk: introduce kptr_restrict level 3
>
> On Thu, Oct 06, 2016 at 01:47:47PM +0000, Roberts, William C wrote:
> > Out of tree modules still affect core kernel security.
>
> So don't use them.
>
> > I would also bet money, that somewhere In-tree someone has put a %p
> > when they wanted a %pK.
>
> So fix them.
As Jann Horn points out, "And in total, there are 13578 matches for %p[^FfSsBRrhbMmIiEUVKNadCDgG] throughout the kernel. Reviewing all of those manually would suck."
>
> > So this method is just quite error
> > prone. We currently have a blacklist approach versus whitelist.
>
> Or fix the entire thing, get rid of %pK and always protect %p if you can show that
> it doesn't break anything.
>
> But stop posting patches with bullshit arguments like out of tree modules.
Ok perhaps the commit message sucks, and I should have included the large spread usages of %p throughout
the kernel, I assumed those would just be known, I shouldn't have made that assumption.
We should care about out-of-tree modules wrt security as they affect the security of the whole system, especially when the
modules are linking to core symbols like printing and string routines. There are tons of %p usages throughout the
kernel as noted above.
This is pretty low hanging fruit and we should fix this, as Kees points out.
On Thu, Oct 6, 2016 at 6:56 AM, Christoph Hellwig <[email protected]> wrote:
> On Thu, Oct 06, 2016 at 01:47:47PM +0000, Roberts, William C wrote:
> > On Thu, Oct 6, 2016 at 6:31 AM, Christoph Hellwig <[email protected]> wrote:
> > > So what? We a) don't care about out of tree modules and b) you could just triviall
> > > fix them up if you care.
> >
> > Out of tree modules still affect core kernel security.
>
> So don't use them.
I'm really interested in keeping these discussions productive.
Declaring that the upstream kernel community (your implied "we")
doesn't care about out-of-tree code is both false and short-sighted.
Huge numbers of people depend on Linux on their phones, their IoT
devices, cars, etc, and huge volumes of those (unfortunately) are
running out-of-tree code.
This isn't about supporting some special interface that a single third
party driver uses; this is about robust engineering and security
practices that benefits everything, even out-of-tree code.
>> I would also bet money, that somewhere
>> In-tree someone has put a %p when they wanted a %pK.
>
> So fix them.
%pK is an ugly situation that I'd like to fix correctly. It was
designed originally as a blacklisting method, when it should have been
designed as a whitelist (as William mentions). This is what will need
fixing (and is what I was suggesting in my original reply to the
patch).
All that said, "so fix them" sounds very much like "just fix the bugs"
which is another troublesome attitude to have for dealing with
potential security flaws. The kernel community already finds/fixes
obvious flaws; the efforts with changes like what William is proposing
are based around assuming (correctly, I can argue) that there are
already bugs present, and they will remain in the kernel for years to
come. Designing the kernel to safely deal with bugs being present is a
fundamental design principle for kernel security self-defense
technologies.
The challenge is not "fix them", the challenge is "design the kernel
to do the right thing even when the bugs are unfixed".
>> So this method is just quite error
>> prone. We currently have a blacklist approach versus whitelist.
>
> Or fix the entire thing, get rid of %pK and always protect %p if you
> can show that it doesn't break anything.
>
> But stop posting patches with bullshit arguments like out of tree
> modules.
As a _singlular_ argument, "it's for out-of-tree code" is weak. As an
_additional_ argument, it has value. Saying "this only helps
out-of-tree code" doesn't carry much weight. Saying "this helps kernel
security, even for out-of-tree code" is perfectly valid. And a wrinkle
in this is that some day, either that out-of-tree code, or brand new
code, will land in the kernel, and we don't want to continue to
require authors be aware of an opt-in security feature. The kernel
should protect itself (and all of itself, including out-of-tree or
future code) by default.
And based on my read of this thread, we all appear to be in violent
agreement. :) "always protect %p" is absolutely the goal, and we can
figure out the best way to get there.
-Kees
--
Kees Cook
Nexus Security
On Thu, 2016-10-06 at 14:00 -0700, Kees Cook wrote:
> And based on my read of this thread, we all appear to be in violent
> agreement. :) "always protect %p" is absolutely the goal, and we can
> figure out the best way to get there.
I proposed emitting pointers from the const and text sections by default
and using NULL for data pointers.
https://lkml.org/lkml/2016/8/5/380
On Thu, Oct 6, 2016 at 2:19 PM, Joe Perches <[email protected]> wrote:
> On Thu, 2016-10-06 at 14:00 -0700, Kees Cook wrote:
>
>> And based on my read of this thread, we all appear to be in violent
>> agreement. :) "always protect %p" is absolutely the goal, and we can
>> figure out the best way to get there.
>
> I proposed emitting pointers from the const and text sections by default
> and using NULL for data pointers.
>
> https://lkml.org/lkml/2016/8/5/380
Leaks of const and text (while not useful for write-attacks) can leak
KASLR offset (though yes, yes, there are many existing leaks -- but we
should avoid adding a new one regardless).
I think the logic of "is this destined for userspace" is likely the
cleanest approach. There still may be many things this breaks, though.
(I expect perf. Everything breaks perf. ;)
-Kees
--
Kees Cook
Nexus Security
On Thu, Oct 06, 2016 at 04:05:53PM +0200, Jann Horn wrote:
> On Thu, Oct 06, 2016 at 01:47:47PM +0000, Roberts, William C wrote:
> > > -----Original Message-----
> > > From: Christoph Hellwig [mailto:[email protected]]
> > > Sent: Thursday, October 6, 2016 9:32 AM
> > > To: Roberts, William C <[email protected]>
> > > Cc: [email protected]; [email protected]; linux-
> > > [email protected]; [email protected]
> > > Subject: Re: [PATCH] printk: introduce kptr_restrict level 3
> > >
> > > On Wed, Oct 05, 2016 at 02:04:46PM -0400, [email protected] wrote:
> > > > From: William Roberts <[email protected]>
> > > >
> > > > Some out-of-tree modules do not use %pK and just use %p, as it's the
> > > > common C paradigm for printing pointers. Because of this,
> > > > kptr_restrict has no affect on the output and thus, no way to contain
> > > > the kernel address leak.
> > >
> > > So what? We a) don't care about out of tree modules and b) you could just triviall
> > > fix them up if you care.
> >
> > Out of tree modules still affect core kernel security. I would also bet money, that somewhere
> > In-tree someone has put a %p when they wanted a %pK. So this method is just quite error
> > prone. We currently have a blacklist approach versus whitelist.
>
> grep says you have a point:
>
> $ grep -IR 'seq_printf.*%p[^FfSsBRrhbMmIiEUVKNadCDgG].*&'
> drivers/dma/qcom/hidma_dbg.c: seq_printf(s, "dev_trca=%p\n", &dmadev->dev_trca);
> drivers/dma/qcom/hidma_dbg.c: seq_printf(s, "dev_evca=%p\n", &dmadev->dev_evca);
>
> $ grep -IR 'pr_info.*%p[^FfSsBRrhbMmIiEUVKNadCDgG].*&'
> drivers/misc/lkdtm_heap.c: pr_info("Allocated memory %p-%p\n", base, &base[offset * 2]);
>
> $ grep -IR 'pr_err.*%p[^FfSsBRrhbMmIiEUVKNadCDgG].*&'
> drivers/net/ethernet/qlogic/qlge/qlge_dbg.c: pr_err("rx_ring->cqicb = %p\n", &rx_ring->cqicb);
Actually, I think I missed something here.
We don't really want to censor pointers in dmesg, right? dmesg can
contain pointers by design - and in particular, when an oops
happens, the register dump will reveal pointers. dmesg should just
be restricted using dmesg_restrict.
(What is annoying, though, is that e.g. Debian's default rsyslog
config sends all KERN_EMERG messages to all active PTYs by default,
meaning that on a system with such a config, making the kernel oops
can be used to leak some ASLR information.)
(And why does __die() in arch/x86/kernel/dumpstack.c use KERN_EMERG
for CONFIG_X86_32, but KERN_ALERT for !CONFIG_X86_32?)
<snip>
>
> As a _singlular_ argument, "it's for out-of-tree code" is weak. As an _additional_
> argument, it has value. Saying "this only helps out-of-tree code" doesn't carry
> much weight. Saying "this helps kernel security, even for out-of-tree code" is
> perfectly valid. And a wrinkle in this is that some day, either that out-of-tree
> code, or brand new code, will land in the kernel, and we don't want to continue
> to require authors be aware of an opt-in security feature. The kernel should
> protect itself (and all of itself, including out-of-tree or future code) by default.
>
I should have made this more clear in my message, this was in my head and I assumed
that people would just get it. But I shouldn't have made such an assumption.
> And based on my read of this thread, we all appear to be in violent agreement. :)
> "always protect %p" is absolutely the goal, and we can figure out the best way to
> get there.
>
> -Kees
>
> --
> Kees Cook
> Nexus Security