Re: [PATCH] staging: ccree: Fix bool comparison

[Tobin C. Harding]

On Mon, Oct 16, 2017 at 03:38:11PM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> Bool tests don't need comparisons.

This commit log could be a bit longer. You may like to read
Documentation/process/submitting-patches.rst (section 2).

> This fixes the following coccinelle warning:
> WARNING: Comparison of bool to 0/1
> 
> Signed-off-by: Suniel Mahesh 
> ---
> Note:
> - Patch was tested and built(ARCH=arm) on latest
>   linux-next.

Building is _not_ testing.

> - No build issues reported, however it was not
>   tested on real hardware.

This is implicit if you state 'builds on ARM'

> - Please discard this changeset, if this is not
>   helping the code look better.

This is implicit also ;)

> ---
>  drivers/staging/ccree/ssi_request_mgr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_request_mgr.c 
> b/drivers/staging/ccree/ssi_request_mgr.c
> index 2e0df57..942afe2 100644
> --- a/drivers/staging/ccree/ssi_request_mgr.c
> +++ b/drivers/staging/ccree/ssi_request_mgr.c
> @@ -272,7 +272,7 @@ int send_request(
>   unsigned int max_required_seq_len = (total_seq_len +
>   ((ssi_req->ivgen_dma_addr_len == 0) ? 0 
> :
>   SSI_IVPOOL_SEQ_LEN) +
> - ((is_dout == 0) ? 1 : 0));
> + (!is_dout ? 1 : 0));

Perhaps

-   ((is_dout == 0) ? 1 : 0));
+   (is_dout ? 0 : 1)

Good luck,
Tobin.

Re: [PATCH] staging: ccree: fix boolreturn.cocci warning

[Tobin C. Harding]

On Mon, Oct 16, 2017 at 03:39:57PM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> This fixes the following coccinelle warning:
> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.

Perhaps

Coccinelle emits WARNING: return of 0/1 in function 'ssi_is_hw_key' with return 
type bool.

Return 'false' instead of 0.

> Signed-off-by: Suniel Mahesh 
> ---
> Note:
> - Patch was tested and built(ARCH=arm) on latest
>   linux-next.
> - No build issues reported, however it was not
>   tested on real hardware.
> - Please discard this changeset, if this is not
>   helping the code look better.
> ---
>  drivers/staging/ccree/ssi_cipher.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_cipher.h 
> b/drivers/staging/ccree/ssi_cipher.h
> index c9a83df..f499962 100644
> --- a/drivers/staging/ccree/ssi_cipher.h
> +++ b/drivers/staging/ccree/ssi_cipher.h
> @@ -75,7 +75,7 @@ struct arm_hw_key_info {
>  
>  static inline bool ssi_is_hw_key(struct crypto_tfm *tfm)
>  {
> - return 0;
> + return false;
>  }

Hope this helps,
Tobin.

[PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as follows

git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

arch: 2512
block: 20
crypto: 12
fs: 1221
include: 147
kernel: 109
lib: 77
mm: 120
net: 1516
security: 11
sound: 168
virt: 2
drivers: 8420

Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
address to a 32 bit unique identifier.

Signed-off-by: Tobin C. Harding 
---

V2:
 - Use SipHash to do the hashing

The discussion related to this patch has been fragmented. There are
three other threads associated with this patch. Email threads by
subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

 include/linux/siphash.h |  2 ++
 lib/siphash.c   | 13 +
 lib/vsprintf.c  | 32 +---
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/include/linux/siphash.h b/include/linux/siphash.h
index fa7a6b9cedbf..a9392568c8b8 100644
--- a/include/linux/siphash.h
+++ b/include/linux/siphash.h
@@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const 
siphash_key_t *key);
 u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t 
*key);
 #endif
 
+unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t *key);
+
 u64 siphash_1u64(const u64 a, const siphash_key_t *key);
 u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
 u64 siphash_3u64(const u64 a, const u64 b, const u64 c,
diff --git a/lib/siphash.c b/lib/siphash.c
index 3ae58b4edad6..63f4ff57c9ce 100644
--- a/lib/siphash.c
+++ b/lib/siphash.c
@@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
 #endif
 
 /**
+ * siphash_1ulong - computes siphash PRF value
+ * @first: value to hash
+ * @key: the siphash key
+ */
+unsigned long siphash_1ulong(const unsigned long first, const siphash_key_t 
*key)
+{
+#ifdef CONFIG_64BIT
+   return (unsigned long)siphash_1u64((u64)first, key);
+#endif
+   return (unsigned long)siphash_1u32((u32)first, key);
+}
+
+/**
  * siphash_1u64 - compute 64-bit siphash PRF value of a u64
  * @first: first u64
  * @key: the siphash key
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..afd1c835b0f6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #ifdef CONFIG_BLOCK
 #include 
 #endif
@@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long num,
*buf = '0';
++buf;
}
+
/* actual digits of result */
while (--i >= 0) {
if (buf < end)
@@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct 
device_node *dn,
return widen_string(buf, buf - buf_start, end, spec);
 }
 
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec 
spec)
+{
+   static siphash_key_t ptr_secret __read_mostly;
+   static bool have_key = false;
+   unsigned long hashval;
+
+   /* Kernel doesn't boot if we use get_random_once() */
+   if (!have_key) {
+   get_random_bytes(&ptr_secret, sizeof(ptr_secret));
+   have_key = true;
+   }
+
+   hashval = siphash_1ulong((unsigned long)ptr, &ptr_secret);
+
+   spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
+   spec.flags = SPECIAL | SMALL | ZEROPAD;
+   spec.base = 16;
+
+   return number(buf, end, (u32)hashval, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,6 +1727,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.
+ *
+ * Default behaviour (unadorned %p) is to hash the address, rendering it useful
+ * as a unique identifier.
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -1858,14 +1885,13 @@ char *pointer(const char *fmt, char *buf, char *end, 
void *ptr,
return device_node_string(buf, end, ptr, spec, fmt + 1);
}
}
-   spec.flags |= SMALL;
+
if (spec.field_width == -1) {
spec.field_width = default_width;
spec.flags |= ZEROPAD;
}
-   spec.base =

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:27:15PM +, Roberts, William C wrote:
> 
> 
> > -Original Message-
> > From: Tobin C. Harding [mailto:m...@tobin.cc]
> > Sent: Monday, October 16, 2017 9:53 PM
> > To: kernel-harden...@lists.openwall.com
> > Cc: Tobin C. Harding ; Linus Torvalds  > foundation.org>; Kees Cook ; Paolo Bonzini
> > ; Tycho Andersen ; Roberts,
> > William C ; Tejun Heo ; Jordan
> > Glover ; Greg KH
> > ; Petr Mladek ; Joe
> > Perches ; Ian Campbell ; Sergey
> > Senozhatsky ; Catalin Marinas
> > ; Will Deacon ; Steven
> > Rostedt ; Chris Fries ; Dave
> > Weinstein ; Daniel Micay ; Djalal
> > Harouni ; linux-kernel@vger.kernel.org
> > Subject: [PATCH v2] printk: hash addresses printed with %p
> > 
> > Currently there are many places in the kernel where addresses are being 
> > printed
> > using an unadorned %p. Kernel pointers should be printed using %pK allowing
> > some control via the kptr_restrict sysctl. Exposing addresses gives 
> > attackers
> > sensitive information about the kernel layout in memory.
> > 
> > We can reduce the attack surface by hashing all addresses printed with %p. 
> > This
> > will of course break some users, forcing code printing needed addresses to 
> > be
> > updated.
> > 
> > For what it's worth, usage of unadorned %p can be broken down as follows
> > 
> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> > 
> > arch: 2512
> > block: 20
> > crypto: 12
> > fs: 1221
> > include: 147
> > kernel: 109
> > lib: 77
> > mm: 120
> > net: 1516
> > security: 11
> > sound: 168
> > virt: 2
> > drivers: 8420
> > 
> > Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> > address to a 32 bit unique identifier.
> > 
> > Signed-off-by: Tobin C. Harding 
> > ---
> > 
> > V2:
> >  - Use SipHash to do the hashing
> > 
> > The discussion related to this patch has been fragmented. There are three 
> > other
> > threads associated with this patch. Email threads by
> > subject:
> > 
> > [PATCH] printk: hash addresses printed with %p [PATCH 0/3] add %pX specifier
> > [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
> > 
> >  include/linux/siphash.h |  2 ++
> >  lib/siphash.c   | 13 +
> >  lib/vsprintf.c  | 32 +---
> >  3 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/siphash.h b/include/linux/siphash.h index
> > fa7a6b9cedbf..a9392568c8b8 100644
> > --- a/include/linux/siphash.h
> > +++ b/include/linux/siphash.h
> > @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const
> > siphash_key_t *key);
> >  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t
> > *key);  #endif
> > 
> > +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t
> > +*key);
> > +
> >  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
> >  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
> >  u64 siphash_3u64(const u64 a, const u64 b, const u64 c, diff --git 
> > a/lib/siphash.c
> > b/lib/siphash.c index 3ae58b4edad6..63f4ff57c9ce 100644
> > --- a/lib/siphash.c
> > +++ b/lib/siphash.c
> > @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
> >  #endif
> > 
> >  /**
> > + * siphash_1ulong - computes siphash PRF value
> > + * @first: value to hash
> > + * @key: the siphash key
> > + */
> > +unsigned long siphash_1ulong(const unsigned long first, const
> > +siphash_key_t *key) { #ifdef CONFIG_64BIT
> > +   return (unsigned long)siphash_1u64((u64)first, key); #endif
> > +   return (unsigned long)siphash_1u32((u32)first, key); }
> > +
> > +/**
> >   * siphash_1u64 - compute 64-bit siphash PRF value of a u64
> >   * @first: first u64
> >   * @key: the siphash key
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 
> > 86c3385b9eb3..afd1c835b0f6 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -33,6 +33,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #ifdef CONFIG_BLOCK
> >  #include 
> >  #endif
> > @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long
> > num,
> > *buf = '0';
> > ++buf;
> > }
> > +
> 
> Unneeded whitespac

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> On Tue, 17 Oct 2017 15:52:51 +1100
> "Tobin C. Harding"  wrote:
> 
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> > 
> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> > 
> > For what it's worth, usage of unadorned %p can be broken down as follows
> > 
> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> 
> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> found in kallsyms, but otherwise you have:
> 
>   function+0x

You are correct %pF and %pS print an offset. Does this provide an attack vector,
I didn't think so but I'm no security expert. If they do then we need to amend
those calls also.

We still have %pa[pd] to see to as well obviously.

thanks for the review,
Tobin.

[PATCH v3] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as
follows (thanks to Joe Perches).

$ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
   1084 arch
 20 block
 10 crypto
 32 Documentation
   8121 drivers
   1221 fs
143 include
101 kernel
 69 lib
100 mm
   1510 net
 40 samples
  7 scripts
 11 security
166 sound
152 tools
  2 virt

Add function ptr_to_id() to map an address to a 32 bit unique identifier.

Signed-off-by: Tobin C. Harding 
---

V3:
 - Use atomic_xchg() to guard setting [random] key.
 - Remove erroneous white space change.

V2:
 - Use SipHash to do the hashing.

The discussion related to this patch has been fragmented. There are
three threads associated with this patch. Email threads by subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

 include/linux/siphash.h |  2 ++
 lib/siphash.c   | 13 +
 lib/vsprintf.c  | 28 +---
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/include/linux/siphash.h b/include/linux/siphash.h
index fa7a6b9cedbf..a9392568c8b8 100644
--- a/include/linux/siphash.h
+++ b/include/linux/siphash.h
@@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const 
siphash_key_t *key);
 u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t 
*key);
 #endif
 
+unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t *key);
+
 u64 siphash_1u64(const u64 a, const siphash_key_t *key);
 u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
 u64 siphash_3u64(const u64 a, const u64 b, const u64 c,
diff --git a/lib/siphash.c b/lib/siphash.c
index 3ae58b4edad6..63f4ff57c9ce 100644
--- a/lib/siphash.c
+++ b/lib/siphash.c
@@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
 #endif
 
 /**
+ * siphash_1ulong - computes siphash PRF value
+ * @first: value to hash
+ * @key: the siphash key
+ */
+unsigned long siphash_1ulong(const unsigned long first, const siphash_key_t 
*key)
+{
+#ifdef CONFIG_64BIT
+   return (unsigned long)siphash_1u64((u64)first, key);
+#endif
+   return (unsigned long)siphash_1u32((u32)first, key);
+}
+
+/**
  * siphash_1u64 - compute 64-bit siphash PRF value of a u64
  * @first: first u64
  * @key: the siphash key
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..b3b680357a85 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #ifdef CONFIG_BLOCK
 #include 
 #endif
@@ -1591,6 +1592,25 @@ char *device_node_string(char *buf, char *end, struct 
device_node *dn,
return widen_string(buf, buf - buf_start, end, spec);
 }
 
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec 
spec)
+{
+   static siphash_key_t ptr_secret __read_mostly;
+   static atomic_t have_key = ATOMIC_INIT(0);
+   unsigned long hashval;
+
+if (atomic_xchg(&have_key, 1) == 0)
+   get_random_bytes(&ptr_secret, sizeof(ptr_secret));
+
+   hashval = siphash_1ulong((unsigned long)ptr, &ptr_secret);
+
+   spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
+   spec.flags = SPECIAL | SMALL | ZEROPAD;
+   spec.base = 16;
+
+   return number(buf, end, (u32)hashval, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,6 +1723,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.
+ *
+ * Default behaviour (unadorned %p) is to hash the address, rendering it useful
+ * as a unique identifier.
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -1858,14 +1881,13 @@ char *pointer(const char *fmt, char *buf, char *end, 
void *ptr,
return device_node_string(buf, end, ptr, spec, fmt + 1);
}
}
-   spec.flags |= SMALL;
+
if (spec.field_width == -1) {
spec.field_width = default_width;
spec.flags |= ZEROPAD;
}
-   spec.base = 16;
 
-   return number(buf, end, (unsigned long) ptr, spec);
+   return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*
-- 
2.7.4

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 07:08:48PM -0700, Joe Perches wrote:
> On Tue, 2017-10-31 at 09:33 +1100, Tobin C. Harding wrote:
> > On Mon, Oct 30, 2017 at 03:03:21PM -0700, Kees Cook wrote:
> > > On Wed, Oct 25, 2017 at 7:53 PM, Tobin C. Harding  wrote:
> > > > Here is the behaviour that this set implements.
> > > > 
> > > > For kpt_restrict==0
> > > > 
> > > > Randomness not ready:
> > > >   printed with %p:  (pointer)  # NOTE: with padding
> > > > Valid pointer:
> > > >   printed with %pK: deadbeefdeadbeef
> > > >   printed with %p:  0xdeadbeef
> > > >   malformed specifier (eg %i):  0xdeadbeef
> > > 
> > > I really think we can't include SPECIAL unless _every_ callsite of %p
> > > is actually doing "0x%p", and then we're replacing all of those. We're
> > > not doing that, though...
> > > 
> > > $ git grep '%p\b' | wc -l
> > > 12766
> > > $ git grep '0x%p\b' | wc -l
> > > 18370x
> > > 
> > > If we need some kind of special marking that this is a hashed
> > > variable, that should be something other than "0x". If we're using the
> > > existing "(null)" and new "(pointer)" text, maybe "(hash:xx)"
> > > should be used instead? Then the (rare) callers with 0x become
> > > "0x(hash:)" and naked callers produce "(hash:)".
> > > 
> > > I think the first step for this is to just leave SPECIAL out.
> > 
> > Thanks Kees. V9 leaves SPECIAL out. Also V9 prints the whole 64 bit
> > address with the first 32 bits masked to zero. The intent being to _not_
> > change the output format from what it currently is. So it will look like
> > this; 
> > 
> > c09e81d0
> > 
> > What do you think?
> > 
> > Amusingly I think this whole conversation is going to come up again
> > when we do %pa, in inverse, since %pa currently does us SPECIAL.
> 
> I once sent a patch set to remove SPECIAL from %pa
> and add 0x where necessary.
> 
> https://patchwork.kernel.org/patch/3875471/
> 
> After that didn't happen, I removed the duplicated
> 0x%pa with a sed.
> 
> https://patchwork.kernel.org/patch/8509421/
> 
> Sending a treewide sed patch would be fine with me.

Cool, thanks Joe I'll keep this in mind for when we get to %pa.

thanks,
Tobin.

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Fri, Oct 27, 2017 at 10:33:01PM +0900, Sergey Senozhatsky wrote:
> On (10/26/17 13:53), Tobin C. Harding wrote:
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing
> > addresses gives attackers sensitive information about the kernel layout
> > in memory.
> > 
> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> > 
> > With this version we include hashing of malformed specifiers also.
> > Malformed specifiers include incomplete (e.g %pi) and also non-existent
> > specifiers. checkpatch should warn for non-existent specifiers but
> > AFAICT won't warn for incomplete specifiers.
> > 
> > Here is the behaviour that this set implements.
> > 
> > For kpt_restrict==0
> > 
> > Randomness not ready:
> >   printed with %p:  (pointer)  # NOTE: with padding
> > Valid pointer:
> >   printed with %pK: deadbeefdeadbeef
> >   printed with %p:  0xdeadbeef
> >   malformed specifier (eg %i):  0xdeadbeef
> > NULL pointer:
> >   printed with %pK: 
> >   printed with %p:  (null)   # NOTE: no padding
> >   malformed specifier (eg %i):  (null)
> 
> a quick question:
>  do we care about cases when kernel pointers are printed with %x/%X and
>  not with %p?

Yes. The question has been raised will we be here again in 6 years time
trying to fix all the uses of %x. And there are already 29K uses of
%[xX] in tree, which of these are leaking addresses? This is why Linus'
has commented that really effort should be directed at finding the leaks
as they happen (in procfs, sysfs, dmesg) instead of fixing this in
the code. So far I haven't been able to come up with any meaningful way
to do this on 32 bit machines. There is a patch adding a script to catch
leaks on 64 bit machines in flight.

This patch needs to be a small part of a continued effort to stop the
leaks if we want to have any hope of stopping them.

If you have any suggestions on dealing with %x please do say. We have
code changes, compiler warnings, and checkpatch - none of which
immediately seem great.

thanks,
Tobin.

Re: [PATCH V9] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 31, 2017 at 04:39:44PM +0100, Petr Mladek wrote:
> On Mon 2017-10-30 09:59:16, Tobin C. Harding wrote:
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> > 
> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> 
> I am sorry for my ignorance but what is the right update, please?

Can I say first that I am in no way an expert, I am new to both this
problem and kernel dev in general.

> I expect that there are several possibilities:
> 
>   + remove the pointer at all

This definitely stops the leak!

>   + replace it with %pK so that it honors kptr_restrict setting

I think this is the option of choice, see concerns below however. I get
the feeling that the hope with this patch is that a vast majority of
users of %p won't care so stopping all those addresses is the real win
for this patch. 

The next hoped benefit is that the hashing will shed light on this topic
and get developers to think about the issue before _wildly_ printing
addresses. Having to work harder to print the address in future will aid
this (assuming everyone doesn't just start using %x).

>   + any other option?

Use %x or %X - really bad, this will create more pain in the future.

> Is kptr_restrict considered a safe mechanism?
> 
> Also kptr_restrict seems to be primary for the messages that are available
> via /proc and /sys. Is it good enough for the messages logged by
> printk()?

There is some concern that kptr_restrict is not overly great. Linus is
the main purveyor of this argument. I won't paraphrase here because I
will not do the argument justice.

See this thread for the whole discussion

[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

Side note: If this (mentioning a previous thread) is bad form, or there
is a better way to do it, could you please tell me.

> Will there be a debug option that would allow to see the original
> pointers? Or what is the preferred way for debug messages?

It seems to me that there is a fundamental conflict between security and
debugging here. To debug we need the addresses, for a secure kernel we
need there to be _no_ addresses shown to user space.

I don't think we have the final solution to this at the moment.

> > For what it's worth, usage of unadorned %p can be broken down as
> > follows (thanks to Joe Perches).
> > 
> > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
> >1084 arch
> >  20 block
> >  10 crypto
> >  32 Documentation
> >8121 drivers
> >1221 fs
> > 143 include
> > 101 kernel
> >  69 lib
> > 100 mm
> >1510 net
> >  40 samples
> >   7 scripts
> >  11 security
> > 166 sound
> > 152 tools
> >   2 virt
> 
> It is evident that it will hit many people. I guess that they will
> be suprised and might have similar questions. It might make sense
> to decribe this in Documentation/printk-formats.txt.

Very good idea, V10 to include.

thanks,
Tobin.

[PATCH V10 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing
addresses gives attackers sensitive information about the kernel layout
in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

This version adds testing, this is my first effort at kernel unit
testing. Modules in `lib` don't seem contained within a selftest target
so in order to incrementally develop the tests I implemented the tests
in `lib/test_printf.c`, built with `make M=lib` and then to insert the
module, instead of running selftest, I spun up a VM and inserted the
module manually. Comments or suggestions much appreciated.

Here is the behaviour that this series implements.

For kpt_restrict==0

Randomness not ready:
  printed with %p: (ptrval) # NOTE: with padding
Valid pointer:
  printed with %pK: deadbeefdeadbeef
  printed with %p:  deadbeef
  malformed specifier (eg %i):  deadbeef
NULL pointer:
  printed with %pK: 
  printed with %p:   (null) # NOTE: with padding
  malformed specifier (eg %i):   (null)

For kpt_restrict==2

Valid pointer:
  printed with %pK: 

All other output as for kptr_restrict==0

V10:
 - Add patch so KASAN uses %pK instead of %p. 
 - Add documentation to Documentation/printk-formats.txt
 - Add tests to lib/test_printf.c
 - Change "(pointer value)" -> "(ptrval)" to fit within columns on 32
   bit machines.

V9:
 - Drop the initial patch from V8, leaving null pointer handling as is.
 - Print the hashed ID _without_ a '0x' suffix.
 - Mask the first 32 bits of the hashed ID to all zeros on 64 bit
   architectures.

V8:
 - Add second patch cleaning up null pointer printing in pointer()
 - Move %pK handling to separate function, further cleaning up pointer()
 - Move ptr_to_id() call outside of switch statement making hashing
   the default behaviour (including malformed specifiers).
 - Remove use of static_key, replace with simple boolean.

V7:
 - Use tabs instead of spaces (ouch!).

V6:
 - Use __early_initcall() to fill the SipHash key.
 - Use static keys to guard hashing before the key is available.

V5:
 - Remove spin lock.
 - Add Jason A. Donenfeld to CC list by request.
 - Add Theodore Ts'o to CC list due to comment on previous version.

V4:
 - Remove changes to siphash.{ch}
 - Do word size check, and return value cast, directly in ptr_to_id().
 - Use add_ready_random_callback() to guard call to get_random_bytes()

V3:
 - Use atomic_xchg() to guard setting [random] key.
 - Remove erroneous white space change.

V2:
 - Use SipHash to do the hashing.

The discussion related to this patch has been fragmented. There are
three threads associated with this patch. Email threads by subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

Tobin C. Harding (2):
  kasan: use %pK to print addresses instead of %p
  printk: hash addresses printed with %p

 Documentation/printk-formats.txt |  17 +++-
 lib/test_printf.c| 108 +++-
 lib/vsprintf.c   | 176 ---
 mm/kasan/report.c|   8 +-
 4 files changed, 217 insertions(+), 92 deletions(-)

-- 
2.7.4

[PATCH V10 1/2] kasan: use %pK to print addresses instead of %p

[Tobin C. Harding]

In preparation for hashing addresses printed using %p. We need the
actual address for error reporting in kasan.

Use %pK instead of %p to print addresses.

Signed-off-by: Tobin C. Harding 
---
 mm/kasan/report.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 6bcfb01ba038..ad042f025a1a 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -134,7 +134,7 @@ static void print_error_description(struct 
kasan_access_info *info)
 
pr_err("BUG: KASAN: %s in %pS\n",
bug_type, (void *)info->ip);
-   pr_err("%s of size %zu at addr %p by task %s/%d\n",
+   pr_err("%s of size %zu at addr %pK by task %s/%d\n",
info->is_write ? "Write" : "Read", info->access_size,
info->access_addr, current->comm, task_pid_nr(current));
 }
@@ -206,7 +206,7 @@ static void describe_object_addr(struct kmem_cache *cache, 
void *object,
const char *rel_type;
int rel_bytes;
 
-   pr_err("The buggy address belongs to the object at %p\n"
+   pr_err("The buggy address belongs to the object at %pK\n"
   " which belongs to the cache %s of size %d\n",
object, cache->name, cache->object_size);
 
@@ -225,7 +225,7 @@ static void describe_object_addr(struct kmem_cache *cache, 
void *object,
}
 
pr_err("The buggy address is located %d bytes %s of\n"
-  " %d-byte region [%p, %p)\n",
+  " %d-byte region [%pK, %pK)\n",
rel_bytes, rel_type, cache->object_size, (void *)object_addr,
(void *)(object_addr + cache->object_size));
 }
@@ -302,7 +302,7 @@ static void print_shadow_for_address(const void *addr)
char shadow_buf[SHADOW_BYTES_PER_ROW];
 
snprintf(buffer, sizeof(buffer),
-   (i == 0) ? ">%p: " : " %p: ", kaddr);
+   (i == 0) ? ">%pK: " : " %pK: ", kaddr);
/*
 * We should not pass a shadow pointer to generic
 * function, because generic functions may try to
-- 
2.7.4

[PATCH V10 2/2] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as
follows (thanks to Joe Perches).

$ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
   1084 arch
 20 block
 10 crypto
 32 Documentation
   8121 drivers
   1221 fs
143 include
101 kernel
 69 lib
100 mm
   1510 net
 40 samples
  7 scripts
 11 security
166 sound
152 tools
  2 virt

Add function ptr_to_id() to map an address to a 32 bit unique
identifier. Hash any unadorned usage of specifier %p and any malformed
specifiers.

Signed-off-by: Tobin C. Harding 

---
 Documentation/printk-formats.txt |  17 +++-
 lib/test_printf.c| 108 +++-
 lib/vsprintf.c   | 176 ---
 3 files changed, 213 insertions(+), 88 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 361789df51ec..ec7deb80d035 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -5,6 +5,9 @@ How to get printk format specifiers right
 :Author: Randy Dunlap 
 :Author: Andrew Murray 
 
+Please do not print kernel addresses using %x. Exposing kernel addresses to
+user space leaks sensitive information that increases the attack surface of the
+kernel. In order to print pointers, please see 'Pointer Types' below.
 
 Integer types
 =
@@ -45,6 +48,18 @@ return from vsnprintf.
 Raw pointer value SHOULD be printed with %p. The kernel supports
 the following extended format specifiers for pointer types:
 
+Pointer Types
+=
+
+Pointers printed without a specifier extension (i.e unadorned %p) are hashed
+to give a unique identifier without leaking kernel addresses to user space.
+If you _really_ want to see the address please use %pK (see 'Kernel Pointers'
+below). On 64 bit machines the first 32 bits are zeroed.
+
+::
+
+   %p  abcdef12 or abcdef12
+
 Symbols/Function Pointers
 =
 
@@ -91,7 +106,7 @@ Kernel Pointers
 
 ::
 
-   %pK 0x01234567 or 0x0123456789abcdef
+   %pK 01234567 or 0123456789abcdef
 
 For printing kernel pointers which should be hidden from unprivileged
 users. The behaviour of ``%pK`` depends on the ``kptr_restrict sysctl`` - see
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 563f10e6876a..71ebfa43ad05 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -24,24 +24,6 @@
 #define PAD_SIZE 16
 #define FILL_CHAR '$'
 
-#define PTR1 ((void*)0x01234567)
-#define PTR2 ((void*)(long)(int)0xfedcba98)
-
-#if BITS_PER_LONG == 64
-#define PTR1_ZEROES "0"
-#define PTR1_SPACES " "
-#define PTR1_STR "1234567"
-#define PTR2_STR "fedcba98"
-#define PTR_WIDTH 16
-#else
-#define PTR1_ZEROES "0"
-#define PTR1_SPACES " "
-#define PTR1_STR "1234567"
-#define PTR2_STR "fedcba98"
-#define PTR_WIDTH 8
-#endif
-#define PTR_WIDTH_STR stringify(PTR_WIDTH)
-
 static unsigned total_tests __initdata;
 static unsigned failed_tests __initdata;
 static char *test_buffer __initdata;
@@ -217,30 +199,79 @@ test_string(void)
test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
 }
 
+#define PLAIN_BUF_SIZE 64  /* leave some space so we don't oops */
+
+#if BITS_PER_LONG == 64
+
+#define PTR_WIDTH 16
+#define PTR ((void *)0x0123456789ab)
+#define PTR_STR "0123456789ab"
+#define ZEROS ""   /* hex 32 zero bits */
+
+static int __init
+plain_format(void)
+{
+   char buf[PLAIN_BUF_SIZE];
+   int nchars;
+
+   nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+
+   if (nchars != PTR_WIDTH || strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
+   return -1;
+
+   return 0;
+}
+
+#else
+
+#define PTR_WIDTH 8
+#define PTR ((void *)0x456789ab)
+#define PTR_STR "456789ab"
+
+static int __init
+plain_format(void)
+{
+   /* Format is implicitly tested for 32 bit machines by plain_hash() */
+   return 0;
+}
+
+#endif /* BITS_PER_LONG == 64 */
+
+static int __init
+plain_hash(void)
+{
+   char buf[PLAIN_BUF_SIZE];
+   int nchars;
+
+   nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+
+   if (nchars != PTR_WIDTH || strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
+   return -1;
+
+ 

[PATCH v2] scripts: add leaking_addresses.pl

[Tobin C. Harding]

Currently we are leaking addresses from the kernel to user space. This
script is an attempt to find those leakages. Script parses `dmesg`
output and /proc and /sys files for hex strings that look like kernel
addresses.

Only works for 64 bit kernels, the reason being that kernel addresses
on 64 bit kernels have '' as the leading bit pattern making greping
possible. On 32 kernels we don't have this luxury.

Exclude vsyscall addresses.
Suggested-by: Tim Starling 

Signed-off-by: Tobin C. Harding 

---

v2:
 - Add regex's to prevent false positives.
 - Clean up white space.

 MAINTAINERS  |   5 +
 scripts/leaking_addresses.pl | 297 +++
 2 files changed, 302 insertions(+)
 create mode 100755 scripts/leaking_addresses.pl

diff --git a/MAINTAINERS b/MAINTAINERS
index e652a3e2929d..c1ad6d133a57 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7745,6 +7745,11 @@ S:   Maintained
 F: Documentation/scsi/53c700.txt
 F: drivers/scsi/53c700*
 
+LEAKING_ADDRESSES
+M: Tobin C. Harding 
+S: Maintained
+F: scripts/leaking_addresses.pl
+
 LED SUBSYSTEM
 M: Richard Purdie 
 M: Jacek Anaszewski 
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
new file mode 100755
index ..9b00ce252e2d
--- /dev/null
+++ b/scripts/leaking_addresses.pl
@@ -0,0 +1,297 @@
+#!/usr/bin/env perl
+#
+# (c) 2017 Tobin C. Harding 
+# Licensed under the terms of the GNU GPL License version 2
+#
+# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
+#  - Scans dmesg output.
+#  - Walks directory tree and parses each file (for each directory in @DIRS).
+#
+# You can configure the behaviour of the script;
+#
+#  - By adding paths, for directories you do not want to walk;
+# absolute paths: @skip_walk_dirs_abs
+# directory names: @skip_walk_dirs_any
+#
+#  - By adding paths, for files you do not want to parse;
+# absolute paths: @skip_parse_files_abs
+# file names: @skip_parse_files_any
+#
+# The use of @skip_xxx_xxx_any causes files to be skipped where ever they 
occur.
+# For example adding 'fd' to @skip_walk_dirs_any causes the fd/ directory to be
+# skipped for all PID sub-directories of /proc
+#
+# The same thing can be achieved by passing command line options to --dont-walk
+# and --dont-parse. If absolute paths are supplied to these options they are
+# appended to the @skip_xxx_xxx_abs arrays. If file names are supplied to these
+# options, they are appended to the @skip_xxx_xxx_any arrays.
+#
+# Use --debug to output path before parsing, this is useful to find files that
+# cause the script to choke.
+#
+# You may like to set kptr_restrict=2 before running script
+# (see Documentation/sysctl/kernel.txt).
+
+use warnings;
+use strict;
+use POSIX;
+use File::Basename;
+use File::Spec;
+use Cwd 'abs_path';
+use Term::ANSIColor qw(:constants);
+use Getopt::Long qw(:config no_auto_abbrev);
+
+my $P = $0;
+my $V = '0.01';
+
+# Directories to scan.
+my @DIRS = ('/proc', '/sys');
+
+# Command line options.
+my $help = 0;
+my $debug = 0;
+my @dont_walk = ();
+my @dont_parse = ();
+
+# Do not parse these files (absolute path).
+my @skip_parse_files_abs = ('/proc/kmsg',
+   '/proc/kcore',
+   '/proc/fs/ext4/sdb1/mb_groups',
+   '/proc/1/fd/3',
+   '/sys/kernel/debug/tracing/trace_pipe',
+   '/sys/kernel/security/apparmor/revision');
+
+# Do not parse thes files under any subdirectory.
+my @skip_parse_files_any = ('0',
+   '1',
+   '2',
+   'pagemap',
+   'events',
+   'access',
+   'registers',
+   'snapshot_raw',
+   'trace_pipe_raw',
+   'ptmx',
+   'trace_pipe');
+
+# Do not walk these directories (absolute path).
+my @skip_walk_dirs_abs = ();
+
+# Do not walk these directories under any subdirectory.
+my @skip_walk_dirs_any = ('self',
+ 'thread-self',
+ 'cwd',
+ 'fd',
+ 'stderr',
+ 'stdin',
+ 'stdout');
+
+sub help
+{
+   my ($exitcode) = @_;
+
+   print << "EOM";
+Usage: $P [OPTIONS]
+Version: $V
+
+Options:
+
+   --dont-walk=  Don't walk tree starting at .
+   --dont-parse=Don't parse .
+   -d, --debugDisplay debugging output

Re: [PATCH v2] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Wed, Nov 01, 2017 at 09:48:19PM +1100, Tobin C. Harding wrote:
> Currently we are leaking addresses from the kernel to user space. This
> script is an attempt to find those leakages. Script parses `dmesg`
> output and /proc and /sys files for hex strings that look like kernel
> addresses.
> 
> Only works for 64 bit kernels, the reason being that kernel addresses
> on 64 bit kernels have '' as the leading bit pattern making greping
> possible. On 32 kernels we don't have this luxury.
> 
> Exclude vsyscall addresses.
> Suggested-by: Tim Starling 
> 
> Signed-off-by: Tobin C. Harding 
> 
> ---
> 
> v2:
>  - Add regex's to prevent false positives.
>  - Clean up white space.
> 
>  MAINTAINERS  |   5 +
>  scripts/leaking_addresses.pl | 297 
> +++
>  2 files changed, 302 insertions(+)
>  create mode 100755 scripts/leaking_addresses.pl
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e652a3e2929d..c1ad6d133a57 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7745,6 +7745,11 @@ S:     Maintained
>  F:   Documentation/scsi/53c700.txt
>  F:   drivers/scsi/53c700*
>  
> +LEAKING_ADDRESSES
> +M:   Tobin C. Harding 
> +S:   Maintained
> +F:   scripts/leaking_addresses.pl
> +
>  LED SUBSYSTEM
>  M:   Richard Purdie 
>  M:   Jacek Anaszewski 
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> new file mode 100755
> index 0000..9b00ce252e2d
> --- /dev/null
> +++ b/scripts/leaking_addresses.pl
> @@ -0,0 +1,297 @@
> +#!/usr/bin/env perl
> +#
> +# (c) 2017 Tobin C. Harding 
> +# Licensed under the terms of the GNU GPL License version 2
> +#
> +# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> +#  - Scans dmesg output.
> +#  - Walks directory tree and parses each file (for each directory in @DIRS).
> +#
> +# You can configure the behaviour of the script;
> +#
> +#  - By adding paths, for directories you do not want to walk;
> +# absolute paths: @skip_walk_dirs_abs
> +# directory names: @skip_walk_dirs_any
> +#
> +#  - By adding paths, for files you do not want to parse;
> +# absolute paths: @skip_parse_files_abs
> +# file names: @skip_parse_files_any
> +#
> +# The use of @skip_xxx_xxx_any causes files to be skipped where ever they 
> occur.
> +# For example adding 'fd' to @skip_walk_dirs_any causes the fd/ directory to 
> be
> +# skipped for all PID sub-directories of /proc
> +#
> +# The same thing can be achieved by passing command line options to 
> --dont-walk
> +# and --dont-parse. If absolute paths are supplied to these options they are
> +# appended to the @skip_xxx_xxx_abs arrays. If file names are supplied to 
> these
> +# options, they are appended to the @skip_xxx_xxx_any arrays.
> +#
> +# Use --debug to output path before parsing, this is useful to find files 
> that
> +# cause the script to choke.
> +#
> +# You may like to set kptr_restrict=2 before running script
> +# (see Documentation/sysctl/kernel.txt).
> +
> +use warnings;
> +use strict;
> +use POSIX;
> +use File::Basename;
> +use File::Spec;
> +use Cwd 'abs_path';
> +use Term::ANSIColor qw(:constants);
> +use Getopt::Long qw(:config no_auto_abbrev);
> +
> +my $P = $0;
> +my $V = '0.01';
> +
> +# Directories to scan.
> +my @DIRS = ('/proc', '/sys');
> +
> +# Command line options.
> +my $help = 0;
> +my $debug = 0;
> +my @dont_walk = ();
> +my @dont_parse = ();
> +
> +# Do not parse these files (absolute path).
> +my @skip_parse_files_abs = ('/proc/kmsg',
> + '/proc/kcore',
> + '/proc/fs/ext4/sdb1/mb_groups',
> + '/proc/1/fd/3',
> + '/sys/kernel/debug/tracing/trace_pipe',
> + '/sys/kernel/security/apparmor/revision');
> +
> +# Do not parse thes files under any subdirectory.
> +my @skip_parse_files_any = ('0',
> + '1',
> + '2',
> + 'pagemap',
> + 'events',
> + 'access',
> + 'registers',
> + 'snapshot_raw',
> + 'trace_pipe_raw',
> + 'ptmx',
> + 'trace_pipe');
> +
> +# Do not walk these directories (absolute path).
> +my @skip_walk_dirs_a

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Thu, Nov 02, 2017 at 05:23:44PM +0900, Sergey Senozhatsky wrote:
> On (11/01/17 10:35), Tobin C. Harding wrote:
> [..]
> > Yes. The question has been raised will we be here again in 6 years time
> > trying to fix all the uses of %x. And there are already 29K uses of
> > %[xX] in tree, which of these are leaking addresses? This is why Linus'
> > has commented that really effort should be directed at finding the leaks
> > as they happen (in procfs, sysfs, dmesg) instead of fixing this in
> > the code.
> 
> got it. thanks.
> 
> > So far I haven't been able to come up with any meaningful way
> > to do this on 32 bit machines. There is a patch adding a script to catch
> > leaks on 64 bit machines in flight.
> 
> who is expected to run that script?

If one person runs it and finds one leaking address, I'd say it wast
worth writing. If a bunch of people with different set ups run it and we
find a bunch of leaking addresses, WIN!

Your comment did give me the idea of adding some output to the command
offering an email address to send suspicious output for those who do not
wish to investigate it further. I can put my email address if there is
not a better option. 

> BTW, can BPF/eBPF printk addresses?

I know absolutely zero about BPF/eBPF. I guess now is a good time to learn.

> > This patch needs to be a small part of a continued effort to stop the
> > leaks if we want to have any hope of stopping them.
> > 
> > If you have any suggestions on dealing with %x please do say. We have
> > code changes, compiler warnings, and checkpatch - none of which
> > immediately seem great.
> 
> hm... just a huge pile of if's
> 
>   if (is_vmalloc_addr(addr))
>   do_hashing(addr);
>   else if (__module_address(addr))
>   do_hashing(addr);
>   else if (is_kernel(addr) || is_kernel_inittext(addr))
>   ...
> 
> but that's going to be really messy and "iffy".

This is the only suggestion we have so far.

thanks,
Tobin.

Re: [PATCH v7] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 24, 2017 at 01:25:52PM +0200, Jason A. Donenfeld wrote:
> On Tue, Oct 24, 2017 at 2:31 AM, Tobin C. Harding  wrote:
> > On Tue, Oct 24, 2017 at 01:00:03AM +0200, Jason A. Donenfeld wrote:
> >> Provided you've tested this and the static_key guard stuff actually
> >> works as intended,
> >
> > I tested by inserting a simple module that calls printf() with a bunch of
> > different specifiers. So it's tested but not stress tested. Some stress 
> > testing
> > might be nice, no ideas how to go about that though.
> 
> By the way, it occurred to me one thing you might want to verify
> closely is whether or not that callback block executes from process
> context and whether or not the static_key stuff sleeps. If both,
> there's a problem.

Ok, will do.

thanks,
Tobin.

Re: [PATCH v7] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 24, 2017 at 01:25:52PM +0200, Jason A. Donenfeld wrote:
> On Tue, Oct 24, 2017 at 2:31 AM, Tobin C. Harding  wrote:
> > On Tue, Oct 24, 2017 at 01:00:03AM +0200, Jason A. Donenfeld wrote:
> >> Provided you've tested this and the static_key guard stuff actually
> >> works as intended,
> >
> > I tested by inserting a simple module that calls printf() with a bunch of
> > different specifiers. So it's tested but not stress tested. Some stress 
> > testing
> > might be nice, no ideas how to go about that though.
> 
> By the way, it occurred to me one thing you might want to verify
> closely is whether or not that callback block executes from process
> context and whether or not the static_key stuff sleeps. If both,
> there's a problem.

TL;DR the static_key stuff can't sleep, the callback block _may_ execute
in process context.

By 'verify closely' I'm guessing you mean 'think really hard about it', is
this correct? Please note, I am not being facetious, I am genuinely
trying to understand the suggestion.I guessed this since writing code
to verify something true has the same flaws as writing unit tests to
verify that code has no bugs.

So if it is 'think really hard about it', then my 'thinking really hard'
will not be the same as a more experienced kernel hackers 'thinking
really hard'. If I outline the results of my 'think really hard' then
perhaps just a 'think a little bit' from a _real_ kernel hacker may
uncover any deficits.

First, the static_key stuff.

DEFINE_STATIC_KEY_TRUE(no_ptr_secret) : Doesn't sleep, just a
declaration. 

if (static_branch_unlikely(&no_ptr_secret)) {} : Doesn't sleep, just
some assembler to jump to returning true or false.

static_branch_disable(&no_ptr_secret) : Doesn't sleep, just atomic read
and set and maybe a WARN_ONCE.

Now for the 'executes from process context' stuff.

If the callback mechanism is utilized (i.e print before randomness is
ready) then the call back will be executed the next time the randomness
pool gets added to, if this is done in process context then it seems
feasible that the call back will execute in process context.

Also, I believe the initialization function (fill_random_ptr_key()) can
be called in process context. This thought experiment shows how. If all
uses of %p in the kernel were removed except a single one which is in a
function that is called for process context then the first time that
function is called the initialization function will be called, hence it
will be called in process context.

I'm not sure if I have done justice to your comment, if not please do
say so .

thanks,
Tobin.

Re: [PATCH v7] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 25, 2017 at 06:00:21AM +0200, Jason A. Donenfeld wrote:
> On Wed, Oct 25, 2017 at 5:49 AM, Tobin C. Harding  wrote:
> > static_branch_disable(&no_ptr_secret) : Doesn't sleep, just atomic read
> > and set and maybe a WARN_ONCE.
> 
> Are you sure about that? I just looked myself, and though there is a
> !HAVE_JUMP_LABEL ifdef that does what you described, there's also a
> HAVE_JUMP_LABEL that takes a mutex, which sleeps:
> 
> static_branch_disable
>   static_key_disable
> cpus_read_lock
>   percpu_down_read
> percpu_down_read_preempt_disable
>   might_sleep

Hilarious, the actual function name is 'might_sleep' and I missed it. I
love being wrong, it means I'm learning. Thanks for taking the time to
point this out.

> > Now for the 'executes from process context' stuff.
> 
> Er, sorry, I meant to write non-process context in my original
> message, which is generally where you're worried about sleeping.

Tomorrow I'm going to re-read 'sleeping' sections from ldd3 and Love.

> > If the callback mechanism is utilized (i.e print before randomness is
> > ready) then the call back will be executed the next time the randomness
> > pool gets added to
> 
> So it sounds to me like this might be called in non-process context.
> Disaster. I realize the static_key thing was my idea in the original
> email, so sorry for leading you astray.

You bastard.

> But moving to do this in
> early_initcall wound up fixing other issues too, so all and all a net
> good in going this direction.

I wanted to know how to do this since Linus said 'boot time variable' in
one of the first comments on this topic. So I'm super glad you pointed
it out.

> Two options: you stick with static_branch, because it's cool and speed
> is fun, and work around all of the above with a call to queue_work so
> that static_branch_enable is called only from process context.
> 
> Or, you give up on static_key, because it's not actually super
> necessary, and instead just use an atomic, and reason that using `if
> (unlikely(!atomic_read(&whatever)))` is probably good enough. In this
> option, the code would be pretty much the same as v7, except you'd
> s/static_branch/atomic_t/, and change the helpers, etc. This is
> probably the more reasonable way.

I'm going to sleep, then re-reading these bits.

thanks Jason, appreciate your input big time.

Cheers,
Tobin.

Re: [PATCH v7] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 25, 2017 at 09:02:34PM +0200, Rasmus Villemoes wrote:
> On 25 October 2017 at 01:57, Tobin C. Harding  wrote:
> > On Tue, Oct 24, 2017 at 09:25:20PM +0200, Rasmus Villemoes wrote:
> >>
> >> I haven't followed the discussion too closely, but has it been
> >> considered to exempt NULL from hashing?
> >
> [snip]
> >
> > The code in question is;
> >
> > static noinline_for_stack
> > char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >   struct printf_spec spec)
> > {
> > const int default_width = 2 * sizeof(void *);
> >
> > if (!ptr && *fmt != 'K') {
> > /*
> >  * Print (null) with the same width as a pointer so it makes
> >  * tabular output look nice.
> >  */
> > if (spec.field_width == -1)
> > spec.field_width = default_width;
> > return string(buf, end, "(null)", spec);
> > }
> 
> Ah, yes, I should have re-read the current code before commenting. So
> we're effectively already exempting NULL due to this early handling.
> Good, let's leave that.
> 
> Regarding the tabular output: Ignore it, it's completely irrelevant to
> the hardening efforts (good work, btw), and probably completely
> irrelevant period. If anything I'd say the comment and the attempted
> alignment should just be killed.

Righto, I'm happy with that. Will add to next version.

> > This check and print "(null)" is at the wrong level of abstraction. If we 
> > want tabular output to be
> > correct for _all_ pointer specifiers then spec.field_width (for NULL) 
> > should be set to match whatever
> > field_width is used in the associated output function. Removing the NULL 
> > check above would require
> > NULL checks adding to at least;
> >
> > resource_string()
> > bitmap_list_string()
> > bitmap_string()
> > mac_address_string()
> > ip4_addr_string()
> > ip4_addr_string_sa()
> > ip6_addr_string_sa()
> > uuid_string()
> > netdev_bits()
> > address_val()
> > dentry_name()
> > bdev_name()
> > ptr_to_id()
> 
> No, please don't. The NULL check makes perfect sense early in
> pointer(), because many of these handlers would dereference the
> pointer, and while it's probably a bug to pass NULL to say %pD, it's
> obviously better to print (null) than crash. Adding NULL checks to
> each of these is error-prone and lots of bloat for no real value
> (consider that many of these can produce lots of variants of output,
> and e.g. dotted-decimal ip addresses don't even have a well-defined
> width at all).
> 
> > My question is [snip] is it too trivial to matter?
> 
> Yes.

thanks,
Tobin.

Re: [PATCH v7] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 25, 2017 at 06:00:21AM +0200, Jason A. Donenfeld wrote:
> On Wed, Oct 25, 2017 at 5:49 AM, Tobin C. Harding  wrote:
> > static_branch_disable(&no_ptr_secret) : Doesn't sleep, just atomic read
> > and set and maybe a WARN_ONCE.
> 
> Are you sure about that? I just looked myself, and though there is a
> !HAVE_JUMP_LABEL ifdef that does what you described, there's also a
> HAVE_JUMP_LABEL that takes a mutex, which sleeps:
> 
> static_branch_disable
>   static_key_disable
> cpus_read_lock
>   percpu_down_read
> percpu_down_read_preempt_disable
>   might_sleep
> 
> > Now for the 'executes from process context' stuff.
> 
> Er, sorry, I meant to write non-process context in my original
> message, which is generally where you're worried about sleeping.
> 
> > If the callback mechanism is utilized (i.e print before randomness is
> > ready) then the call back will be executed the next time the randomness
> > pool gets added to
> 
> So it sounds to me like this might be called in non-process context.
> Disaster. I realize the static_key thing was my idea in the original
> email, so sorry for leading you astray. But moving to do this in
> early_initcall wound up fixing other issues too, so all and all a net
> good in going this direction.
> 
> Two options: you stick with static_branch, because it's cool and speed
> is fun, and work around all of the above with a call to queue_work so
> that static_branch_enable is called only from process context.

This definitely sounds more fun, the static_branch stuff is dead sexy.

> Or, you give up on static_key, because it's not actually super
> necessary, and instead just use an atomic, and reason that using `if
> (unlikely(!atomic_read(&whatever)))` is probably good enough. In this
> option, the code would be pretty much the same as v7, except you'd
> s/static_branch/atomic_t/, and change the helpers, etc. This is
> probably the more reasonable way.

How good is unlikely()?

It doesn't _feel_ right adding a check on every call to printk just to
check for a condition that was only true for the briefest time when the
kernel booted. But if unlikely() is good then I guess it doesn't hurt.

I'm leaning towards the option 1, but then all those text books I read
are telling me to implement the simplest solution first then if we need
to go faster implement the more complex solution.

This is a pretty airy fairy discussion now, but if you have an opinion
I'd love to hear it.

thanks,
Tobin.

Re: [PATCH v7] printk: hash addresses printed with %p

[Tobin C. Harding]

On Thu, Oct 26, 2017 at 12:59:08AM +0200, Jason A. Donenfeld wrote:
> On Thu, Oct 26, 2017 at 12:27 AM, Tobin C. Harding  wrote:
> > How good is unlikely()?
> 
> It places that branch way at the bottom of the function so that it's
> less likely to pollute the icache.
> 
> > It doesn't _feel_ right adding a check on every call to printk just to
> > check for a condition that was only true for the briefest time when the
> > kernel booted. But if unlikely() is good then I guess it doesn't hurt.
> >
> > I'm leaning towards the option 1, but then all those text books I read
> > are telling me to implement the simplest solution first then if we need
> > to go faster implement the more complex solution.
> >
> > This is a pretty airy fairy discussion now, but if you have an opinion
> > I'd love to hear it.
> 
> I don't think adding a single branch there really matters that much,
> considering how many random other branches there are all over the
> printk code. However, if you really want to optimize on the little
> bits, and sensibly don't want to go with the overcomplex
> workqueue-to-statickey thing, you could consider using a plain vanilla
> `bool has_gotten_random_ptr_secret` instead of using the atomic_t. The
> reason is that there's only ever one single writer, changing from a 0
> to a 1. Basically the only thing doing the atomic_t got you was a
> cache flush surrounding the read (and the write) so that assigning
> has_gotten_random_ptr_secret=true would take effect _immediately_.
> However, since you might not necessarily about that, going with a bool
> instead will save you an expensive cache flush, while potentially
> being a microsecond out of date the first time it's used. Seems like
> an okay trade off to me. (That kind of cache latency, also, is a few
> orders of magnitude better than using a work queue for the statickey
> stuff.)

Awesome. Patch to follow.

thanks,
Tobin.

[PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Tobin C. Harding]

Currently pointer() checks for a NULL pointer argument and then if so
attempts to print "(null)" with _some_ standard width. This width cannot
correctly be ascertained here because many of the printk specifiers
print pointers of varying widths.

Remove the attempt to print NULL pointers with a correct width.

Signed-off-by: Tobin C. Harding 
---
 lib/vsprintf.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..16a587aed40e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1710,15 +1710,8 @@ char *pointer(const char *fmt, char *buf, char *end, 
void *ptr,
 {
const int default_width = 2 * sizeof(void *);
 
-   if (!ptr && *fmt != 'K') {
-   /*
-* Print (null) with the same width as a pointer so it makes
-* tabular output look nice.
-*/
-   if (spec.field_width == -1)
-   spec.field_width = default_width;
+   if (!ptr && *fmt != 'K')
return string(buf, end, "(null)", spec);
-   }
 
switch (*fmt) {
case 'F':
-- 
2.7.4

[PATCH V8 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing
addresses gives attackers sensitive information about the kernel layout
in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

With this version we include hashing of malformed specifiers also.
Malformed specifiers include incomplete (e.g %pi) and also non-existent
specifiers. checkpatch should warn for non-existent specifiers but
AFAICT won't warn for incomplete specifiers.

Here is the behaviour that this set implements.

For kpt_restrict==0

Randomness not ready:
  printed with %p:  (pointer)  # NOTE: with padding
Valid pointer:
  printed with %pK: deadbeefdeadbeef
  printed with %p:  0xdeadbeef
  malformed specifier (eg %i):  0xdeadbeef
NULL pointer:
  printed with %pK: 
  printed with %p:  (null)   # NOTE: no padding
  malformed specifier (eg %i):  (null)

For kpt_restrict==2

Valid pointer:
  printed with %pK: 

All other output as for kptr_restrict==0

V8:
 - Add second patch cleaning up null pointer printing in pointer()
 - Move %pK handling to separate function, further cleaning up pointer()
 - Move ptr_to_id() call outside of switch statement making hashing
   the default behaviour (including malformed specifiers).
 - Remove use of static_key, replace with simple boolean.

V7:
 - Use tabs instead of spaces (ouch!).

V6:
 - Use __early_initcall() to fill the SipHash key.
 - Use static keys to guard hashing before the key is available.

V5:
 - Remove spin lock.
 - Add Jason A. Donenfeld to CC list by request.
 - Add Theodore Ts'o to CC list due to comment on previous version.

V4:
 - Remove changes to siphash.{ch}
 - Do word size check, and return value cast, directly in ptr_to_id().
 - Use add_ready_random_callback() to guard call to get_random_bytes()

V3:
 - Use atomic_xchg() to guard setting [random] key.
 - Remove erroneous white space change.

V2:
 - Use SipHash to do the hashing.

The discussion related to this patch has been fragmented. There are
three threads associated with this patch. Email threads by subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

Tobin C. Harding (2):
  printk: remove tabular output for NULL pointer
  printk: hash addresses printed with %p

 lib/vsprintf.c | 166 +
 1 file changed, 108 insertions(+), 58 deletions(-)

-- 
2.7.4

[PATCH V8 2/2] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as
follows (thanks to Joe Perches).

$ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
   1084 arch
 20 block
 10 crypto
 32 Documentation
   8121 drivers
   1221 fs
143 include
101 kernel
 69 lib
100 mm
   1510 net
 40 samples
  7 scripts
 11 security
166 sound
152 tools
  2 virt

Add function ptr_to_id() to map an address to a 32 bit unique identifier.

Signed-off-by: Tobin C. Harding 
---
 lib/vsprintf.c | 157 +++--
 1 file changed, 107 insertions(+), 50 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 16a587aed40e..8f4aebd10c7e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #ifdef CONFIG_BLOCK
 #include 
 #endif
@@ -1344,6 +1346,57 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 }
 
 static noinline_for_stack
+char *kernel_pointer(char *buf, char *end, const void *ptr,
+struct printf_spec spec)
+{
+   spec.base = 16;
+   spec.flags |= SMALL;
+   if (spec.field_width == -1) {
+   spec.field_width = 2 * sizeof(void *);
+   spec.flags |= ZEROPAD;
+   }
+
+   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())
+   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;
+   }
+
+   return number(buf, end, (unsigned long)ptr, spec);
+}
+
+static noinline_for_stack
 char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 {
unsigned long long num;
@@ -1591,6 +1644,54 @@ char *device_node_string(char *buf, char *end, struct 
device_node *dn,
return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static bool have_filled_random_ptr_key;
+static siphash_key_t ptr_key __read_mostly;
+
+static void fill_random_ptr_key(struct random_ready_callback *unused)
+{
+   get_random_bytes(&ptr_key, sizeof(ptr_key));
+   WRITE_ONCE(have_filled_random_ptr_key, true);
+}
+
+static struct random_ready_callback random_ready = {
+   .func = fill_random_ptr_key
+};
+
+static int __init initialize_ptr_random(void)
+{
+   int ret = add_random_ready_callback(&random_ready);
+
+   if (!ret)
+   return 0;
+   else if (ret == -EALREADY) {
+   fill_random_ptr_key(&random_ready);
+   return 0;
+   }
+
+   return ret;
+}
+early_initcall(initialize_ptr_random);
+
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec 
spec)
+{
+   unsigned int hashval;
+   int size = sizeof(hashval);
+
+   if (unlikely(!have_filled_random_ptr_key)) {
+   spec.field_width = 2 + 2 * size; /* 0x + hex */
+   return string(buf, end, "(pointer)", spec);
+   }
+
+#ifdef CONFIG_64BIT
+   hashval = (unsigned int)siphash_1u64((u64)ptr, &ptr_key);
+#else
+   hashval = (unsigned int)siphash_1u32((u32)ptr, &ptr_key);
+#endif
+
+   return special_hex_number(buf, end, h

Re: [PATCH V8 2/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Thu, Oct 26, 2017 at 01:53:56PM +1100, Tobin C. Harding wrote:
> Currently there are many places in the kernel where addresses are being
> printed using an unadorned %p. Kernel pointers should be printed using
> %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> gives attackers sensitive information about the kernel layout in memory.
> 
> We can reduce the attack surface by hashing all addresses printed with
> %p. This will of course break some users, forcing code printing needed
> addresses to be updated.
> 
> For what it's worth, usage of unadorned %p can be broken down as
> follows (thanks to Joe Perches).
> 
> $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
>1084 arch
>  20 block
>  10 crypto
>  32 Documentation
>8121 drivers
>1221 fs
> 143 include
> 101 kernel
>  69 lib
> 100 mm
>1510 net
>  40 samples
>   7 scripts
>  11 security
> 166 sound
> 152 tools
>       2 virt
> 
> Add function ptr_to_id() to map an address to a 32 bit unique identifier.
> 
> Signed-off-by: Tobin C. Harding 
> ---
>  lib/vsprintf.c | 157 
> +++--
>  1 file changed, 107 insertions(+), 50 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 16a587aed40e..8f4aebd10c7e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -33,6 +33,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #ifdef CONFIG_BLOCK
>  #include 
>  #endif
> @@ -1344,6 +1346,57 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
>  }
>  
>  static noinline_for_stack
> +char *kernel_pointer(char *buf, char *end, const void *ptr,
> +  struct printf_spec spec)
> +{
> + spec.base = 16;
> + spec.flags |= SMALL;
> + if (spec.field_width == -1) {
> + spec.field_width = 2 * sizeof(void *);
> + spec.flags |= ZEROPAD;
> + }
> +
> + 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())
> + 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;
> + }
> +
> + return number(buf, end, (unsigned long)ptr, spec);
> +}
> +
> +static noinline_for_stack
>  char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
>  {
>   unsigned long long num;
> @@ -1591,6 +1644,54 @@ char *device_node_string(char *buf, char *end, struct 
> device_node *dn,
>   return widen_string(buf, buf - buf_start, end, spec);
>  }
>  
> +static bool have_filled_random_ptr_key;
> +static siphash_key_t ptr_key __read_mostly;
> +
> +static void fill_random_ptr_key(struct random_ready_callback *unused)
> +{
> + get_random_bytes(&ptr_key, sizeof(ptr_key));
> + WRITE_ONCE(have_filled_random_ptr_key, true);

This usage of WRITE_ONCE was suggested by Jason A. Donenfeld. I read
include/linux/compiler.h but was not able to grok it. Is this enough to
stop the compiler re-ordering these two statements? 

Or do I need to read Documentation/memory-barriers.txt [again]?

thanks,
Tobin.

[PATCH] scripts: add leaking_addresses.pl

[Tobin C. Harding]

Currently we are leaking addresses from the kernel to user space. This
script is an attempt to find those leakages. Script parses `dmesg`
output and /proc and /sys files for hex strings that look like kernel
addresses.

Only works for 64 bit kernels, the reason being that kernel addresses
on 64 bit kernels have '' as the leading bit pattern making greping
possible. On 32 kernels we don't have this luxury.

Signed-off-by: Tobin C. Harding 
---

Changes since RFC V2
 - Refactor the code that skips directories and files into helper functions.
 - Add documentation.
 - Add an entry in MAINTAINERS.

 MAINTAINERS  |   5 +
 scripts/leaking_addresses.pl | 282 +++
 2 files changed, 287 insertions(+)
 create mode 100755 scripts/leaking_addresses.pl

diff --git a/MAINTAINERS b/MAINTAINERS
index e652a3e2929d..c1ad6d133a57 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7745,6 +7745,11 @@ S:   Maintained
 F: Documentation/scsi/53c700.txt
 F: drivers/scsi/53c700*
 
+LEAKING_ADDRESSES
+M: Tobin C. Harding 
+S: Maintained
+F: scripts/leaking_addresses.pl
+
 LED SUBSYSTEM
 M: Richard Purdie 
 M: Jacek Anaszewski 
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
new file mode 100755
index ..430ff1b0c3ec
--- /dev/null
+++ b/scripts/leaking_addresses.pl
@@ -0,0 +1,282 @@
+#!/usr/bin/env perl
+#
+# (c) 2017 Tobin C. Harding 
+# Licensed under the terms of the GNU GPL License version 2
+#
+# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
+#  - Scans dmesg output.
+#  - Walks directory tree and parses each file (for each directory in @DIRS).
+#
+# You can configure the behaviour of the script;
+#
+#  - By adding paths, for directories you do not want to walk;
+# absolute paths: @skip_walk_dirs_abs
+# directory names: @skip_walk_dirs_any
+#
+#  - By adding paths, for files you do not want to parse;
+# absolute paths: @skip_parse_files_abs
+# file names: @skip_parse_files_any
+#
+# The use of @skip_xxx_xxx_any causes files to be skipped where ever they 
occur.
+# For example adding 'fd' to @skip_walk_dirs_any causes the fd/ directory to be
+# skipped for all PID sub-directories of /proc
+#
+# The same thing can be achieved by passing command line options to --dont-walk
+# and --dont-parse. If absolute paths are supplied to these options they are
+# appended to the @skip_xxx_xxx_abs arrays. If file names are supplied to these
+# options, they are appended to the @skip_xxx_xxx_any arrays.
+#
+# Use --debug to output path before parsing, this is useful to find files that
+# cause the script to choke.
+#
+# You may like to set kptr_restrict=2 before running script
+# (see Documentation/sysctl/kernel.txt).
+
+use warnings;
+use strict;
+use POSIX;
+use File::Basename;
+use File::Spec;
+use Cwd 'abs_path';
+use Term::ANSIColor qw(:constants);
+use Getopt::Long qw(:config no_auto_abbrev);
+
+my $P = $0;
+my $V = '0.01';
+
+# Directories to scan.
+my @DIRS = ('/proc', '/sys');
+
+# Command line options.
+my $help = 0;
+my $debug = 0;
+my @dont_walk = ();
+my @dont_parse = ();
+
+# Do not parse these files (ablosute path).
+my @skip_parse_files_abs = ('/proc/kmsg',
+'/proc/kcore',
+'/proc/fs/ext4/sdb1/mb_groups',
+'/proc/1/fd/3',
+'/sys/kernel/debug/tracing/trace_pipe',
+'/sys/kernel/security/apparmor/revision');
+
+# Do not parse thes files under any subdirectory.
+my @skip_parse_files_any = ('0',
+'1',
+'2',
+'pagemap',
+'events',
+'access',
+'registers',
+'snapshot_raw',
+'trace_pipe_raw',
+'ptmx',
+'trace_pipe');
+
+# Do not walk these directories (absolute path).
+my @skip_walk_dirs_abs = ();
+
+# Do not walk these directories under any subdirectory.
+my @skip_walk_dirs_any = ('self',
+  'thread-self',
+  'cwd',
+  'fd',
+  'stderr',
+  'stdin',
+  'stdout');
+
+sub help
+{
+   my ($exitcode) = @_;
+
+   print << "EOM";
+Usage: $P [OPTIONS]
+Version: $V
+
+Options:
+
+  --dont-walk=  Don't walk tree starting at .
+  --dont-parse=Don't parse .
+  -d, --debu

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Tobin C. Harding]

Hi Joe,

thanks for your review.

On Wed, Oct 25, 2017 at 09:57:23PM -0700, Joe Perches wrote:
> On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> > Currently pointer() checks for a NULL pointer argument and then if so
> > attempts to print "(null)" with _some_ standard width. This width cannot
> > correctly be ascertained here because many of the printk specifiers
> > print pointers of varying widths.
> 
> I believe this is not a good change.
> Only pointers without a  extension call pointer()

Sorry, I don't understand what you mean here. All the %p specifier code is
handled by pointer()?

> > Remove the attempt to print NULL pointers with a correct width.
> 
> the correct width for a %p is the default width.

It is the default width if we are printing addresses. Once we hash 64
bit address to a 32 bit identifier then we don't have a default width.

> The correct width for %p is unknown.

I agree.

If I have misunderstood you, please forgive me. I am very appreciative
of the reviews this patch is getting and the patience the list is having
with the many iterations.

thanks,
Tobin.

Re: [PATCH v7] printk: hash addresses printed with %p

[Tobin C. Harding]

On Thu, Oct 26, 2017 at 09:00:03AM +0200, Greg KH wrote:
> On Thu, Oct 26, 2017 at 12:59:08AM +0200, Jason A. Donenfeld wrote:
> > On Thu, Oct 26, 2017 at 12:27 AM, Tobin C. Harding  wrote:
> > > How good is unlikely()?
> > 
> > It places that branch way at the bottom of the function so that it's
> > less likely to pollute the icache.
> 
> But always measure it.  Lots of times (old numbers were 90% or so), we
> get the marking wrong, so please, always benchmark the thing to verify
> it actually is doing what you think it should be doing, otherwise it
> could make the code worse.

Does this come under 'premature optimization is the root of all evil'?

Should we be leaving out things like unlikely() and __read_only until
the code has been profiled?

thanks,
Tobin.

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Tobin C. Harding]

On Thu, Oct 26, 2017 at 01:05:39AM -0700, Joe Perches wrote:
> On Thu, 2017-10-26 at 17:27 +1100, Tobin C. Harding wrote:
> > Hi Joe,
> > 
> > thanks for your review.
> > 
> > On Wed, Oct 25, 2017 at 09:57:23PM -0700, Joe Perches wrote:
> > > On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> > > > Currently pointer() checks for a NULL pointer argument and then if so
> > > > attempts to print "(null)" with _some_ standard width. This width cannot
> > > > correctly be ascertained here because many of the printk specifiers
> > > > print pointers of varying widths.
> > > 
> > > I believe this is not a good change.
> > > Only pointers without a  extension call pointer()
> >
> > Sorry, I don't understand what you mean here. All the %p specifier 
> > code is
> > handled by pointer()?
> 
> Sorry, I was imprecise/wrong.
> 
> None of the %p extensions except %pK and %p
> actually use this bit of the pointer() call.

if (!ptr && *fmt != 'K') {
/*
 * Print (null) with the same width as a pointer so it makes
 * tabular output look nice.
 */
if (spec.field_width == -1)
spec.field_width = default_width;
return string(buf, end, "(null)", spec);
}

Is there something I'm missing here? This code reads like its all %p
(including %p and %p) except %pK that hit this block when
a NULL pointer is passed in.

> All of the other valid %p extension uses do not end up
> at this block being executed so it's effectively only regular
> pointers being output by number()
> 
> > > > Remove the attempt to print NULL pointers with a correct width.
> > > 
> > > the correct width for a %p is the default width.
> > 
> > It is the default width if we are printing addresses. Once we hash 64
> > bit address to a 32 bit identifier then we don't have a default width.
> 
> Perhaps that 32 bit identifier should use leading 0's for
> the default width.

That's a fair comment.

> aside:
> 
> Why hash 64 bits to 32?
> Why shouldn't the hash width be 64 bits on 64 bit systems?

Quoted from Linus in an earlier thread discussing this change

Date: Thu, 12 Oct 2017 11:37:22 -0700 Linus Torvalds wrote:

In fact, I'd prefer mapping the pointer to a 32-bit value, even on
64-bit architectures. When people use these things for debugging and
for identifying which device node or socket or whatever they are
tracking, we're generally talking a (small) handful of different
devices or whatever.


Hope this helps,
Tobin.

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Tobin C. Harding]

On Thu, Oct 26, 2017 at 07:47:19AM -0700, Joe Perches wrote:
> On Thu, 2017-10-26 at 20:37 +1100, Tobin C. Harding wrote:
> > On Thu, Oct 26, 2017 at 01:05:39AM -0700, Joe Perches wrote:
> > > On Thu, 2017-10-26 at 17:27 +1100, Tobin C. Harding wrote:
> > > > Hi Joe,
> > > > 
> > > > thanks for your review.
> > > > 
> > > > On Wed, Oct 25, 2017 at 09:57:23PM -0700, Joe Perches wrote:
> > > > > On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> > > > > > Currently pointer() checks for a NULL pointer argument and then if 
> > > > > > so
> > > > > > attempts to print "(null)" with _some_ standard width. This width 
> > > > > > cannot
> > > > > > correctly be ascertained here because many of the printk specifiers
> > > > > > print pointers of varying widths.
> > > > > 
> > > > > I believe this is not a good change.
> > > > > Only pointers without a  extension call pointer()
> > > > 
> > > > Sorry, I don't understand what you mean here. All the %p specifier 
> > > > code is
> > > > handled by pointer()?
> > > 
> > > Sorry, I was imprecise/wrong.
> > > 
> > > None of the %p extensions except %pK and %p
> > > actually use this bit of the pointer() call.
> > 
> > if (!ptr && *fmt != 'K') {
> > /*
> >  * Print (null) with the same width as a pointer so it makes
> >  * tabular output look nice.
> >  */
> > if (spec.field_width == -1)
> > spec.field_width = default_width;
> > return string(buf, end, "(null)", spec);
> > }
> > 
> > Is there something I'm missing here? This code reads like its all %p
> > (including %p and %p) except %pK that hit this block when
> > a NULL pointer is passed in.
> 
> The idea for aligning is described in commit 5e0579812834a
> 
> $ git log --stat -p -1 --format=email 5e0579812834a
> From 5e0579812834ab7fa072db4a15ebdff68d62e2e7 Mon Sep 17 00:00:00 2001
> From: Joe Perches 
> Date: Tue, 26 Oct 2010 14:22:50 -0700
> Subject: [PATCH] vsprintf.c: use default pointer field size for "(null)"
>  strings
> 
> It might be nicer to align the output.
> 
> For instance, ACPI messages sometimes have "(null)" pointers.
> 
> $ dmesg | grep "(null)"  -A 1 -B 1
> [0.198733] ACPI: Dynamic OEM Table Load:
> [0.198745] ACPI: SSDT (null) 00239 (v02  PmRef  Cpu0Ist 3000 INTL 
> 20051117)
> [0.199294] ACPI: SSDT 7f596e10 001C7 (v02  PmRef  Cpu0Cst 3001 INTL 
> 20051117)
> [0.200708] ACPI: Dynamic OEM Table Load:
> [0.200721] ACPI: SSDT (null) 001C7 (v02  PmRef  Cpu0Cst 3001 INTL 
> 20051117)
> [0.201950] ACPI: SSDT 7f597f10 000D0 (v02  PmRef  Cpu1Ist 3000 INTL 
> 20051117)
> [0.203386] ACPI: Dynamic OEM Table Load:
> [0.203398] ACPI: SSDT (null) 000D0 (v02  PmRef  Cpu1Ist 3000 INTL 
> 20051117)
> [0.203871] ACPI: SSDT 7f595f10 00083 (v02  PmRef  Cpu1Cst 3000 INTL 
> 20051117)
> [0.205301] ACPI: Dynamic OEM Table Load:
> [0.205315] ACPI: SSDT (null) 00083 (v02  PmRef  Cpu1Cst 3000 INTL 
> 20051117)

Does this give the correct level of control? Would it not be better to
control the output of NULL pointers in the code that prints them. The
other side of the coin is that with padding a random single debug
message ends up with unwanted white space, e.g

 [0.205315] foo: This pointer (null) some useful error message 

Just thoughts.

> > > All of the other valid %p extension uses do not end up
> > > at this block being executed so it's effectively only regular
> > > pointers being output by number()
> 
> Because passing NULL to any of the %p extensions
> excluding %pK is probably a defect.

This implies that passing NULL to %p is a defect also, does it not. Like
already stated, if it is not a defect, the calling code is in a better
position to decide on the formatting, unless we printed
<-(null)->

with the correct spacing for all architectures, this seems a bit
extravagant though.

> > > > > > Remove the attempt to print NULL pointers with a correct width.
> > > > > 
> > > > > the correct width for a %p is the default width.
> > > > 
> > > > It is the default width if we are printing addresses. Once we hash 64
> > > > bit address to a 32 bit identifier then we don't have a default w

Re: [kernel-hardening] [PATCH v4] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Sun, Nov 12, 2017 at 10:02:55AM -0800, Frank Rowand wrote:
> Hi Michael,
> 
> On 11/12/17 03:49, Michael Ellerman wrote:
> > Hi Frank,
> > 
> > Frank Rowand  writes:
> >> Hi Michael, Tobin,
> >>
> >> On 11/08/17 04:10, Michael Ellerman wrote:
> >>> "Tobin C. Harding"  writes:
> >>>> Currently we are leaking addresses from the kernel to user space. This
> >>>> script is an attempt to find some of those leakages. Script parses
> >>>> `dmesg` output and /proc and /sys files for hex strings that look like
> >>>> kernel addresses.
> >>>>
> >>>> Only works for 64 bit kernels, the reason being that kernel addresses
> >>>> on 64 bit kernels have '' as the leading bit pattern making greping
> >>>> possible.
> >>>
> >>> That doesn't work super well on other architectures :D
> >>>
> >>> I don't speak perl but presumably you can check the arch somehow and
> >>> customise the regex?
> >>>
> >>> ...
> >>>> +# Return _all_ non false positive addresses from $line.
> >>>> +sub extract_addresses
> >>>> +{
> >>>> +my ($line) = @_;
> >>>> +my $address = '\b(0x)?[[:xdigit:]]{12}\b';
> >>>
> >>> On 64-bit powerpc (ppc64/ppc64le) we'd want:
> >>>
> >>> +my $address = '\b(0x)?[89abcdef]00[[:xdigit:]]{13}\b';
> >>>
> >>>
> >>>> +# Do not parse these files (absolute path).
> >>>> +my @skip_parse_files_abs = ('/proc/kmsg',
> >>>> +'/proc/kcore',
> >>>> +'/proc/fs/ext4/sdb1/mb_groups',
> >>>> +'/proc/1/fd/3',
> >>>> +'/sys/kernel/debug/tracing/trace_pipe',
> >>>> +'/sys/kernel/security/apparmor/revision');
> >>>
> >>> Can you add:
> >>>
> >>>   /sys/firmware/devicetree
> >>>
> >>> and/or /proc/device-tree (which is a symlink to the above).
> >>
> >> /proc/device-tree is a symlink to /sys/firmware/devicetree/base
> > 
> > Oh yep, forgot about the base part.
> > 
> >> /sys/firmware contains
> >>fdt  -- the flattened device tree that was passed to the
> >>kernel on boot
> >>devicetree/base/ -- the data that is currently in the live device tree.
> >>This live device tree is represented as directories
> >>and files beneath base/
> >>
> >> The information in fdt is directly available in the kernel source tree
> > 
> > On ARM that might be true, but not on powerpc.

Looks like we should be considering architecture specific lists for
files/directories to skip.

thanks,
Tobin.

Re: [kernel-hardening] [PATCH v4] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Fri, Nov 10, 2017 at 07:26:34PM +0530, kaiwan.billimo...@gmail.com wrote:
> On Tue, 2017-11-07 at 21:32 +1100, Tobin C. Harding wrote:
> > Currently we are leaking addresses from the kernel to user space.
> > This
> > script is an attempt to find some of those leakages. Script parses
> > `dmesg` output and /proc and /sys files for hex strings that look
> > like
> > kernel addresses.
> > 
> > Only works for 64 bit kernels, the reason being that kernel addresses
> > on 64 bit kernels have '' as the leading bit pattern making
> > greping
> > possible. On 32 kernels we don't have this luxury.
> 
> Tobin C. Harding  wrote:
> >Only works for 64 bit kernels, the reason being that kernel addresses
> >on 64 bit kernels have '' as the leading bit pattern making greping
> >possible. On 32 kernels we don't have this luxury.
> 
> [RFC] leaking_addresses.pl - enhance it to work for 32-bit kernels as well
> 
> (Firstly, apologies if I've got the protocol horribly wrong- should this
> be a new thread altogether?)

I think this patch will need to wait until the patch set that is
currently in flight is either merged or dropped.

> Ok so, I was interested in figuring - why not have this useful script work
> for 32-bit kernel virtual addresses as well (and those systems by
> extension).

Awesome.

> The approach am considering, pl correct me if I'm way off:
> on 32-bit, the kernel macro PAGE_OFFSET will give us the user-kernel split;
> (alternatively, could also script up CONFIG_VMSPLIT_[n]G and figure the
> split from there.)
> 
> For the time being, lets say we go with the "use PAGE_OFFSET" approach and
> PAGE_OFFSET = 0xc000 , whch implies we have a 3:1 GB user:kernel split.
> So any virtual addresses >= PAGE_OFFSET are kernel virtual addresses (i
> know, untrue on some ARM-32 systems!).
> 
> As a very early and *far-from-perfect* start, I've enhanced Tobin's Perl
> script to take into account 32-bit address space by passing the
> parameter '--bit-size='.

We can work this out pragmatically, Perl can give us an architecture
string then a few regexs can ascertain which architecture we are running
on. This is in the inflight patch set. 

> The patch below does Not take into account (yet) stuff like:
>  - exactly which files & dirs should be skipped on 32-bit (will it be
> identical to 64-bit?; unsure..)

As per discussion later in this thread we may need to consider
architecture specific lists for files/directories to skip. 

>  - it currently hard-codes a global 'PAGE_OFFSET_32BIT=0xc000' , just
>  so I can test quickly; must figure whether to query it or pass it;
>  Suggestions?

Perhaps we should have a command line option for this.

--kernel-base-address

>  - the 'false positives'; again, what differs for 32-bit?
>(BTW, shouldn't the dmesg 'root=UUID=<...>' line be a false positive
> & skipped?).

We could probably do with architecture specific false
positives. Inflight patch set refactors false_positive() so adding to
this should be easy.

> Also, I must point out that I'm a complete newbie to Perl :-) so, pl excuse
> my highly inadequate perl-foo; I rely on you perl gurus out there to fix
> and optimize :)

I'm no Perl guru but following are a few tips I have picked up over the
last month.

> Yes, I've **Very Minimally** tested the patch in it's current form on:
> a) a regular (Fedora 26) x86_64 desktop,
> b) a (Debian 7) 32-bit kernel (VM) with PAGE_OFFSET=3 Gb
> and it seems all right, considering...
> 
> Some sample output from test (b), if interested:
> =
> dmesg: [0.00] found SMP MP-table at [c00f1280] f1280
> dmesg: [0.00] Base memory trampoline at [c009b000] 9b000 size 16384
> dmesg: [0.00] ACPI: Local APIC address 0xfee0
> dmesg: [0.00] free_area_init_node: node 0, pgdat c1418bc0, 
> node_mem_map dfbfa200
> dmesg: [0.00] ACPI: Local APIC address 0xfee0
> dmesg: [0.00] ACPI: IOAPIC (id[0x00] address[0xfec0] gsi_base[0])
> dmesg: [0.00] IOAPIC[0]: apic_id 0, version 17, address 0xfec0, 
> GSI 0-23
> dmesg: [0.00] PERCPU: Embedded 14 pages/cpu @dfbe8000 s33344 r0 
> d24000 u57344
> dmesg: [0.00] fixmap  : 0xffd36000 - 0xf000   (2852 kB)
> dmesg: [0.00] pkmap   : 0xffa0 - 0xffc0   (2048 kB)
> dmesg: [0.00] vmalloc : 0xe07fb000 - 0xff9fe000   ( 498 MB)
> dmesg: [0.00] lowmem  : 0xc000 - 0xdfffb000   ( 511 MB)
> dmesg: [0.00]   .init : 0xc1421000 - 0xc148c000   ( 428 kB)
> 
> [...]
> 
> /proc/kallsyms: c10010

Re: [PATCH v4] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Sun, Nov 12, 2017 at 02:10:07AM +0300, Kirill A. Shutemov wrote:
> On Tue, Nov 07, 2017 at 09:32:11PM +1100, Tobin C. Harding wrote:
> > Currently we are leaking addresses from the kernel to user space. This
> > script is an attempt to find some of those leakages. Script parses
> > `dmesg` output and /proc and /sys files for hex strings that look like
> > kernel addresses.
> > 
> > Only works for 64 bit kernels, the reason being that kernel addresses
> > on 64 bit kernels have '' as the leading bit pattern making greping
> > possible. On 32 kernels we don't have this luxury.
> 
> Well, it's not going to work as well as intented on x86 machine with
> 5-level paging. Kernel address space there starts at 0xff10.
> It will still catch pointers to kernel/modules text, but the rest is
> outside of 0x... space. See Documentation/x86/x86_64/mm.txt.

Thanks for the link. So it looks like we need to refactor the kernel
address regular expression into a function that takes into account the
machine architecture and the number of page table levels. We will need
to add this to the false positive checks also.

> Not sure if we care. It won't work too for other 64-bit architectrues that
> have more than 256TB of virtual address space.

Is this because of the virtual memory map? Did you mean 512TB?

from mm.txt:
ffd4 - ffd5 (=49 bits) virtual memory map (512TB)

Perhaps an option (--terse) that only checks the virtual memory map
range (above for 5-level paging) and

ea00 - eaff (=40 bits) virtual memory map (1TB)

for 4-level paging?

> Just wanted to point to the limitation.

Appreciate it, thanks.

Tobin.

Re: [PATCH v4] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 13, 2017 at 06:37:28AM +0300, Kirill A. Shutemov wrote:
> On Mon, Nov 13, 2017 at 10:06:46AM +1100, Tobin C. Harding wrote:
> > On Sun, Nov 12, 2017 at 02:10:07AM +0300, Kirill A. Shutemov wrote:
> > > On Tue, Nov 07, 2017 at 09:32:11PM +1100, Tobin C. Harding wrote:
> > > > Currently we are leaking addresses from the kernel to user space. This
> > > > script is an attempt to find some of those leakages. Script parses
> > > > `dmesg` output and /proc and /sys files for hex strings that look like
> > > > kernel addresses.
> > > > 
> > > > Only works for 64 bit kernels, the reason being that kernel addresses
> > > > on 64 bit kernels have '' as the leading bit pattern making greping
> > > > possible. On 32 kernels we don't have this luxury.
> > > 
> > > Well, it's not going to work as well as intented on x86 machine with
> > > 5-level paging. Kernel address space there starts at 0xff10.
> > > It will still catch pointers to kernel/modules text, but the rest is
> > > outside of 0x... space. See Documentation/x86/x86_64/mm.txt.
> > 
> > Thanks for the link. So it looks like we need to refactor the kernel
> > address regular expression into a function that takes into account the
> > machine architecture and the number of page table levels. We will need
> > to add this to the false positive checks also.
> > 
> > > Not sure if we care. It won't work too for other 64-bit architectrues that
> > > have more than 256TB of virtual address space.
> > 
> > Is this because of the virtual memory map?
> 
> On x86 direct mapping is the nearest thing we have to userspace.
> 
> > Did you mean 512TB?
> 
> No, I mean 256TB.
> 
> You have all kernel memory in the range from 0x to
> 0x if you have 256 TB of virtual address space. If you
> hvae more, some thing might be ouside the range.

Doesn't 4-level paging already limit a system to 64TB of memory? So any
system better equipped than this will use 5-level paging right? If I am
totally talking rubbish please ignore, I'm appreciative that you pointed
out the limitation already. Perhaps we can add a comment to the script

# Script may miss some addresses on machines with more than 256TB of
# memory.

thanks,
Tobin.

Re: [kernel-hardening] [PATCH v4] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 13, 2017 at 11:16:28AM +0530, kaiwan.billimo...@gmail.com wrote:
> On Mon, 2017-11-13 at 09:21 +1100, Tobin C. Harding wrote:
> > On Fri, Nov 10, 2017 at 07:26:34PM +0530, kaiwan.billimo...@gmail.com
> >  wrote:
> > > On Tue, 2017-11-07 at 21:32 +1100, Tobin C. Harding wrote:
> > > > Currently we are leaking addresses from the kernel to user space.
> > > > This
> > > > script is an attempt to find some of those leakages. Script
> > > > parses
> > > > `dmesg` output and /proc and /sys files for hex strings that look
> > > > like
> > > > kernel addresses.
> > > > 
> > > > Only works for 64 bit kernels, the reason being that kernel
> > > > addresses
> > > > on 64 bit kernels have '' as the leading bit pattern making
> > > > greping
> > > > possible. On 32 kernels we don't have this luxury.
> > > 
> > > Tobin C. Harding  wrote:
> > > > Only works for 64 bit kernels, the reason being that kernel
> > > > addresses
> > > > on 64 bit kernels have '' as the leading bit pattern making
> > > > greping
> > > > possible. On 32 kernels we don't have this luxury.
> > > 
> > > [RFC] leaking_addresses.pl - enhance it to work for 32-bit kernels
> > > as well
> > > 
> > > (Firstly, apologies if I've got the protocol horribly wrong- should
> > > this
> > > be a new thread altogether?)
> > 
> > I think this patch will need to wait until the patch set that is
> > currently in flight is either merged or dropped.
> > 
> Thanks for looking at it!
> Okay; blocking on merge || drop...  :-)

So, Linus has requested that I set up a tree for the development of
this. I have to work out the details of how to do that and then I'll
email you so you can get the pull the current version. I can then take
your patch via LKML as per usual.

> > We can work this out pragmatically, Perl can give us an architecture
> > string then a few regexs can ascertain which architecture we are
> > running
> > on. This is in the inflight patch set. 
> > 
> > > The patch below does Not take into account (yet) stuff like:
> > >  - exactly which files & dirs should be skipped on 32-bit (will it
> > > be
> > > identical to 64-bit?; unsure..)
> > 
> > As per discussion later in this thread we may need to consider
> > architecture specific lists for files/directories to skip. 
> Right
> > 
> > >  - it currently hard-codes a global 'PAGE_OFFSET_32BIT=0xc000'
> > > , just
> > >  so I can test quickly; must figure whether to query it or pass it;
> > >  Suggestions?
> > 
> > Perhaps we should have a command line option for this.
> > 
> > --kernel-base-address
> 
> Why not just detect it programatically? We could devise a series of
> fallbacks; something like:
> - if .config exists in the kernel source tree root, grep it for
> PAGE_OFFSET
> - if not, grep the arch-specific (arch//configs/)
> for the same
> - if for some reason we don't have enough info regarding specific
> platform and thus the defconfig filename (could happen for ARM, PPC?),
> we then fail and request the user to pass it as a parameter.
> 
> > >  - the 'false positives'; again, what differs for 32-bit?

Sounds good to me.

thanks,
Tobin.

[GIT PULL] leaking_addresses updates for 4.15

[Tobin C. Harding]

The following changes since commit: bebc6082da0a9f5d47a1ea2edc099bf671058bd4

  Linux 4.14 (2017-11-12 10:46:13 -0800)

are available in the git repository at:

  https://github.com/tcharding/linux

for you to fetch changes up to a11949ec20635b43d82ee229315fd2e3c80c22a3:

  leaking_addresses: add SigIgn to false positives (2017-11-14 09:25:11 +1100)


--
leaking_addresses updates for 4.15

Here are development updates for the leaking_addresses.pl script.

First there is some clean up. Also we change the command line options;
add summary reporting functionality. We add a timeout to stop the script
blocking indefinitely and add SigIgn to false positive checks. We also
add infrastructure to handle multiple architectures and add support for
ppc64.

Signed-off-by: Tobin C. Harding 

--

Tobin C. Harding (9):
  leaking_addresses: use tabs instead of spaces
  leaking_addresses: remove dead/unused code
  leaking_addresses: remove command line options
  leaking_addresses: fix comment string typo
  leaking_addresses: add to exclude files/paths list
  leaking_addresses: add summary reporting options
  leaking_addresses: add support for ppc64
  leaking_addresses: add timeout on file read
  leaking_addresses: add SigIgn to false positives

 scripts/leaking_addresses.pl | 370 +--
 1 file changed, 283 insertions(+), 87 deletions(-)

[GIT PULL resend] leaking_addresses updates for 4.15

[Tobin C. Harding]

The following changes since commit: bebc6082da0a9f5d47a1ea2edc099bf671058bd4

  Linux 4.14 (2017-11-12 10:46:13 -0800)

are available in the git repository at:

  https://github.com/tcharding/linux for-linus

for you to fetch changes up to a11949ec20635b43d82ee229315fd2e3c80c22a3:

  leaking_addresses: add SigIgn to false positives (2017-11-14 09:25:11 +1100)


--
leaking_addresses updates for 4.15

Here are development updates for the leaking_addresses.pl script.

First there is some clean up. Also we change the command line options;
add summary reporting functionality. We add a timeout to stop the script
blocking indefinitely and add SigIgn to false positive checks. We also
add infrastructure to handle multiple architectures and add support for
ppc64.

Signed-off-by: Tobin C. Harding 

--

Tobin C. Harding (9):
  leaking_addresses: use tabs instead of spaces
  leaking_addresses: remove dead/unused code
  leaking_addresses: remove command line options
  leaking_addresses: fix comment string typo
  leaking_addresses: add to exclude files/paths list
  leaking_addresses: add summary reporting options
  leaking_addresses: add support for ppc64
  leaking_addresses: add timeout on file read
  leaking_addresses: add SigIgn to false positives

 scripts/leaking_addresses.pl | 370 +--
 1 file changed, 283 insertions(+), 87 deletions(-)

[GIT PULL 2nd resend] leaking_addresses updates for 4.15

[Tobin C. Harding]

The following changes since commit: bebc6082da0a9f5d47a1ea2edc099bf671058bd4

  Linux 4.14 (2017-11-12 10:46:13 -0800)

are available in the git repository at:

  https://github.com/tcharding/linux tags/leaking_addresses-4.15

for you to fetch changes up to a11949ec20635b43d82ee229315fd2e3c80c22a3:

  leaking_addresses: add SigIgn to false positives (2017-11-14 09:25:11 +1100)


Correct tag name. Clearly I am doing this manually, and failing pretty
hard. Sorry for the noise.

thanks,
Tobin.

--
leaking_addresses updates for 4.15

Here are development updates for the leaking_addresses.pl script.

First there is some clean up. Also we change the command line options;
add summary reporting functionality. We add a timeout to stop the script
blocking indefinitely and add SigIgn to false positive checks. We also
add infrastructure to handle multiple architectures and add support for
ppc64.

Signed-off-by: Tobin C. Harding 

--

Tobin C. Harding (9):
  leaking_addresses: use tabs instead of spaces
  leaking_addresses: remove dead/unused code
  leaking_addresses: remove command line options
  leaking_addresses: fix comment string typo
  leaking_addresses: add to exclude files/paths list
  leaking_addresses: add summary reporting options
  leaking_addresses: add support for ppc64
  leaking_addresses: add timeout on file read
  leaking_addresses: add SigIgn to false positives

 scripts/leaking_addresses.pl | 370 +--
 1 file changed, 283 insertions(+), 87 deletions(-)

Re: [GIT PULL 2nd resend] leaking_addresses updates for 4.15

[Tobin C. Harding]

On Tue, Nov 14, 2017, at 10:04, Tobin C. Harding wrote:
> The following changes since commit:
> bebc6082da0a9f5d47a1ea2edc099bf671058bd4
> 
>   Linux 4.14 (2017-11-12 10:46:13 -0800)
> 
> are available in the git repository at:
> 
>   https://github.com/tcharding/linux tags/leaking_addresses-4.15
> 
> for you to fetch changes up to a11949ec20635b43d82ee229315fd2e3c80c22a3:
> 
>   leaking_addresses: add SigIgn to false positives (2017-11-14 09:25:11
>   +1100)

Hi Linus,

I did not sign the tag, it looks like you have not processed this yet.
Do you want me to re-do the pull request on a signed tag?

thanks,
Tobin.

Re: git pull

[Tobin C. Harding]

Added Linus to To: header.

On Tue, Nov 14, 2017 at 12:05:00PM +0100, Greg Kroah-Hartman wrote:
> Adding lkml and linux-doc mailing lists...
> 
> On Tue, Nov 14, 2017 at 10:11:55AM +1100, Tobin C. Harding wrote:
> > Hi Greg,
> > 
> > This is totally asking a favour, feel free to ignore. How do you format
> > your [GIT PULL] emails to Linus? Do you create a tag and then run a git
> > command to get the email?
> > 
> > I tried to do it manually and failed pretty hard (as you no doubt will
> > notice on LKML).
> 
> Well, I think you got it right the third time, so nice job :)
> 
> Anyway, this actually came up at the kernel summit / maintainer meeting
> a few weeks ago, in that "how do I make a good pull request to Linus" is
> something we need to document.
> 
> Here's what I do, and it seems to work well, so maybe we should turn it
> into the start of the documentation for how to do it.
> 
> ---
> 
> To start with, put your changes on a branch, hopefully one named in a
> semi-useful way (I use 'char-misc-next' for my char/misc driver patches
> to be merged into linux-next).  That is the branch you wish to tag and
> have Linus pull from.
> 
> Name the tag with something useful that you can understand if you run
> across it in a few weeks, and something that will be "unique".
> Continuing the example of my char-misc tree, for the patches to be sent
> to Linus for the 4.15-rc1 merge window, I would name the tag
> 'char-misc-4.15-rc1':
>   git tag -u KEY_ID -s char-misc-4.15-rc1 char-misc-next
> 
> that will create a signed tag called 'char-misc-4.15-rc1' based on the
> last commit in the char-misc-next branch, and sign it with my gpg key
> KEY_ID (replace KEY_ID with your own gpg key id.)
> 
> When you run the above command, git will drop you into an editor and ask
> you to describe the tag.  In this case, you are describing a pull
> request, so outline what is contained here, why it should be merged, and
> what, if any, testing has happened to it.  All of this information will
> end up in the tag itself, and then in the merge commit that Linus makes,
> so write it up well, as it will be in the kernel tree for forever.
> 
> An example pull request of mine might look like:
>   Char/Misc patches for 4.15-rc1
> 
>   Here is the big char/misc patch set for the 4.15-rc1 merge
>   window.  Contained in here is the normal set of new functions
>   added to all of these crazy drivers, as well as the following
>   brand new subsystems:
>   - time_travel_controller: Finally a set of drivers for
> the latest time travel bus architecture that provides
> i/o to the CPU before it asked for it, allowing
> uninterrupted processing
>   - relativity_shifters: due to the affect that the
> time_travel_controllers have on the overall system,
> there was a need for a new set of relativity shifter
> drivers to accommodate the newly formed black holes
> that would threaten to suck CPUs into them.  This
> subsystem handles this in a way to successfully
> neutralize the problems.  There is a Kconfig option to
> force these to be enabled when needed, so problems
> should not occur.
> 
>   All of these patches have been successfully tested in the latest
>   linux-next releases, and the original problems that it found
>   have all been resolved (apologies to anyone living near Canberra
>   for the lack of the Kconfig options in the earlier versions of
>   the linux-next tree creations.)
> 
>   Signed-off-by: Your-name-here 
> 
> 
> The tag message format is just like a git commit id.  One line at the
> top for a "summary subject" and be sure to sign-off at the bottom.
> 
> Now that you have a local signed tag, you need to push it up to where it
> can be retrieved by others:
>   git push origin char-misc-4.15-rc1
> pushes the char-misc-4.15-rc1 tag to where the 'origin' repo is located.
> 
> The last thing to do is create the pull request message.  Git handily
> will do this for you with the 'git request-pull' command, but it needs a
> bit of help determining what you want to pull, and what to base the pull
> against (to show the correct changes to be pulled and the diffstat.)
> 
> I use the following command to generate a pull request:
>   git request-pull master 
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/ 
> char-misc-4.15-rc1
> 
> This is asking git to compare the d

Re: git pull

[Tobin C. Harding]

On Tue, Nov 14, 2017 at 12:05:00PM +0100, Greg Kroah-Hartman wrote:
> Adding lkml and linux-doc mailing lists...
> 
> On Tue, Nov 14, 2017 at 10:11:55AM +1100, Tobin C. Harding wrote:
> > Hi Greg,
> > 
> > This is totally asking a favour, feel free to ignore. How do you format
> > your [GIT PULL] emails to Linus? Do you create a tag and then run a git
> > command to get the email?
> > 
> > I tried to do it manually and failed pretty hard (as you no doubt will
> > notice on LKML).
> 
> Well, I think you got it right the third time, so nice job :)

Lucky. Three strikes and your out isn't it?

> Anyway, this actually came up at the kernel summit / maintainer meeting
> a few weeks ago, in that "how do I make a good pull request to Linus" is
> something we need to document.
> 
> Here's what I do, and it seems to work well, so maybe we should turn it
> into the start of the documentation for how to do it.

Patch to come.

> ---
> 
> To start with, put your changes on a branch, hopefully one named in a
> semi-useful way (I use 'char-misc-next' for my char/misc driver patches
> to be merged into linux-next).  That is the branch you wish to tag and
> have Linus pull from.
> 
> Name the tag with something useful that you can understand if you run
> across it in a few weeks, and something that will be "unique".
> Continuing the example of my char-misc tree, for the patches to be sent
> to Linus for the 4.15-rc1 merge window, I would name the tag
> 'char-misc-4.15-rc1':
>   git tag -u KEY_ID -s char-misc-4.15-rc1 char-misc-next
> 
> that will create a signed tag called 'char-misc-4.15-rc1' based on the
> last commit in the char-misc-next branch, and sign it with my gpg key
> KEY_ID (replace KEY_ID with your own gpg key id.)
> 
> When you run the above command, git will drop you into an editor and ask
> you to describe the tag.  In this case, you are describing a pull
> request, so outline what is contained here, why it should be merged, and
> what, if any, testing has happened to it.  All of this information will
> end up in the tag itself, and then in the merge commit that Linus makes,
> so write it up well, as it will be in the kernel tree for forever.
> 
> An example pull request of mine might look like:
>   Char/Misc patches for 4.15-rc1
> 
>   Here is the big char/misc patch set for the 4.15-rc1 merge
>   window.  Contained in here is the normal set of new functions
>   added to all of these crazy drivers, as well as the following
>   brand new subsystems:
>   - time_travel_controller: Finally a set of drivers for
> the latest time travel bus architecture that provides
> i/o to the CPU before it asked for it, allowing
> uninterrupted processing
>   - relativity_shifters: due to the affect that the
> time_travel_controllers have on the overall system,
> there was a need for a new set of relativity shifter
> drivers to accommodate the newly formed black holes
> that would threaten to suck CPUs into them.  This
> subsystem handles this in a way to successfully
> neutralize the problems.  There is a Kconfig option to
> force these to be enabled when needed, so problems
> should not occur.
> 
>   All of these patches have been successfully tested in the latest
>   linux-next releases, and the original problems that it found
>   have all been resolved (apologies to anyone living near Canberra
>   for the lack of the Kconfig options in the earlier versions of
>   the linux-next tree creations.)
> 
>   Signed-off-by: Your-name-here 
> 
> 
> The tag message format is just like a git commit id.  One line at the
> top for a "summary subject" and be sure to sign-off at the bottom.
> 
> Now that you have a local signed tag, you need to push it up to where it
> can be retrieved by others:
>   git push origin char-misc-4.15-rc1
> pushes the char-misc-4.15-rc1 tag to where the 'origin' repo is located.
> 
> The last thing to do is create the pull request message.  Git handily
> will do this for you with the 'git request-pull' command, but it needs a
> bit of help determining what you want to pull, and what to base the pull
> against (to show the correct changes to be pulled and the diffstat.)
> 
> I use the following command to generate a pull request:
>   git request-pull master 
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/ 
> char-misc-4.15-rc1
> 
> This

[PATCH] docs: add submitting-pull-requests.rst

[Tobin C. Harding]

There is currently no documentation on how to create a pull request for
Linus.

Anyway, this actually came up at the kernel summit / maintainer
meeting a few weeks ago, in that "how do I make a good pull request
to Linus" is something we need to document.

Here's what I do, and it seems to work well, so maybe we should turn
it into the start of the documentation for how to do it.

Create document from email thread on LKML (referenced in document).

Signed-off-by: Tobin C. Harding 
---

Is it rude to send this during the merge window? Can resend after it closes if
it makes life easier.

thanks,
Tobin.

 Documentation/process/submitting-pull-requests.rst | 171 +
 1 file changed, 171 insertions(+)
 create mode 100644 Documentation/process/submitting-pull-requests.rst

diff --git a/Documentation/process/submitting-pull-requests.rst 
b/Documentation/process/submitting-pull-requests.rst
new file mode 100644
index ..9528aead4809
--- /dev/null
+++ b/Documentation/process/submitting-pull-requests.rst
@@ -0,0 +1,171 @@
+Submitting Pull Requests to Linus: a guide for maintainers
+==
+
+This document is aimed at kernel maintainers.  It describes a method for 
creating
+a pull request to be sent to Linus.
+
+Configure Git
+-
+
+Since you _usually_ would use the same key for the same project, just set it
+once with
+
+   git config user.signingkey "keyname"
+
+and if you use the same key for everything, just add "--global".
+
+Or just edit your .git/config or ~/.gitconfig file by hand, it's designed to be
+human-readable and writable, and not some garbage like XML:
+
+   [torvalds@i7 ~]$ head -4 .gitconfig
+   [user]
+   name = Linus Torvalds
+   email = torva...@linux-foundation.org
+   signingkey = torva...@linux-foundation.org
+
+You may need to tell git to use gpg2
+
+   [gpg]
+   program = /path/to/gpg2
+
+You may also like to tell gpg which tty to use (add to shell rc file)
+
+   export GPG_TTY=$(tty)
+
+
+Branch, Tag, Push
+-
+
+Next, put your changes on a branch, hopefully one named in a semi-useful way (I
+use 'char-misc-next' for my char/misc driver patches to be merged into
+linux-next).  That is the branch you wish to tag and have Linus pull from.
+
+Name the tag with something useful that you can understand if you run across it
+in a few weeks, and something that will be "unique".  Continuing the example of
+the char-misc tree, for the patches to be sent to Linus for the 4.15-rc1 merge
+window, I would name the tag 'char-misc-4.15-rc1':
+
+   git tag -s char-misc-4.15-rc1 char-misc-next
+
+that will create a signed tag called 'char-misc-4.15-rc1' based on the last
+commit in the char-misc-next branch, and sign it with your gpg key (configured
+above).
+
+When you run the above command, git will drop you into an editor and ask you to
+describe the tag.  In this case, you are describing a pull request, so outline
+what is contained here, why it should be merged, and what, if any, testing has
+happened to it.  All of this information will end up in the tag itself, and 
then
+in the merge commit that Linus makes, so write it up well, as it will be in the
+kernel tree for forever.
+
+   Anyway, at least to me, the important part is the *message*. I want to
+   understand what I'm pulling, and why I should pull it. I also want to
+   use that message as the message for the merge, so it should not just
+   make sense to me, but make sense as a historical record too.
+
+   Note that if there is something odd about the pull request, that
+   should very much be in the explanation. If you're touching files that
+   you don't maintain, explain _why_. I will see it in the diffstat
+   anyway, and if you didn't mention it, I'll just be extra suspicious.
+   And when you send me new stuff after the merge window (or even
+   bug-fixes, but ones that look scary), explain not just what they do
+   and why they do it, but explain the _timing_. What happened that this
+   didn't go through the merge window..
+
+   I will take both what you write in the email pull request _and_ in the
+   signed tag, so depending on your workflow, you can either describe
+   your work in the signed tag (which will also automatically make it
+   into the pull request email), or you can make the signed tag just a
+   placeholder with nothing interesting in it, and describe the work
+   later when you actually send me the pull request.
+
+   And yes, I will edit the message. Partly because I tend to do just
+   trivial formatting (the whole indentation and quoting etc), but partly
+   because part of the message may make sense for me at pull time

Re: [PATCH] docs: add submitting-pull-requests.rst

[Tobin C. Harding]

On Tue, Nov 14, 2017 at 04:48:16PM -0700, Jonathan Corbet wrote:

Awesome comments Jon, I knew there would be more to writing docs than
first met the eye.

> On Wed, 15 Nov 2017 09:54:21 +1100
> "Tobin C. Harding"  wrote:
> 
> > There is currently no documentation on how to create a pull request for
> > Linus.
> > 
> > Anyway, this actually came up at the kernel summit / maintainer
> > meeting a few weeks ago, in that "how do I make a good pull request
> > to Linus" is something we need to document.
> > 
> > Here's what I do, and it seems to work well, so maybe we should turn
> > it into the start of the documentation for how to do it.
> > 
> > Create document from email thread on LKML (referenced in document).
> > 
> > Signed-off-by: Tobin C. Harding 
> > ---
> > 
> > Is it rude to send this during the merge window? Can resend after it closes 
> > if
> > it makes life easier.
> 
> I can handle patches during the merge window.  That said, while I welcome
> this effort and think it's a good start, there's a few things I'll
> quibble about:
> 
>  - Much of this was actually written by Greg, I believe, and some by Linus.
>That deserves credit in the changelog, if nowhere else.

Yeah, I struggled for ages with the tense, Greg's stuff is obviously
written as him. But I didn't want to paraphrase and present it as if I'd
written it. After your comments I'm still unsure of the _best_ way to
present this material with a good flow but still giving credit where
credit is due? I didn't seem right to add their names to the document
(thereby presenting myself as them). I did not think of the changelog -
I'll go that path for v2.

>  - Putting it in Documentation/process as RST is good.  But it should be
>added to index.rst and made part of the docs build.  I suspect you
>haven't run it through sphinx at all yet, right?  Some things are
>unlikely to format the way you think they might.

My bad, I knew I would botch some of the RST stuff, didn't think to run
it through sphinx (I tend to view kernel docs as the raw files ;)

> Finally, I see this as being the first installment in what, I hope, will
> someday be a nice "how to be a kernel maintainer" manual.  I wouldn't
> insist on it before taking a patch like this, but if you could see
> through to organizing it as a chapter in a bigger sub-book, that would be
> great.

Happy to do so. I'm no way qualified to produce much of the text but
perhaps can assist in getting things moving.

> Finally finally... Dan Williams [CC'd] has plans for doing some
> maintainer-level documentation.  He may have thoughts on how this fits
> into what he's scheming, and I'd suggest copying him on the next
> iteration.

Let's liaise on this Dan (if you want to).

> Finally finally finally...some specific comments on the text.  Some of
> them might be read to suggest a major expansion of the work you've done;
> please see that as me saying "that would be nice".  Doing all of this is
> not a precondition to getting this document added!

There is no rush to get merged, let's get it into shape first :)

> > +Submitting Pull Requests to Linus: a guide for maintainers
> > +==
> > +
> > +This document is aimed at kernel maintainers.  It describes a method for 
> > creating
> > +a pull request to be sent to Linus.
> 
> Limiting text widths to, say, 75 columns when possible is preferable.  Word
> has it some maintainers are still reading the docs on their adm3a
> terminals.

Got it. (idea for next doc 'column widths howto' - your canonical guide
to column widths (includes git brief, commit log, email, source code,
and docs).

I'm kidding. 75 it is.

> Most maintainers push directly to Linus, so that's an obvious best focus,
> but pull requests happen at other levels too.  One would hope that this
> information would be applicable at all levels, so it might be nice to
> describe it as such.

Oh, Greg had this, I stripped it out. Back in on next spin.

> > +Configure Git
> > +-
> 
> "Configure Git to use your private key"
> 
> We are, of course, missing the whole discussion on why one would want a
> keypair, how to create it, how to get it into the web of trust, etc.  All
> fodder for a separate chapter in our shiny new maintainer book :)  But it
> is worth saying at least that this is about making Git use your key so you
> can sign tags for pull requests.

Funny you should say that, I'm trying to get into the web of trust so
perhaps I can help wit

checkpatch potential false positive

[Tobin C. Harding]

Hi,

When parsing drivers/staging/unisys/visorbus/visorchipset.c in Greg's
staging tree checkpatch emits

--
visorchipset.c
--
WARNING: char * array declaration might be better as static const
#1050: FILE: visorchipset.c:1050:
+   char *envp[] = { env_cmd, env_id, env_state, env_bus, env_dev,

WARNING: char * array declaration might be better as static const
#1140: FILE: visorchipset.c:1140:
+   char *envp[] = { env_selftest, NULL };

total: 0 errors, 2 warnings, 1694 lines checked

I may be wrong but I think the code in question is clean and
correct. Since checkpatch is saying this _might_ be better ... perhaps
checkpatch could emit CHECK instead of WARNING for this?

Here is the complete function to save finding the file.

/*
 * parahotplug_request_kickoff() - initiate parahotplug request
 * @req: the request to initiate
 *
 * Cause uevent to run the user level script to do the disable/enable specified
 * in the parahotplug_request.
 */
static int parahotplug_request_kickoff(struct parahotplug_request *req)
{
struct controlvm_message_packet *cmd = &req->msg.cmd;
char env_cmd[40], env_id[40], env_state[40], env_bus[40], env_dev[40],
 env_func[40];
char *envp[] = { env_cmd, env_id, env_state, env_bus, env_dev,
 env_func, NULL
};

sprintf(env_cmd, "VISOR_PARAHOTPLUG=1");
sprintf(env_id, "VISOR_PARAHOTPLUG_ID=%d", req->id);
sprintf(env_state, "VISOR_PARAHOTPLUG_STATE=%d",
cmd->device_change_state.state.active);
sprintf(env_bus, "VISOR_PARAHOTPLUG_BUS=%d",
cmd->device_change_state.bus_no);
sprintf(env_dev, "VISOR_PARAHOTPLUG_DEVICE=%d",
cmd->device_change_state.dev_no >> 3);
sprintf(env_func, "VISOR_PARAHOTPLUG_FUNCTION=%d",
cmd->device_change_state.dev_no & 0x7);
return kobject_uevent_env(&chipset_dev->acpi_device->dev.kobj,
  KOBJ_CHANGE, envp);
}

If this is not the sort of bug report you like, could you please (if you
have time) school me on how to better report such things in the future.

thanks,
Tobin.

[PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

Currently we are leaking addresses from the kernel to user space. This
script is an attempt to find some of those leakages. Script parses
`dmesg` output and /proc and /sys files for hex strings that look like
kernel addresses.

Only works for 64 bit kernels, the reason being that kernel addresses
on 64 bit kernels have '' as the leading bit pattern making greping
possible. On 32 kernels we don't have this luxury.

Scripts is _slightly_ smarter than a straight grep, we check for false
positives (all 0's or all 1's, and vsyscall start/finish addresses).

Signed-off-by: Tobin C. Harding 
---

v3:
 - Iterate matches to check for results instead of matching input line against
   false positives i.e catch lines that contain results as well as false
   positives. 

v2:
 - Add regex's to prevent false positives.
 - Clean up white space.

 MAINTAINERS  |   5 +
 scripts/leaking_addresses.pl | 306 +++
 2 files changed, 311 insertions(+)
 create mode 100755 scripts/leaking_addresses.pl

diff --git a/MAINTAINERS b/MAINTAINERS
index e652a3e2929d..c1ad6d133a57 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7745,6 +7745,11 @@ S:   Maintained
 F: Documentation/scsi/53c700.txt
 F: drivers/scsi/53c700*
 
+LEAKING_ADDRESSES
+M: Tobin C. Harding 
+S: Maintained
+F: scripts/leaking_addresses.pl
+
 LED SUBSYSTEM
 M: Richard Purdie 
 M: Jacek Anaszewski 
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
new file mode 100755
index ..1487a75cfc52
--- /dev/null
+++ b/scripts/leaking_addresses.pl
@@ -0,0 +1,306 @@
+#!/usr/bin/env perl
+#
+# (c) 2017 Tobin C. Harding 
+# Licensed under the terms of the GNU GPL License version 2
+#
+# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
+#  - Scans dmesg output.
+#  - Walks directory tree and parses each file (for each directory in @DIRS).
+#
+# You can configure the behaviour of the script;
+#
+#  - By adding paths, for directories you do not want to walk;
+# absolute paths: @skip_walk_dirs_abs
+# directory names: @skip_walk_dirs_any
+#
+#  - By adding paths, for files you do not want to parse;
+# absolute paths: @skip_parse_files_abs
+# file names: @skip_parse_files_any
+#
+# The use of @skip_xxx_xxx_any causes files to be skipped where ever they 
occur.
+# For example adding 'fd' to @skip_walk_dirs_any causes the fd/ directory to be
+# skipped for all PID sub-directories of /proc
+#
+# The same thing can be achieved by passing command line options to --dont-walk
+# and --dont-parse. If absolute paths are supplied to these options they are
+# appended to the @skip_xxx_xxx_abs arrays. If file names are supplied to these
+# options, they are appended to the @skip_xxx_xxx_any arrays.
+#
+# Use --debug to output path before parsing, this is useful to find files that
+# cause the script to choke.
+#
+# You may like to set kptr_restrict=2 before running script
+# (see Documentation/sysctl/kernel.txt).
+
+use warnings;
+use strict;
+use POSIX;
+use File::Basename;
+use File::Spec;
+use Cwd 'abs_path';
+use Term::ANSIColor qw(:constants);
+use Getopt::Long qw(:config no_auto_abbrev);
+
+my $P = $0;
+my $V = '0.01';
+
+# Directories to scan.
+my @DIRS = ('/proc', '/sys');
+
+# Command line options.
+my $help = 0;
+my $debug = 0;
+my @dont_walk = ();
+my @dont_parse = ();
+
+# Do not parse these files (absolute path).
+my @skip_parse_files_abs = ('/proc/kmsg',
+   '/proc/kcore',
+   '/proc/fs/ext4/sdb1/mb_groups',
+   '/proc/1/fd/3',
+   '/sys/kernel/debug/tracing/trace_pipe',
+   '/sys/kernel/security/apparmor/revision');
+
+# Do not parse thes files under any subdirectory.
+my @skip_parse_files_any = ('0',
+   '1',
+   '2',
+   'pagemap',
+   'events',
+   'access',
+   'registers',
+   'snapshot_raw',
+   'trace_pipe_raw',
+   'ptmx',
+   'trace_pipe');
+
+# Do not walk these directories (absolute path).
+my @skip_walk_dirs_abs = ();
+
+# Do not walk these directories under any subdirectory.
+my @skip_walk_dirs_any = ('self',
+ 'thread-self',
+ 'cwd',
+ 'fd',
+ 'stderr',
+ 'stdin',
+ 'stdout');
+
+sub help
+{
+   my

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
> Registers ioread/iowrite operations were done via macros,
> sometime using a "magical" implicit parameter.
> 
> Replace all register access with simple inline macros.
> 
> Signed-off-by: Gilad Ben-Yossef 

Hi,

Nice work. I had a little trouble following this one. Perhaps you are
doing more than one thing per patch, feel free to ignore me if I am
wrong but it seems you are moving the macro definition of CC_REG to a
different header, adding the new inline functions and doing some other
change that I can't grok (commented on below).

Perhaps this patch could be broken up.

> ---
>  drivers/staging/ccree/cc_hal.h   | 33 --
>  drivers/staging/ccree/cc_regs.h  | 35 
>  drivers/staging/ccree/dx_reg_base_host.h | 25 --
>  drivers/staging/ccree/ssi_driver.c   | 47 --
>  drivers/staging/ccree/ssi_driver.h   | 20 +--
>  drivers/staging/ccree/ssi_fips.c | 12 +++
>  drivers/staging/ccree/ssi_pm.c   |  4 +--
>  drivers/staging/ccree/ssi_request_mgr.c  | 57 
> 
>  drivers/staging/ccree/ssi_sysfs.c| 11 +++---
>  9 files changed, 78 insertions(+), 166 deletions(-)
>  delete mode 100644 drivers/staging/ccree/cc_hal.h
>  delete mode 100644 drivers/staging/ccree/cc_regs.h
>  delete mode 100644 drivers/staging/ccree/dx_reg_base_host.h
> 
> diff --git a/drivers/staging/ccree/cc_hal.h b/drivers/staging/ccree/cc_hal.h
> deleted file mode 100644
> index eecc866..000
> --- a/drivers/staging/ccree/cc_hal.h
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, see .
> - */
> -
> -/* pseudo cc_hal.h for cc7x_perf_test_driver (to be able to include code from
> - * CC drivers).
> - */
> -
> -#ifndef __CC_HAL_H__
> -#define __CC_HAL_H__
> -
> -#include 
> -
> -#define READ_REGISTER(_addr) ioread32((_addr))
> -#define WRITE_REGISTER(_addr, _data)  iowrite32((_data), (_addr))
> -
> -#define CC_HAL_WRITE_REGISTER(offset, val) \
> - WRITE_REGISTER(cc_base + (offset), val)
> -#define CC_HAL_READ_REGISTER(offset) READ_REGISTER(cc_base + (offset))
> -
> -#endif
> diff --git a/drivers/staging/ccree/cc_regs.h b/drivers/staging/ccree/cc_regs.h
> deleted file mode 100644
> index 2a8fc73..000
> --- a/drivers/staging/ccree/cc_regs.h
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, see .
> - */
> -
> -/*!
> - * @file
> - * @brief This file contains macro definitions for accessing ARM TrustZone
> - *CryptoCell register space.
> - */
> -
> -#ifndef _CC_REGS_H_
> -#define _CC_REGS_H_
> -
> -#include 
> -
> -#define AXIM_MON_COMP_VALUE GENMASK(DX_AXIM_MON_COMP_VALUE_BIT_SIZE + \
> - DX_AXIM_MON_COMP_VALUE_BIT_SHIFT, \
> - DX_AXIM_MON_COMP_VALUE_BIT_SHIFT)
> -
> -/* Register name mangling macro */
> -#define CC_REG(reg_name) DX_ ## reg_name ## _REG_OFFSET
> -
> -#endif /*_CC_REGS_H_*/
> diff --git a/drivers/staging/ccree/dx_reg_base_host.h 
> b/drivers/staging/ccree/dx_reg_base_host.h
> deleted file mode 100644
> index 47bbadb..000
> --- a/drivers/staging/ccree/dx_reg_base_host.h
> +++ /dev/null
> @@ -1,25 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILIT

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 04:46:54PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Nov 06, 2017 at 10:59:47AM +0200, Gilad Ben-Yossef wrote:
> > On Mon, Nov 6, 2017 at 10:37 AM, Tobin C. Harding  wrote:
> > > On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
> > >> Registers ioread/iowrite operations were done via macros,
> > >> sometime using a "magical" implicit parameter.
> > >>
> > >> Replace all register access with simple inline macros.
> > >>
> > >> Signed-off-by: Gilad Ben-Yossef 
> > >
> > > Hi,
> > >
> > > Nice work. I had a little trouble following this one. Perhaps you are
> > > doing more than one thing per patch, feel free to ignore me if I am
> > > wrong but it seems you are moving the macro definition of CC_REG to a
> > > different header, adding the new inline functions and doing some other
> > > change that I can't grok (commented on below).
> > >
> > > Perhaps this patch could be broken up.
> > 
> > Thank you Tobin.
> > 
> > The original macro that I am replacing had an assumption of a variable void 
> > *
> > cc_base being defined in the context of the macro being called, even though
> > it was not listed in the explicit parameter list of the macro.
> > 
> > The inline function that replace it instead takes an explicit
> > parameter a pointer to
> > struct ssi_drive data * , who has said cc_base as one of the fields.
> > 
> > As a result several function that took a void * cc_base parameter
> > (which is than only
> > used implicitly via the macro without ever being visibly referenced), now 
> > take
> > struct ssi_drive data * parameter instead which is passed explicitly
> > to the inline
> > function.
> > 
> > These seems to be the places you are referring to. They are cascading 
> > changes
> > resulting from the change in API between the macro and the inline
> > function that replaces it.
> > 
> > I imagine I can try to break that change to two patches but at least
> > in my mind this is artificial
> > and it is a single logical change.
> > 
> > Having said that, if you think otherwise and consider this
> > none-reviewable even after this
> > explanation let me know and  I'd be happy to break it down.
> 
> Nah, this is fine, I'll take it as-is.  Tobin, thanks for the review.

No worries. Greg make sure you yell at me if I start causing you more
work than I'm saving. It's a fine line reviewing patches when you are
not super experienced.

thanks,
Tobin.

Re: checkpatch potential false positive

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 07:29:18AM -0800, Joe Perches wrote:
> On Mon, 2017-11-06 at 08:33 +, Andy Whitcroft wrote:
> > On Mon, Nov 06, 2017 at 03:19:14PM +1100, Tobin C. Harding wrote:
> > > Hi,
> 
> Hello.
> 
> > > When parsing drivers/staging/unisys/visorbus/visorchipset.c in Greg's
> > > staging tree checkpatch emits
> > > 
> > > --
> > > visorchipset.c
> > > --
> > > WARNING: char * array declaration might be better as static const
> > > #1050: FILE: visorchipset.c:1050:
> > > + char *envp[] = { env_cmd, env_id, env_state, env_bus, env_dev,
> > > 
> > > WARNING: char * array declaration might be better as static const
> > > #1140: FILE: visorchipset.c:1140:
> > > + char *envp[] = { env_selftest, NULL };
> > > 
> > > total: 0 errors, 2 warnings, 1694 lines checked
> > > 
> > > I may be wrong but I think the code in question is clean and
> > > correct. Since checkpatch is saying this _might_ be better ... perhaps
> > > checkpatch could emit CHECK instead of WARNING for this?
> 
> CHECKs aren't enabled by default except for a few
> directories and this warning is much more commonly
> correct than incorrect.

Ok, thanks.

> checkpatch will always have both false positives and
> false negatives.  It's stupid, people generally aren't.
> 
> Just ignore checkpatch bleats that aren't appropriate.

Got it, cheers Andy.

Re: [kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:25:33PM +0300, Pavel Vasilyev wrote:
>  ./leaking_addresses.pl --dont_walk_abs /proc --dont_walk_abs /sys
> Unknown option: dont_walk_abs
> Unknown option: dont_walk_abs

Oh thanks. Documentation is out of sync with the code, what are the odds.

v4 to come.

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:41:09AM -0800, Linus Torvalds wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
>  wrote:
> >
> > Lovely. This is great. It shows just how much totally pointless stuff
> > we leak, and to normal users that really shouldn't need it.
> 
> Side note: it would be good to have some summary view, and perhaps
> some way to limit duplicates.

This has been bothering me also.

> I ended up running this command line from hell to summarize the
> different sources:
> 
> perl leaking_addresses.pl |
> cut -d: -f1 |
> sed 's:/[0-9]*/:/X/:g' |
> sed 's:/module/[^/]*/:/module/X/:g' |
> sort | uniq | less -S
> 
> and maybe that kind of duplicate culling could be part of the script
> itself if you pass it some summary line.
> 
> In particular, if would be nice to have a summary report that
> 
>  - only shows the first address for a particular source
> 
>  - have some logic to collapse repeated entries of "same file, just
> different instance"
> 
> my sed-invocations there are obviously very ad-hoc, I'm  not actually
> advocating that crap, it's only meant as hacky example of what I'm
> talking about. Something smarter would be much better.
> 
> Because right now if some developer runs it, they might miss some case
> that they should care about, simply because it's hidden among all the
> thousands of essentially duplicate cases.

Awesome. I'm on it. thanks.

So, cull duplicates by default, add summary report to end of output, add
'--raw' option to dump all the lines (the current output).

thanks,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 02:27:43AM +0200, Jason A. Donenfeld wrote:
[snip]

Thank you for your extensive comments Jason. I had v3 in flight before I 
received your email, please
don't think I ignored your suggestions.

v4 to come!

thanks,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:13:10PM -0700, Kees Cook wrote:
> On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding  wrote:
> > On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> >> On Tue, 17 Oct 2017 15:52:51 +1100
> >> "Tobin C. Harding"  wrote:
> >>
> >> > Currently there are many places in the kernel where addresses are being
> >> > printed using an unadorned %p. Kernel pointers should be printed using
> >> > %pK allowing some control via the kptr_restrict sysctl. Exposing 
> >> > addresses
> >> > gives attackers sensitive information about the kernel layout in memory.
> >> >
> >> > We can reduce the attack surface by hashing all addresses printed with
> >> > %p. This will of course break some users, forcing code printing needed
> >> > addresses to be updated.
> >> >
> >> > For what it's worth, usage of unadorned %p can be broken down as follows
> >> >
> >> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> >>
> >> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> >> found in kallsyms, but otherwise you have:
> >>
> >>   function+0x
> >
> > You are correct %pF and %pS print an offset. Does this provide an attack 
> > vector,
> > I didn't think so but I'm no security expert. If they do then we need to 
> > amend
> > those calls also.
> 
> They haven't traditionally been a big deal. If they turn out to be
> problematic, we can deal with it then, IMO.

Thanks Kees,
Tobin.

Re: [kernel-hardening] [PATCH v3] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 02:59:17AM +0200, Jason A. Donenfeld wrote:
> Hi Tobin,
> 
> You submitted v3 without replying to my v2 comments. I'll give a
> condensed version of those here for convenience.

Wow, thanks for taking the time to do this. Lesson learned: recheck emails 
right before submitting
patches :)

thanks,
Tobin.

Re: [PATCH v2] staging: ccree: Fix bool comparison

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 07:40:14AM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> Comparision operator "equal to" not required on a variable
> "foo" of type "bool". Bool has only two values, can be used
> directly or with logical not.
> 
> This fixes the following coccinelle warning:
> WARNING: Comparison of bool to 0/1
> 
> Signed-off-by: Suniel Mahesh 

Nice.

Re: [PATCH v2] staging: ccree: fix boolreturn.cocci warning

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 07:42:53AM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
>
> Return "false" instead of 0.
> 
> This fixes the following coccinelle warning:
> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.

So close! The order of problem description and fix is inverted. What about

```
coccinelle emits: WARNING: return of 0/1 in function 'ssi_is_hw_key' with
return type bool.

Return "false" instead of 0.
```


Good luck,
Tobin.

[PATCH v4] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as
follows (thanks to Joe Perches).

$ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
   1084 arch
 20 block
 10 crypto
 32 Documentation
   8121 drivers
   1221 fs
143 include
101 kernel
 69 lib
100 mm
   1510 net
 40 samples
  7 scripts
 11 security
166 sound
152 tools
  2 virt

Add function ptr_to_id() to map an address to a 32 bit unique identifier.

Signed-off-by: Tobin C. Harding 
---

V4:
 - Remove changes to siphash.{ch}
 - Do word size check, and return value cast, directly in ptr_to_id().
 - Use add_ready_random_callback() to guard call to get_random_bytes()

V3:
 - Use atomic_xchg() to guard setting [random] key.
 - Remove erroneous white space change.

V2:
 - Use SipHash to do the hashing.

The discussion related to this patch has been fragmented. There are
three threads associated with this patch. Email threads by subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

 lib/vsprintf.c | 74 +++---
 1 file changed, 71 insertions(+), 3 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..4609738cd2cd 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #ifdef CONFIG_BLOCK
 #include 
 #endif
@@ -1591,6 +1593,70 @@ char *device_node_string(char *buf, char *end, struct 
device_node *dn,
return widen_string(buf, buf - buf_start, end, spec);
 }
 
+/* protects ptr_secret and have_key */
+DEFINE_SPINLOCK(key_lock);
+static siphash_key_t ptr_secret __read_mostly;
+static atomic_t have_key = ATOMIC_INIT(0);
+
+static int initialize_ptr_secret(void)
+{
+   spin_lock(&key_lock);
+   if (atomic_read(&have_key) == 1)
+   goto unlock;
+
+   get_random_bytes(&ptr_secret, sizeof(ptr_secret));
+   atomic_set(&have_key, 1);
+
+unlock:
+   spin_unlock(&key_lock);
+   return 0;
+}
+
+static void schedule_async_key_init(struct random_ready_callback *rdy)
+{
+   initialize_ptr_secret();
+}
+
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec 
spec)
+{
+   static struct random_ready_callback random_ready;
+   unsigned int hashval;
+   int err;
+
+   if (atomic_read(&have_key) == 0) {
+   random_ready.owner = NULL;
+   random_ready.func = schedule_async_key_init;
+
+   err = add_random_ready_callback(&random_ready);
+
+   switch (err) {
+   case 0:
+   return "(pointer value)";
+
+   case -EALREADY:
+   initialize_ptr_secret();
+   break;
+
+   default:
+   /* shouldn't get here */
+   return "(ptr_to_id() error)";
+   }
+   }
+
+#ifdef CONFIG_64BIT
+   hashval = (unsigned int)siphash_1u64((u64)ptr, &ptr_secret);
+#else
+   hashval = (unsigned int)siphash_1u32((u32)ptr, &ptr_secret);
+#endif
+
+   spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
+   spec.flags = SPECIAL | SMALL | ZEROPAD;
+   spec.base = 16;
+
+   return number(buf, end, hashval, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,6 +1769,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.
+ *
+ * Default behaviour (unadorned %p) is to hash the address, rendering it useful
+ * as a unique identifier.
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -1858,14 +1927,13 @@ char *pointer(const char *fmt, char *buf, char *end, 
void *ptr,
return device_node_string(buf, end, ptr, spec, fmt + 1);
}
}
-   spec.flags |= SMALL;
+
if (spec.field_width == -1) {
spec.field_width = default_width;
spec.flags |= ZEROPAD;
}
-   spec.base = 16;
 
-   return number(buf, end, (unsigned long) ptr, spec);
+   return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*
-- 
2.7.4

Re: [PATCH v4] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 02:44:31PM +0900, Sergey Senozhatsky wrote:
> On (10/18/17 15:21), Tobin C. Harding wrote:
> [..]
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 86c3385b9eb3..4609738cd2cd 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -33,6 +33,8 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  #ifdef CONFIG_BLOCK
> >  #include 
> >  #endif
> > @@ -1591,6 +1593,70 @@ char *device_node_string(char *buf, char *end, 
> > struct device_node *dn,
> > return widen_string(buf, buf - buf_start, end, spec);
> >  }
> >  
> > +/* protects ptr_secret and have_key */
> > +DEFINE_SPINLOCK(key_lock);
> > +static siphash_key_t ptr_secret __read_mostly;
> > +static atomic_t have_key = ATOMIC_INIT(0);
> > +
> > +static int initialize_ptr_secret(void)
> > +{
> > +   spin_lock(&key_lock);
> > +   if (atomic_read(&have_key) == 1)
> > +   goto unlock;
> > +
> > +   get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> > +   atomic_set(&have_key, 1);
> > +
> > +unlock:
> > +   spin_unlock(&key_lock);
> > +   return 0;
> > +}
> 
> is this spinlock legal? what happens if we are getting interrupted by NMI?

I think we can do without the spinlock. I think I was already told that when
I tried to put it [some where else] in v1.

It's fun failing in public ;)

> printk()
>  vprintk_emit()
>   vscnprintf()
>pointer()
> ptr_to_id()
>  initialize_ptr_secret()
>   spin_lock(&key_lock)
> 
> > NMI
> 
>   printk()
>printk_safe_log_store()
> vscnprintf()
>  pointer()
>   ptr_to_id()
>initialize_ptr_secret()
> spin_lock(&key_lock)   <<<<
> 
> 
> or am I completely misreading the patch? sorry if so.
> 
>   -ss

thanks,
Tobin.

Re: [PATCH v3] staging: ccree: fix boolreturn.cocci warning

[Tobin C. Harding]

Hi Suniel,

Well done with you continued versions. I am being particularly nit picky here 
but since we are
striving for perfection I'm sure will humour me. If English is not your first 
language please
forgive me for picking you up on language subtleties.

On Wed, Oct 18, 2017 at 12:11:55PM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> This fixes the following coccinelle warning:
> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.

This should be a description of the problem, so saying _why_ there is a problem 
or _what_ is wrong
with the code currently that warrants a patch. Sometimes while describing the 
problem you may
include descriptions of the solution especially it is not immediately obvious 
why your proposed
solution fixes the issue being explained. As an extra we shouldn't ever say 
'This patch ...' or
'This does xyz'.

> return "false" instead of 0.

Perfect, this is in imperative mood. Spot on!

> Signed-off-by: Suniel Mahesh 
> ---
> Changes for v3:
> - Changed the commit log even more to give an accurate
>   description of the changeset as suggested by Toby C.Harding.

My name is Tobin :)

> ---
> Changes for v2:
> - Changed the commit log to give a more accurate description
>   of the changeset as suggested by Toby C.Harding.
> ---
> Note:
> - Patch was built(ARCH=arm) on latest linux-next.
> - No build issues reported, however it was not
>   tested on real hardware.
> - Please discard this changeset, if this is not
>   helping the code look better.
> ---
>  drivers/staging/ccree/ssi_cipher.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_cipher.h 
> b/drivers/staging/ccree/ssi_cipher.h
> index c9a83df..f499962 100644
> --- a/drivers/staging/ccree/ssi_cipher.h
> +++ b/drivers/staging/ccree/ssi_cipher.h
> @@ -75,7 +75,7 @@ struct arm_hw_key_info {
>  
>  static inline bool ssi_is_hw_key(struct crypto_tfm *tfm)
>  {
> - return 0;
> + return false;
>  }
>  
>  #endif /* CRYPTO_TFM_REQ_HW_KEY */
> -- 
> 1.9.1
> 

For what it's worth, Reviewed-by: Tobin C. Harding 

As stated I am being particularly 'nit picky', the commit log is _probably_ 
good enough to be
merged, I am not a maintainer though so it's not really anything to do with me. 
I do know however
that sometimes patches go to the bottom of Greg's list if they have 
comments/suggestions. I mention
this only so you learn more about the process and to help you with successfully 
getting you patches
merged. Keep up the work!

Good luck,
Tobin.

[PATCH v5] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as
follows (thanks to Joe Perches).

$ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
   1084 arch
 20 block
 10 crypto
 32 Documentation
   8121 drivers
   1221 fs
143 include
101 kernel
 69 lib
100 mm
   1510 net
 40 samples
  7 scripts
 11 security
166 sound
152 tools
  2 virt

Add function ptr_to_id() to map an address to a 32 bit unique identifier.

Signed-off-by: Tobin C. Harding 
---

V5:
 - Remove spin lock.
 - Add Jason A. Donenfeld to CC list by request.
 - Add Theodore Ts'o to CC list due to comment on previous version.

V4:
 - Remove changes to siphash.{ch}
 - Do word size check, and return value cast, directly in ptr_to_id().
 - Use add_ready_random_callback() to guard call to get_random_bytes()

V3:
 - Use atomic_xchg() to guard setting [random] key.
 - Remove erroneous white space change.

V2:
 - Use SipHash to do the hashing.

The discussion related to this patch has been fragmented. There are
three threads associated with this patch. Email threads by subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

 lib/vsprintf.c | 66 +++---
 1 file changed, 63 insertions(+), 3 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..14d4c6653384 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #ifdef CONFIG_BLOCK
 #include 
 #endif
@@ -1591,6 +1592,63 @@ char *device_node_string(char *buf, char *end, struct 
device_node *dn,
return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static siphash_key_t ptr_secret __read_mostly;
+static atomic_t have_key = ATOMIC_INIT(0);
+
+static void initialize_ptr_secret(void)
+{
+   if (atomic_read(&have_key) == 1)
+   return;
+
+   get_random_bytes(&ptr_secret, sizeof(ptr_secret));
+   atomic_set(&have_key, 1);
+}
+
+static void schedule_async_key_init(struct random_ready_callback *rdy)
+{
+   initialize_ptr_secret();
+}
+
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec 
spec)
+{
+   static struct random_ready_callback random_ready;
+   unsigned int hashval;
+   int err;
+
+   if (atomic_read(&have_key) == 0) {
+   random_ready.owner = NULL;
+   random_ready.func = schedule_async_key_init;
+
+   err = add_random_ready_callback(&random_ready);
+
+   switch (err) {
+   case 0:
+   return "(pointer value)";
+
+   case -EALREADY:
+   initialize_ptr_secret();
+   break;
+
+   default:
+   /* shouldn't get here */
+   return "(ptr_to_id() error)";
+   }
+   }
+
+#ifdef CONFIG_64BIT
+   hashval = (unsigned int)siphash_1u64((u64)ptr, &ptr_secret);
+#else
+   hashval = (unsigned int)siphash_1u32((u32)ptr, &ptr_secret);
+#endif
+
+   spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
+   spec.flags = SPECIAL | SMALL | ZEROPAD;
+   spec.base = 16;
+
+   return number(buf, end, hashval, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,6 +1761,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.
+ *
+ * Default behaviour (unadorned %p) is to hash the address, rendering it useful
+ * as a unique identifier.
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -1858,14 +1919,13 @@ char *pointer(const char *fmt, char *buf, char *end, 
void *ptr,
return device_node_string(buf, end, ptr, spec, fmt + 1);
}
}
-   spec.flags |= SMALL;
+
if (spec.field_width == -1) {
spec.field_width = default_width;
spec.flags |= ZEROPAD;
}
-   spec.base = 16;
 
-   return number(buf, end, (unsigned long) ptr, spec);
+   return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*
-- 
2.7.4

Re: [PATCH v5] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 03:31:16PM -0700, Kees Cook wrote:
> On Wed, Oct 18, 2017 at 2:30 PM, Tobin C. Harding  wrote:
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> 
> Is it intended for %pK to be covered by the hash as well? (When a
> disallowed user is looking at %pK output, like kallsyms, the same hash
> is seen for all values, rather than just zero -- I assume since the
> value hashed is zero.)

Good catch, thanks. Have fixed for v6, will wait 24 hours before submitting.

> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> >
> > For what it's worth, usage of unadorned %p can be broken down as
> > follows (thanks to Joe Perches).
> >
> > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
> >1084 arch
> >  20 block
> >  10 crypto
> >  32 Documentation
> >8121 drivers
> >1221 fs
> > 143 include
> > 101 kernel
> >  69 lib
> > 100 mm
> >1510 net
> >  40 samples
> >   7 scripts
> >  11 security
> > 166 sound
> > 152 tools
> >   2 virt
> >
> > Add function ptr_to_id() to map an address to a 32 bit unique identifier.
> >
> > Signed-off-by: Tobin C. Harding 
> > ---
> >
> > V5:
> >  - Remove spin lock.
> >  - Add Jason A. Donenfeld to CC list by request.
> >  - Add Theodore Ts'o to CC list due to comment on previous version.
> >
> > V4:
> >  - Remove changes to siphash.{ch}
> >  - Do word size check, and return value cast, directly in ptr_to_id().
> >  - Use add_ready_random_callback() to guard call to get_random_bytes()
> >
> > V3:
> >  - Use atomic_xchg() to guard setting [random] key.
> >  - Remove erroneous white space change.
> >
> > V2:
> >  - Use SipHash to do the hashing.
> >
> > The discussion related to this patch has been fragmented. There are
> > three threads associated with this patch. Email threads by subject:
> >
> > [PATCH] printk: hash addresses printed with %p
> > [PATCH 0/3] add %pX specifier
> > [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
> >
> >  lib/vsprintf.c | 66 
> > +++---
> >  1 file changed, 63 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 86c3385b9eb3..14d4c6653384 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -33,6 +33,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #ifdef CONFIG_BLOCK
> >  #include 
> >  #endif
> > @@ -1591,6 +1592,63 @@ char *device_node_string(char *buf, char *end, 
> > struct device_node *dn,
> > return widen_string(buf, buf - buf_start, end, spec);
> >  }
> >
> > +static siphash_key_t ptr_secret __read_mostly;
> > +static atomic_t have_key = ATOMIC_INIT(0);
> > +
> > +static void initialize_ptr_secret(void)
> > +{
> > +   if (atomic_read(&have_key) == 1)
> > +   return;
> > +
> > +   get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> > +   atomic_set(&have_key, 1);
> > +}
> > +
> > +static void schedule_async_key_init(struct random_ready_callback *rdy)
> > +{
> > +   initialize_ptr_secret();
> > +}
> > +
> > +/* Maps a pointer to a 32 bit unique identifier. */
> > +static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec 
> > spec)
> > +{
> > +   static struct random_ready_callback random_ready;
> > +   unsigned int hashval;
> > +   int err;
> > +
> > +   if (atomic_read(&have_key) == 0) {
> > +   random_ready.owner = NULL;
> > +   random_ready.func = schedule_async_key_init;
> > +
> > +   err = add_random_ready_callback(&random_ready);
> > +
> > +   switch (err) {
> > +   case 0:
> > +   return "(pointer value)";
> > +
> > +   case -EALREADY:
> > +   initialize_ptr_secret();
> >

Re: [PATCH v5] printk: hash addresses printed with %p

[Tobin C. Harding]

On Thu, Oct 19, 2017 at 03:36:20AM +0200, Jason A. Donenfeld wrote:
> On Thu, Oct 19, 2017 at 3:31 AM, Sergey Senozhatsky
>  wrote:
> > On (10/19/17 03:03), Jason A. Donenfeld wrote:
> > [..]
> >> 1) Go back to the spinlock yourself.
> >
> > so we ruled out NMI deadlocks?
> 
> Oh, right. No, I haven't thought through this enough to rule it out.
> Indeed if that's an issue, the locks in the _once code will also be an
> issue.
> 
> So if locking is totally impossible, then a race-free way of doing
> this is with a tri-state compare and exchange. Things are either: in
> state 1: no key, state 2: getting key, state 3: have key. If state 1
> or 2, print the placeholder token. If state 3, do the hashing.

Cool! That's the solution I've been looking for since day 1. You the man.

thanks,
Tobin.

[RFC] scripts: add leaking_addresses.pl

[Tobin C. Harding]

Currently we are leaking addresses from the kernel to user space. This
script as an attempt to find those leakages. Script parses `dmesg`
output and /proc and /sys files for suspicious entries.

Signed-off-by: Tobin C. Harding 
---

My usual disclaimer; I am a long way from being a Perl monger, any tips,
however trivial, most welcome.

Parses dmesg output first then;

Algorithm walks the directory tree of /proc and /sys, opens each file
for reading and parses file line by line. We therefore need to skip
certain files;

 - binary files.
 - relay large files of fixed format that _definitely_ won't leak.
 - non-readable files.

Since I do not know procfs or sysfs extensively I set `DEBUG = 1` within
the script (causes output of file name before parsing) and checked each
file it choked on. Obviously this means there are going to be a bunch of
other files not present on my system. Either more files to skip or a
suggestion of a better way to do this most appreciated.

Like I said, happy to take suggestions, abuse, tweaks etc

Thanks in advance for taking the time to look at this. Oh, I didn't
comment on my regex skills, no further comment required ;)

thanks,
Tobin.

 scripts/leaking_addresses.pl | 139 +++
 1 file changed, 139 insertions(+)
 create mode 100755 scripts/leaking_addresses.pl

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
new file mode 100755
index ..940547b716e3
--- /dev/null
+++ b/scripts/leaking_addresses.pl
@@ -0,0 +1,139 @@
+#!/usr/bin/env perl
+#
+# leaking_addresses.pl scan kernel for potential leaking addresses.
+
+use warnings;
+use strict;
+use File::Basename;
+use feature 'say';
+
+my $DEBUG = 0;
+my @dirs = ('/proc', '/sys');
+
+parse_dmesg();
+
+foreach(@dirs)
+{
+walk($_);
+}
+
+exit 0;
+
+#
+# TODO
+#
+# - Add support for 32 bit architectures.
+#
+sub may_leak_address
+{
+my $line = $_[0];
+my $regex = '[a-fA-F0-9]{12}';
+my $mask = '';
+
+if ($line =~ /$mask/) {
+return
+}
+
+if ($line =~ /$regex/) {
+return 1;
+}
+return;
+}
+
+sub parse_dmesg
+{
+my $line;
+open my $cmd, '-|', 'dmesg';
+while ($line = <$cmd>) {
+if (may_leak_address($line)) {
+print 'dmesg: ' . $line;
+}
+}
+close $cmd;
+}
+
+# We should skip these files
+sub skip_file
+{
+my $path = $_[0];
+
+my @skip_paths = ('/proc/kmsg', '/proc/kcore', '/proc/kallsyms',
+  '/proc/fs/ext4/sdb1/mb_groups', 
'/sys/kernel/debug/tracing/trace_pipe',
+  '/sys/kernel/security/apparmor/revision');
+my @skip_files = ('pagemap', 'events', 'access','registers', 
'snapshot_raw',
+  'trace_pipe_raw', 'trace_pipe');
+
+foreach(@skip_paths) {
+if ($_ eq $_[0]) {
+return 1;
+}
+}
+
+my($filename, $dirs, $suffix) = fileparse($path);
+
+foreach(@skip_files) {
+if ($_ eq $filename) {
+return 1;
+}
+}
+
+return;
+}
+
+sub parse_file
+{
+my $file = $_[0];
+
+if (! -R $file) {
+return;
+}
+
+if (skip_file($file)) {
+if ($DEBUG == 1) {
+print "skipping file: $file\n";
+}
+return;
+}
+if ($DEBUG == 1) {
+print "parsing $file\n";
+}
+
+open my $fh, $file or return;
+
+while( my $line = <$fh>)  {
+if (may_leak_address($line)) {
+print $file . ': ' . $line;
+}
+}
+
+close $fh;
+}
+
+# Recursively walk directory tree
+sub walk
+{
+my @dirs = ($_[0]);
+my %seen;
+
+while (my $pwd = shift @dirs) {
+if (!opendir(DIR,"$pwd")) {
+print STDERR "Cannot open $pwd\n";  
+next;
+} 
+my @files = readdir(DIR);
+closedir(DIR);
+foreach my $file (@files) {
+next if ($file eq '.' or $file eq '..');
+
+my $path = "$pwd/$file";
+next if (-l $path);
+
+if (-d $path and !$seen{$path}) {
+$seen{$path} = 1;
+push @dirs, "$path";
+} else {
+parse_file("$path");
+}
+}
+}
+}
-- 
2.7.4

Re: [RFC] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Thu, Oct 19, 2017 at 08:44:31AM -0400, Steven Rostedt wrote:
> On Thu, 19 Oct 2017 17:34:44 +1100
> "Tobin C. Harding"  wrote:
> 
> > 
> > My usual disclaimer; I am a long way from being a Perl monger, any tips,
> 
> I'm a semi Perl monger.
> 
> > however trivial, most welcome.
> > 
> > Parses dmesg output first then;
> > 
> > Algorithm walks the directory tree of /proc and /sys, opens each file
> > for reading and parses file line by line. We therefore need to skip
> > certain files;
> > 
> >  - binary files.
> >  - relay large files of fixed format that _definitely_ won't leak.
> 
> "relay large files"? What do the files relay with? ;-)

:) All bugs are shallow to enough eyes eh.

Good tips, thank you. V2 to come.

thanks,
Tobin.

Re: [RFC] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Thu, Oct 19, 2017 at 05:19:49PM +0200, Petr Mladek wrote:
> On Thu 2017-10-19 17:34:44, Tobin C. Harding wrote:
[snip]

Good tips, thank you. Will merge every ones suggestions. V2 to come.

thanks,
Tobin.

[PATCH V9] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as
follows (thanks to Joe Perches).

$ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
   1084 arch
 20 block
 10 crypto
 32 Documentation
   8121 drivers
   1221 fs
143 include
101 kernel
 69 lib
100 mm
   1510 net
 40 samples
  7 scripts
 11 security
166 sound
152 tools
  2 virt

Add function ptr_to_id() to map an address to a 32 bit unique
identifier. Hash any unadorned usage of specifier %p and any malformed
specifiers. 

Signed-off-by: Tobin C. Harding 

---

It seems we don't have consensus on a couple of things

1. The size of the hashed address on 64 bit architectures.
2. The use of '0x' pre-fix for hashed addresses.

In regards to (1), we are agreed that we only need 32 bits of
information. There is some questions however that outputting _only_ 32
bits may break userland.

In regards to (2), irrespective of the arguments for and against, if
point 1 is correct and changing the format will break userland then we
can't add the '0x' suffix for the same reason.

Therefore this patch masks off the first 32 bits, retaining
only 32 bits of information. We do not add a '0x' suffix. All in all,
that results in _no_ change to the format of output only the content of
the output.

The leading 0's also make explicit that we have messed with the address,
maybe this will save some debugging time by doing so. Although this
would probably already be obvious since there is no leading ''.

We hash malformed specifiers also. Malformed specifiers include
incomplete (e.g %pi) and also non-existent specifiers. checkpatch should
warn for non-existent specifiers but AFAICT won't warn for incomplete
specifiers.

Here is the behaviour that this patch implements.

For kpt_restrict==0

Randomness not ready:
  printed with %p:  (pointer value) # NOTE: with padding
Valid pointer:
  printed with %pK: deadbeefdeadbeef
  printed with %p:  deadbeef
  malformed specifier (eg %i):  deadbeef
NULL pointer:
  printed with %pK: 
  printed with %p:   (null) # NOTE: with padding
  malformed specifier (eg %i):   (null)

For kpt_restrict==2

Valid pointer:
  printed with %pK: 

All other output as for kptr_restrict==0

V9:
 - Drop the initial patch from V8, leaving null pointer handling as is.
 - Print the hashed ID _without_ a '0x' suffix.
 - Mask the first 32 bits of the hashed ID to all zeros on 64 bit
   architectures.

V8:
 - Add second patch cleaning up null pointer printing in pointer()
 - Move %pK handling to separate function, further cleaning up pointer()
 - Move ptr_to_id() call outside of switch statement making hashing
   the default behaviour (including malformed specifiers).
 - Remove use of static_key, replace with simple boolean.

V7:
 - Use tabs instead of spaces (ouch!).

V6:
 - Use __early_initcall() to fill the SipHash key.
 - Use static keys to guard hashing before the key is available.

V5:
 - Remove spin lock.
 - Add Jason A. Donenfeld to CC list by request.
 - Add Theodore Ts'o to CC list due to comment on previous version.

V4:
 - Remove changes to siphash.{ch}
 - Do word size check, and return value cast, directly in ptr_to_id().
 - Use add_ready_random_callback() to guard call to get_random_bytes()

V3:
 - Use atomic_xchg() to guard setting [random] key.
 - Remove erroneous white space change.

V2:
 - Use SipHash to do the hashing.

The discussion related to this patch has been fragmented. There are
three threads associated with this patch. Email threads by subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

 lib/vsprintf.c | 167 -
 1 file changed, 119 insertions(+), 48 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..0c9a008fc256 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #ifdef CONFIG_BLOCK
 #include 
 #endif
@@ -1344,6 +1346,57 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 }
 
 static noinline_for_stack
+char *kernel_pointer(char *buf, char *end, const void *ptr,
+struct printf_s

Re: [PATCH v7] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 04:22:44PM -0400, Steven Rostedt wrote:
> On Wed, 25 Oct 2017 14:49:34 +1100
> "Tobin C. Harding"  wrote:
> > 
> > First, the static_key stuff.
> > 
> > DEFINE_STATIC_KEY_TRUE(no_ptr_secret) : Doesn't sleep, just a
> > declaration. 
> > 
> > if (static_branch_unlikely(&no_ptr_secret)) {} : Doesn't sleep, just
> > some assembler to jump to returning true or false.
> > 
> > static_branch_disable(&no_ptr_secret) : Doesn't sleep, just atomic read
> > and set and maybe a WARN_ONCE.
> 
> How quickly do you need static_branch_disable() executed? You could
> always pass the work off to a worker thread (that can schedule).
> 
> random_ready_callback -> initiates worker thread -> enables the static branch
> 
> -- Steve

thanks Steve, v8 and onward does away with the static branch and uses a
global boolean instead.

thanks,
Tobin.

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 03:03:21PM -0700, Kees Cook wrote:
> On Wed, Oct 25, 2017 at 7:53 PM, Tobin C. Harding  wrote:
> > Here is the behaviour that this set implements.
> >
> > For kpt_restrict==0
> >
> > Randomness not ready:
> >   printed with %p:  (pointer)  # NOTE: with padding
> > Valid pointer:
> >   printed with %pK: deadbeefdeadbeef
> >   printed with %p:  0xdeadbeef
> >   malformed specifier (eg %i):  0xdeadbeef
> 
> I really think we can't include SPECIAL unless _every_ callsite of %p
> is actually doing "0x%p", and then we're replacing all of those. We're
> not doing that, though...
> 
> $ git grep '%p\b' | wc -l
> 12766
> $ git grep '0x%p\b' | wc -l
> 1837
> 
> If we need some kind of special marking that this is a hashed
> variable, that should be something other than "0x". If we're using the
> existing "(null)" and new "(pointer)" text, maybe "(hash:xx)"
> should be used instead? Then the (rare) callers with 0x become
> "0x(hash:)" and naked callers produce "(hash:)".
> 
> I think the first step for this is to just leave SPECIAL out.

Thanks Kees. V9 leaves SPECIAL out. Also V9 prints the whole 64 bit
address with the first 32 bits masked to zero. The intent being to _not_
change the output format from what it currently is. So it will look like
this; 

c09e81d0

What do you think?

Amusingly I think this whole conversation is going to come up again
when we do %pa, in inverse, since %pa currently does us SPECIAL.

thanks,
Tobin.

Re: [PATCH V8 2/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 05:33:22PM -0400, Steven Rostedt wrote:
> On Thu, 26 Oct 2017 13:58:38 +1100
> "Tobin C. Harding"  wrote:
> 
> > > +static bool have_filled_random_ptr_key;
> > > +static siphash_key_t ptr_key __read_mostly;
> > > +
> > > +static void fill_random_ptr_key(struct random_ready_callback *unused)
> > > +{
> > > + get_random_bytes(&ptr_key, sizeof(ptr_key));
> > > + WRITE_ONCE(have_filled_random_ptr_key, true);  
> > 
> > This usage of WRITE_ONCE was suggested by Jason A. Donenfeld. I read
> > include/linux/compiler.h but was not able to grok it. Is this enough to
> > stop the compiler re-ordering these two statements? 
> > 
> > Or do I need to read Documentation/memory-barriers.txt [again]?
> 
> No, the WRITE_ONCE does not stop the compiler from reordering those
> statements. If you need that, then you need to do:
> 
>   get_random_bytes(&ptr_key, sizeof(ptr_key));
>   barrier();
>   WRITE_ONCE(have_filled_random_ptr_key, true);
> 
> and that only works against interrupts. If you need synchronization
> across CPUs, then you need smp_mb().

Cool. So I think we need

get_random_bytes(&ptr_key, sizeof(ptr_key));
smp_mb();
WRITE_ONCE(have_filled_random_ptr_key, true);

V10 to include this unless I have it wrong.

thanks,
Tobin.

Re: [PATCH V9] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 03:31:41PM -0700, Kees Cook wrote:
> On Sun, Oct 29, 2017 at 3:59 PM, Tobin C. Harding  wrote:
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> >
> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> >
> > For what it's worth, usage of unadorned %p can be broken down as
> > follows (thanks to Joe Perches).
> >
> > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
> >1084 arch
> >  20 block
> >  10 crypto
> >  32 Documentation
> >8121 drivers
> >1221 fs
> > 143 include
> > 101 kernel
> >  69 lib
> > 100 mm
> >1510 net
> >  40 samples
> >   7 scripts
> >  11 security
> > 166 sound
> > 152 tools
> >   2 virt
> >
> > Add function ptr_to_id() to map an address to a 32 bit unique
> > identifier. Hash any unadorned usage of specifier %p and any malformed
> > specifiers.
> >
> > Signed-off-by: Tobin C. Harding 
> >
> > ---
> >
> > It seems we don't have consensus on a couple of things
> >
> > 1. The size of the hashed address on 64 bit architectures.
> > 2. The use of '0x' pre-fix for hashed addresses.
> >
> > In regards to (1), we are agreed that we only need 32 bits of
> > information. There is some questions however that outputting _only_ 32
> > bits may break userland.
> >
> > In regards to (2), irrespective of the arguments for and against, if
> > point 1 is correct and changing the format will break userland then we
> > can't add the '0x' suffix for the same reason.
> >
> > Therefore this patch masks off the first 32 bits, retaining
> > only 32 bits of information. We do not add a '0x' suffix. All in all,
> > that results in _no_ change to the format of output only the content of
> > the output.
> >
> > The leading 0's also make explicit that we have messed with the address,
> > maybe this will save some debugging time by doing so. Although this
> > would probably already be obvious since there is no leading ''.
> >
> > We hash malformed specifiers also. Malformed specifiers include
> > incomplete (e.g %pi) and also non-existent specifiers. checkpatch should
> > warn for non-existent specifiers but AFAICT won't warn for incomplete
> > specifiers.
> >
> > Here is the behaviour that this patch implements.
> >
> > For kpt_restrict==0
> >
> > Randomness not ready:
> >   printed with %p:  (pointer value) # NOTE: with padding
> > Valid pointer:
> >   printed with %pK: deadbeefdeadbeef
> >   printed with %p:  deadbeef
> >   malformed specifier (eg %i):  deadbeef
> > NULL pointer:
> >   printed with %pK: 
> >   printed with %p:   (null) # NOTE: with padding
> >   malformed specifier (eg %i):   (null)
> >
> > For kpt_restrict==2
> >
> > Valid pointer:
> >   printed with %pK: 
> >
> > All other output as for kptr_restrict==0
> >
> > V9:
> >  - Drop the initial patch from V8, leaving null pointer handling as is.
> >  - Print the hashed ID _without_ a '0x' suffix.
> >  - Mask the first 32 bits of the hashed ID to all zeros on 64 bit
> >architectures.
> 
> Oops, I had missed v9. This addresses my concerns. I think the leading
> zeros are a good way to identify the "this is clearly not a kernel
> address" issue (though the 32-bit folks may remain confused, but we
> can fix that later, IMO).

Awesome. Yeah this patch (coupled with the leaking_addresses.pl script)
is turning out to be a bit 64-bit centric. However, as we plug more
leaks in 64-bit kernels hopefully they will be plugged in 32-bit ones
too.

I can't think of any way to have leaking_addresses.pl grep for 32-bit
addresses, especially once/if this patch gets merged. We will not be
able to differentiate between hashed addresses and real addresses on
32-bit machines.

thanks,
Tobin.

Re: [PATCH V8 2/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 08:00:46PM -0400, Steven Rostedt wrote:
> On Tue, 31 Oct 2017 09:41:02 +1100
> "Tobin C. Harding"  wrote:
> 
> 
> > Cool. So I think we need
> > 
> > get_random_bytes(&ptr_key, sizeof(ptr_key));
> 
> You'll need to add a comment here to describe what ordering the memory
> barrier is used against. That is, somewhere else there's something that
> needs to see the "true" after the "get_random_bytes" update. Describe
> it here.

Got it, thanks Tobin.

Re: [lkp-robot] [printk] 7f7c60e066: BUG:KASAN:slab-out-of-bounds

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 05:14:59PM -0700, Kees Cook wrote:
> On Wed, Oct 25, 2017 at 9:22 AM, kernel test robot
>  wrote:

thanks for looking at this, I was at a loss as to what (if any) action I
needed to take.

> > FYI, we noticed the following commit (built with gcc-4.9):
> >
> > commit: 7f7c60e0663645e757e520245606fde9c6e326bb ("printk: hash addresses 
> > printed with %p")
> > url: 
> > https://github.com/0day-ci/linux/commits/Tobin-C-Harding/printk-hash-addresses-printed-with-p/20171024-231922
> 
> It's not clear to me which of the various versions this test ran
> against, but it seems like the printf self-tests got very confused by
> the results:
> 
> > [   40.275423] test_printf: kvasprintf(..., "%p %p", ...) returned 
> > '3cf9adbe eff717bf', expected '01234567 fedcba98'
> > [   40.296739] test_printf: vsnprintf(buf, 256, "|%-*p|%*p|", ...) returned 
> > 19, expected 39
> > [   40.322776] test_printf: vsnprintf(buf, 16, "|%-*p|%*p|", ...) returned 
> > 19, expected 39
> > [   40.334834] test_printf: vsnprintf(buf, 0, "|%-*p|%*p|", ...) returned 
> > 19, expected 39
> 
> I assume v10 will fix the width issues, but probably not the value tests...

Oh, so I need to update lib/test_printf.c to cover hashed %p.

> And it claims a use-after-free, too:
> 
> > [   39.757461] The buggy address belongs to the object at 22cb34bb
> > [   39.757461]  which belongs to the cache kmalloc-32 of size 32
> > [   39.757461] The buggy address is located 0 bytes inside of
> > [   39.757461]  32-byte region [22cb34bb, 24ac3a60)
> 
> Which becomes rather unreadable, since the address got hashed. :P

So I think we need to patch mm/kasan/report to use %pK instead of %p.

I don't know what I should be doing about

[   39.757461] BUG: KASAN: slab-out-of-bounds in __test+0xee/0x13f

Awesome, thanks,
Tobin.

Re: [PATCH bpf-next 05/13] docs: net: Fix indentation issues for code snippets

[Tobin C. Harding]

On Fri, Aug 03, 2018 at 10:44:23AM +0200, Daniel Borkmann wrote:
> On 08/01/2018 07:09 AM, Tobin C. Harding wrote:
> [...]
> > -Starting bpf_dbg is trivial and just requires issuing:
> > +Starting bpf_dbg is trivial and just requires issuing::
> >  
> > -# ./bpf_dbg
> > +  # ./bpf_dbg
> >  
> >  In case input and output do not equal stdin/stdout, bpf_dbg takes an
> >  alternative stdin source as a first argument, and an alternative stdout
> > @@ -381,86 +384,87 @@ file "~/.bpf_dbg_init" and the command history is 
> > stored in the file
> >  Interaction in bpf_dbg happens through a shell that also has 
> > auto-completion
> >  support (follow-up example commands starting with '>' denote bpf_dbg 
> > shell).
> >  The usual workflow would be to ...
> > -
> > -> load bpf 6,40 0 0 12,21 0 3 2048,48 0 0 23,21 0 1 1,6 0 0 65535,6 0 0 0
> > -  Loads a BPF filter from standard output of bpf_asm, or transformed via
> > -  e.g. `tcpdump -iem1 -ddd port 22 | tr '\n' ','`. Note that for JIT
> > -  debugging (next section), this command creates a temporary socket and
> > -  loads the BPF code into the kernel. Thus, this will also be useful for
> > -  JIT developers.
> > -
> > -> load pcap foo.pcap
> > -  Loads standard tcpdump pcap file.
> > -
> > -> run []
> > -bpf passes:1 fails:9
> > -  Runs through all packets from a pcap to account how many passes and fails
> > -  the filter will generate. A limit of packets to traverse can be given.
> > -
> > -> disassemble
> > -l0:ldh [12]
> > -l1:jeq #0x800, l2, l5
> > -l2:ldb [23]
> > -l3:jeq #0x1, l4, l5
> > -l4:ret #0x
> > -l5:ret #0
> > -  Prints out BPF code disassembly.
> > -
> > -> dump
> > -/* { op, jt, jf, k }, */
> > -{ 0x28,  0,  0, 0x000c },
> > -{ 0x15,  0,  3, 0x0800 },
> > -{ 0x30,  0,  0, 0x0017 },
> > -{ 0x15,  0,  1, 0x0001 },
> > -{ 0x06,  0,  0, 0x },
> > -{ 0x06,  0,  0, 00 },
> > -  Prints out C-style BPF code dump.
> > -
> > -> breakpoint 0
> > -breakpoint at: l0: ldh [12]
> > -> breakpoint 1
> > -breakpoint at: l1: jeq #0x800, l2, l5
> > -  ...
> > -  Sets breakpoints at particular BPF instructions. Issuing a `run` command
> > -  will walk through the pcap file continuing from the current packet and
> > -  break when a breakpoint is being hit (another `run` will continue from
> > -  the currently active breakpoint executing next instructions):
> > -
> > -  > run
> > -  -- register dump --
> > -  pc:   [0]   <-- program counter
> > -  code: [40] jt[0] jf[0] k[12]<-- plain BPF code of current 
> > instruction
> > -  curr: l0:ldh [12]  <-- disassembly of current 
> > instruction
> > -  A:[][0] <-- content of A (hex, decimal)
> > -  X:[][0] <-- content of X (hex, decimal)
> > -  M[0,15]:  [][0] <-- folded content of M (hex, 
> > decimal)
> > -  -- packet dump --   <-- Current packet from pcap (hex)
> > -  len: 42
> > -0: 00 19 cb 55 55 a4 00 14 a4 43 78 69 08 06 00 01
> > -   16: 08 00 06 04 00 01 00 14 a4 43 78 69 0a 3b 01 26
> > -   32: 00 00 00 00 00 00 0a 3b 01 01
> > -  (breakpoint)
> > -  >
> > -
> > -> breakpoint
> > -breakpoints: 0 1
> > -  Prints currently set breakpoints.
> > -
> > -> step [-, +]
> > -  Performs single stepping through the BPF program from the current pc
> > -  offset. Thus, on each step invocation, above register dump is issued.
> > -  This can go forwards and backwards in time, a plain `step` will break
> > -  on the next BPF instruction, thus +1. (No `run` needs to be issued here.)
> > -
> > -> select 
> > -  Selects a given packet from the pcap file to continue from. Thus, on
> > -  the next `run` or `step`, the BPF program is being evaluated against
> > -  the user pre-selected packet. Numbering starts just as in Wireshark
> > -  with index 1.
> > -
> > -> quit
> > -#
> > -  Exits bpf_dbg.
> > +::
> > +
> > +  > load bpf 6,40 0 0 12,21 0 3 2048,48 0 0 23,21 0 1 1,6 0 0 65535,6 0 0 0
> > +Loads a BPF filter from standard output of bpf_asm, or transformed via
> > +e.g. `tcpdump -iem1 -ddd port 22 | tr '\n' ','`. Note that for JIT
> > +debugging (next section), this command creates a temporary socket and
> > +loads the BPF code into the kernel. Thus, this will also be useful for
> > +JIT developers.
> 
> Here for the bpf_dbg howto, it would be good to separate explanation from
> the cmdline code snippets. This would more easily clarify the commands
> themselves if we already go the rst route, so I'd prefer splitting this up.

Sure thing.  Will do as suggested. Thanks for the review.


Tobin

Re: [PATCH bpf-next 12/13] docs: net: Fix various minor typos

[Tobin C. Harding]

On Fri, Aug 03, 2018 at 10:41:12AM +0200, Daniel Borkmann wrote:
> On 08/01/2018 07:09 AM, Tobin C. Harding wrote:
> > There are a few minor typos and grammatical issues.  We should however
> > try to keep the current flavour of the document.
> > 
> > Fix typos and grammar if glaringly required.
> > 
> > Signed-off-by: Tobin C. Harding 
> 
> Overall looks good, just some minor nits:
> 
> >  Documentation/networking/filter.rst | 65 +++--
> >  1 file changed, 33 insertions(+), 32 deletions(-)
> > 
> > diff --git a/Documentation/networking/filter.rst 
> > b/Documentation/networking/filter.rst
> > index 99dfa74fc4f7..b989a6c882b8 100644
> > --- a/Documentation/networking/filter.rst
> > +++ b/Documentation/networking/filter.rst
> > @@ -32,10 +32,10 @@ removing the old one and placing your new one in its 
> > place, assuming your
> >  filter has passed the checks, otherwise if it fails the old filter will
> >  remain on that socket.
> >  
> > -SO_LOCK_FILTER option allows to lock the filter attached to a socket. Once
> > -set, a filter cannot be removed or changed. This allows one process to
> > +SO_LOCK_FILTER option allows locking of the filter attached to a socket.
> > +Once set, a filter cannot be removed or changed. This allows one process to
> >  setup a socket, attach a filter, lock it then drop privileges and be
> > -assured that the filter will be kept until the socket is closed.
> > +assured  that the filter will be kept until the socket is closed.
> 
>   ^-- looks like extra whitespace slipped in?

Fixed in the RFC.  Thanks for taking the time review.

Tobin

Re: [PATCH v8 4/4] vsprintf: Add command line option debug_boot_weak_hash

[Tobin C. Harding]

On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
> > Currently printing [hashed] pointers requires enough entropy to be
> > available.  Early in the boot sequence this may not be the case
> > resulting in a dummy string '(ptrval)' being printed.  This
> > makes debugging the early boot sequence difficult.  We can relax the
> > requirement to use cryptographically secure hashing during debugging.
> > This enables debugging while keeping development/production kernel
> > behaviour the same.
> > 
> > If new command line option debug_boot_weak_hash is enabled use
> > cryptographically insecure hashing and hash pointer value immediately.
> > 
> > Signed-off-by: Tobin C. Harding 
> > Reviewed-by: Steven Rostedt (VMware) 
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |  9 +
> >  lib/vsprintf.c  | 17 +
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> > b/Documentation/admin-guide/kernel-parameters.txt
> > index 638342d0a095..a116fc0366b0 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -748,6 +748,15 @@
> >  
> > debug   [KNL] Enable kernel debugging (events log level).
> >  
> > +   debug_boot_weak_hash
> > +   [KNL] Enable printing pointers early in the boot
> > +   sequence.  If enabled, we use a weak hash instead of
> > +   siphash to hash pointers.  Use this option if you need
> > +   to see pointer values during early boot (i.e you are
> 
> maybe:
>   to see hashed pointer values
> i.e., not raw pointers.

You cannot see 'raw pointers' anyways?

> 
> > +   seeing instances of '(___ptrval___)').
> > +   Cryptographically insecure, please do not use on
> > +   production kernels.

thanks for the review, I don't quiet see how to use your suggestion to
make the text clearer.  If you still feel this change is needed perhaps
you could write so I understand i.e 'Use this option if ...'


thanks,
Tobin.

Re: [PATCH v8 4/4] vsprintf: Add command line option debug_boot_weak_hash

[Tobin C. Harding]

On Wed, Jun 20, 2018 at 03:36:44PM -0700, Randy Dunlap wrote:
> On 06/20/2018 03:30 PM, Tobin C. Harding wrote:
> > On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
> >> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
> >>> Currently printing [hashed] pointers requires enough entropy to be
> >>> available.  Early in the boot sequence this may not be the case
> >>> resulting in a dummy string '(ptrval)' being printed.  This
> >>> makes debugging the early boot sequence difficult.  We can relax the
> >>> requirement to use cryptographically secure hashing during debugging.
> >>> This enables debugging while keeping development/production kernel
> >>> behaviour the same.
> >>>
> >>> If new command line option debug_boot_weak_hash is enabled use
> >>> cryptographically insecure hashing and hash pointer value immediately.
> >>>
> >>> Signed-off-by: Tobin C. Harding 
> >>> Reviewed-by: Steven Rostedt (VMware) 
> >>> ---
> >>>  Documentation/admin-guide/kernel-parameters.txt |  9 +
> >>>  lib/vsprintf.c  | 17 +
> >>>  2 files changed, 26 insertions(+)
> >>>
> >>> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> >>> b/Documentation/admin-guide/kernel-parameters.txt
> >>> index 638342d0a095..a116fc0366b0 100644
> >>> --- a/Documentation/admin-guide/kernel-parameters.txt
> >>> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >>> @@ -748,6 +748,15 @@
> >>>  
> >>>   debug   [KNL] Enable kernel debugging (events log level).
> >>>  
> >>> + debug_boot_weak_hash
> >>> + [KNL] Enable printing pointers early in the boot
> >>> + sequence.  If enabled, we use a weak hash instead of
> >>> + siphash to hash pointers.  Use this option if you need
> >>> + to see pointer values during early boot (i.e you are
> >>
> >> maybe:
> >>to see hashed pointer values
> >> i.e., not raw pointers.
> > 
> > You cannot see 'raw pointers' anyways?
> 
> only if using %px ?
> 
> Maybe it's just terminology.  I don't consider a hashed value as a pointer 
> value.
> It's just a key or handle or some other number, but it's not a pointer.
> 
> >>
> >>> + seeing instances of '(___ptrval___)').
> >>> + Cryptographically insecure, please do not use on
> >>> + production kernels.
> > 
> > thanks for the review, I don't quiet see how to use your suggestion to
> > make the text clearer.  If you still feel this change is needed perhaps
> > you could write so I understand i.e 'Use this option if ...'
> 
> 
> OK, if you are good with it, I am too.  :)

I get you know.  I agree, how about this

[KNL] Enable printing pointers early in the boot
sequence.  If enabled, we use a weak hash instead of
siphash to hash pointers.  Use this option if you need
to print pointers with %px during early boot
(i.e you are seeing instances of '(___ptrval___)').
Cryptographically insecure, please do not use on
production kernels.


thanks for clarifying,
Tobin.

Re: [PATCH v8 4/4] vsprintf: Add command line option debug_boot_weak_hash

[Tobin C. Harding]

On Wed, Jun 20, 2018 at 04:38:05PM -0700, Randy Dunlap wrote:
> On 06/20/2018 04:22 PM, Tobin C. Harding wrote:
> > On Wed, Jun 20, 2018 at 03:36:44PM -0700, Randy Dunlap wrote:
> >> On 06/20/2018 03:30 PM, Tobin C. Harding wrote:
> >>> On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
> >>>> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
> >>>>> Currently printing [hashed] pointers requires enough entropy to be
> >>>>> available.  Early in the boot sequence this may not be the case
> >>>>> resulting in a dummy string '(ptrval)' being printed.  This
> >>>>> makes debugging the early boot sequence difficult.  We can relax the
> >>>>> requirement to use cryptographically secure hashing during debugging.
> >>>>> This enables debugging while keeping development/production kernel
> >>>>> behaviour the same.
> >>>>>
> >>>>> If new command line option debug_boot_weak_hash is enabled use
> >>>>> cryptographically insecure hashing and hash pointer value immediately.
> >>>>>
> >>>>> Signed-off-by: Tobin C. Harding 
> >>>>> Reviewed-by: Steven Rostedt (VMware) 
> >>>>> ---
> >>>>>  Documentation/admin-guide/kernel-parameters.txt |  9 +
> >>>>>  lib/vsprintf.c  | 17 +
> >>>>>  2 files changed, 26 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> >>>>> b/Documentation/admin-guide/kernel-parameters.txt
> >>>>> index 638342d0a095..a116fc0366b0 100644
> >>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
> >>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >>>>> @@ -748,6 +748,15 @@
> >>>>>  
> >>>>> debug   [KNL] Enable kernel debugging (events log 
> >>>>> level).
> >>>>>  
> >>>>> +   debug_boot_weak_hash
> >>>>> +   [KNL] Enable printing pointers early in the boot
> >>>>> +   sequence.  If enabled, we use a weak hash 
> >>>>> instead of
> >>>>> +   siphash to hash pointers.  Use this option if 
> >>>>> you need
> >>>>> +   to see pointer values during early boot (i.e 
> >>>>> you are
> >>>>
> >>>> maybe:
> >>>>  to see hashed pointer values
> >>>> i.e., not raw pointers.
> >>>
> >>> You cannot see 'raw pointers' anyways?
> >>
> >> only if using %px ?
> >>
> >> Maybe it's just terminology.  I don't consider a hashed value as a pointer 
> >> value.
> >> It's just a key or handle or some other number, but it's not a pointer.
> >>
> >>>>
> >>>>> +   seeing instances of '(___ptrval___)').
> >>>>> +   Cryptographically insecure, please do not use on
> >>>>> +   production kernels.
> >>>
> >>> thanks for the review, I don't quiet see how to use your suggestion to
> >>> make the text clearer.  If you still feel this change is needed perhaps
> >>> you could write so I understand i.e 'Use this option if ...'
> >>
> >>
> >> OK, if you are good with it, I am too.  :)
> > 
> > I get you know.  I agree, how about this
> > 
> > [KNL] Enable printing pointers early in the boot
> > sequence.  If enabled, we use a weak hash instead of
> > siphash to hash pointers.  Use this option if you need
> > to print pointers with %px during early boot
> > (i.e you are seeing instances of '(___ptrval___)').
> > Cryptographically insecure, please do not use on
> > production kernels.
> 
> Sorry, I'm still confused by this paragraph.  It seems to say two different
> things.

My bad, I got totally confused myself.  After all this time you would
think I knew which specifier hashed and which didn't.  My apologies,
how about this:

[KNL] Enable printing [hashed] pointers early in
the boot sequence.  If enabled, we use a weak hash
instead of siphash to hash pointers.  Use this option if
you are seeing instances of '(___ptrval___)') and need
to see a value (hashed pointer) instead. 
Cryptographically
insecure, please do not use on production kernels.


thanks for your patience,
Tobin.

Re: [PATCH v8 4/4] vsprintf: Add command line option debug_boot_weak_hash

[Tobin C. Harding]

On Wed, Jun 20, 2018 at 09:09:49PM -0700, Randy Dunlap wrote:
> On 06/20/2018 08:15 PM, Tobin C. Harding wrote:
> > On Wed, Jun 20, 2018 at 04:38:05PM -0700, Randy Dunlap wrote:
> >> On 06/20/2018 04:22 PM, Tobin C. Harding wrote:
> >>> On Wed, Jun 20, 2018 at 03:36:44PM -0700, Randy Dunlap wrote:
> >>>> On 06/20/2018 03:30 PM, Tobin C. Harding wrote:
> >>>>> On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
> >>>>>> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
> >>>>>>> Currently printing [hashed] pointers requires enough entropy to be
> >>>>>>> available.  Early in the boot sequence this may not be the case
> >>>>>>> resulting in a dummy string '(ptrval)' being printed.  This
> >>>>>>> makes debugging the early boot sequence difficult.  We can relax the
> >>>>>>> requirement to use cryptographically secure hashing during debugging.
> >>>>>>> This enables debugging while keeping development/production kernel
> >>>>>>> behaviour the same.
> >>>>>>>
> >>>>>>> If new command line option debug_boot_weak_hash is enabled use
> >>>>>>> cryptographically insecure hashing and hash pointer value immediately.
> >>>>>>>
> >>>>>>> Signed-off-by: Tobin C. Harding 
> >>>>>>> Reviewed-by: Steven Rostedt (VMware) 
> >>>>>>> ---
> >>>>>>>  Documentation/admin-guide/kernel-parameters.txt |  9 +
> >>>>>>>  lib/vsprintf.c  | 17 
> >>>>>>> +
> >>>>>>>  2 files changed, 26 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> >>>>>>> b/Documentation/admin-guide/kernel-parameters.txt
> >>>>>>> index 638342d0a095..a116fc0366b0 100644
> >>>>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
> >>>>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >>>>>>> @@ -748,6 +748,15 @@
> >>>>>>>  
> >>>>>>>   debug   [KNL] Enable kernel debugging (events log 
> >>>>>>> level).
> >>>>>>>  
> >>>>>>> + debug_boot_weak_hash
> >>>>>>> + [KNL] Enable printing pointers early in the boot
> >>>>>>> + sequence.  If enabled, we use a weak hash 
> >>>>>>> instead of
> >>>>>>> + siphash to hash pointers.  Use this option if 
> >>>>>>> you need
> >>>>>>> + to see pointer values during early boot (i.e 
> >>>>>>> you are
> >>>>>>
> >>>>>> maybe:
> >>>>>>to see hashed pointer values
> >>>>>> i.e., not raw pointers.
> >>>>>
> >>>>> You cannot see 'raw pointers' anyways?
> >>>>
> >>>> only if using %px ?
> >>>>
> >>>> Maybe it's just terminology.  I don't consider a hashed value as a 
> >>>> pointer value.
> >>>> It's just a key or handle or some other number, but it's not a pointer.
> >>>>
> >>>>>>
> >>>>>>> + seeing instances of '(___ptrval___)').
> >>>>>>> + Cryptographically insecure, please do not use on
> >>>>>>> + production kernels.
> >>>>>
> >>>>> thanks for the review, I don't quiet see how to use your suggestion to
> >>>>> make the text clearer.  If you still feel this change is needed perhaps
> >>>>> you could write so I understand i.e 'Use this option if ...'
> >>>>
> >>>>
> >>>> OK, if you are good with it, I am too.  :)
> >>>
> >>> I get you know.  I agree, how about this
> >>>
> >>>   [KNL] Enable printing pointers early in the boot
> >>>   sequence.  If enabled, we use a weak hash instead of
> >>>   siphash to hash pointers.  Use this option if you need
> >>>   to print pointers with %px during early boot
> >>>   (i.e you are seeing instances of '(___ptrval___)').
> >>>   Cryptographically insecure, please do not use on
> >>>   production kernels.
> >>
> >> Sorry, I'm still confused by this paragraph.  It seems to say two different
> >> things.
> > 
> > My bad, I got totally confused myself.  After all this time you would
> > think I knew which specifier hashed and which didn't.  My apologies,
> > how about this:
> > 
> > [KNL] Enable printing [hashed] pointers early in
> > the boot sequence.  If enabled, we use a weak hash
> > instead of siphash to hash pointers.  Use this option if
> > you are seeing instances of '(___ptrval___)') and need
> > to see a value (hashed pointer) instead. 
> > Cryptographically
> > insecure, please do not use on production kernels.
> > 
> > 
> > thanks for your patience,
> > Tobin.
> 
> Yes, that's good.  Thanks.

Awesome, v9 on it's way :)

thanks,
Tobin.

[PATCH v9 0/4] enable early printing of hashed pointers

[Tobin C. Harding]

Hi,

Here is v9, only change is to update kernel parameters documentation
after discussion on LKML with Randy. (And add a full stop to comment
string.)


Currently printing pointers early in the boot sequence can result in a
dummy string '(ptrval)' being printed.  While resolving this
issue it was noticed that we can use the hw RNG if available for hashing
pointers.

Patch one and two do the ground work to be able to use hw RNG removing
from get_random_bytes_arch() the call to get_random_bytes() and
returning the number of bytes of random material successfully returned. 

Patch three uses the hw RNG to get keying material if it is available.

Patch four further assists debugging early in the boot sequence for
machines that do not have a hw RNG by adding a command line option
'debug_boot_weak_hash'.  If enabled, non-cryptographically secure hashing
is used instead of siphash so we can hash at any time. 


thanks,
Tobin.

v9
 - Improve documentation text of new kernel parameter
   debug_boot_weak_hash (thanks Randy).
 - Add full stop to comment string.

v8
 - Remove pointless EXPORT_SYMBOL on static variable (thanks Steve).
 - Remove unnecessary integer cast from min_t() argument (thanks Andy).

v7
 - Remove unused variable, clearing compiler warning (found by Stephen
   Rothwell's linux-next build infrastructure).

v6
 - Rebase on top of Steve's patch (fixing race condition).  Uses static
   branch instead of memory barrier.

v5
 - Use 'upside-down-xmas-tree' style to declare local variables (Steve)
 - Added Reviewed-by tag from Steve (patch 2 and 3).

v4
 - remove last patch of series (command line option patch)

v3
 - Add __ro_after_init (suggested by Kees).

v2
 - Use min_t() instead of min() (thanks checkpatch).
 - Add __must_check to function declaration (thanks Steve).
 - Use hw RNG by default if available (as originally suggested by Kees).
 - Add command line option to use cryptographically insecure hashing.
   If debug_early_boot is enabled use hash_long() instead of siphash
   (as requested by Steve, and solves original problem for Anna-Maria).
 - Added Acked-by tag from Ted (patch 1 and 2)


Tobin C. Harding (4):
  random: Fix whitespace pre random-bytes work
  random: Return nbytes filled from hw RNG
  vsprintf: Use hw RNG for ptr_key
  vsprintf: Add command line option debug_boot_weak_hash

 .../admin-guide/kernel-parameters.txt |  8 ++
 drivers/char/random.c | 19 ++---
 include/linux/random.h|  2 +-
 lib/vsprintf.c| 27 ++-
 4 files changed, 45 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH v9 1/4] random: Fix whitespace pre random-bytes work

[Tobin C. Harding]

There are a couple of whitespace issues around the function
get_random_bytes_arch().  In preparation for patching this function
let's clean them up.

Acked-by: Theodore Ts'o 
Signed-off-by: Tobin C. Harding 
---
 drivers/char/random.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index a8fb0020ba5c..ed679099afba 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1736,7 +1736,7 @@ void get_random_bytes_arch(void *buf, int nbytes)
 
if (!arch_get_random_long(&v))
break;
-   
+
memcpy(p, &v, chunk);
p += chunk;
nbytes -= chunk;
@@ -1747,7 +1747,6 @@ void get_random_bytes_arch(void *buf, int nbytes)
 }
 EXPORT_SYMBOL(get_random_bytes_arch);
 
-
 /*
  * init_std_data - initialize pool with system data
  *
-- 
2.17.1

[PATCH v9 3/4] vsprintf: Use hw RNG for ptr_key

[Tobin C. Harding]

Currently we must wait for enough entropy to become available before
hashed pointers can be printed.  We can remove this wait by using the
hw RNG if available.

Use hw RNG to get keying material.

Reviewed-by: Steven Rostedt (VMware) 
Suggested-by: Kees Cook 
Signed-off-by: Tobin C. Harding 
---
 lib/vsprintf.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a48aaa79d352..6c1fb395bddf 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1675,8 +1675,16 @@ static struct random_ready_callback random_ready = {
 
 static int __init initialize_ptr_random(void)
 {
-   int ret = add_random_ready_callback(&random_ready);
+   int key_size = sizeof(ptr_key);
+   int ret;
+
+   /* Use hw RNG if available. */
+   if (get_random_bytes_arch(&ptr_key, key_size) == key_size) {
+   static_branch_disable(¬_filled_random_ptr_key);
+   return 0;
+   }
 
+   ret = add_random_ready_callback(&random_ready);
if (!ret) {
return 0;
} else if (ret == -EALREADY) {
-- 
2.17.1

[PATCH v9 4/4] vsprintf: Add command line option debug_boot_weak_hash

[Tobin C. Harding]

Currently printing [hashed] pointers requires enough entropy to be
available.  Early in the boot sequence this may not be the case
resulting in a dummy string '(ptrval)' being printed.  This
makes debugging the early boot sequence difficult.  We can relax the
requirement to use cryptographically secure hashing during debugging.
This enables debugging while keeping development/production kernel
behaviour the same.

If new command line option debug_boot_weak_hash is enabled use
cryptographically insecure hashing and hash pointer value immediately.

Reviewed-by: Steven Rostedt (VMware) 
Signed-off-by: Tobin C. Harding 
---
 Documentation/admin-guide/kernel-parameters.txt |  8 
 lib/vsprintf.c  | 17 +
 2 files changed, 25 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 638342d0a095..c47b5efbef82 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -748,6 +748,14 @@
 
debug   [KNL] Enable kernel debugging (events log level).
 
+   debug_boot_weak_hash
+   [KNL] Enable printing [hashed] pointers early in the
+   boot sequence.  If enabled, we use a weak hash instead
+   of siphash to hash pointers.  Use this option if you are
+   seeing instances of '(___ptrval___)') and need to see a
+   value (hashed pointer) instead. Cryptographically
+   insecure, please do not use on production kernels.
+
debug_locks_verbose=
[KNL] verbose self-tests
Format=<0|1>
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6c1fb395bddf..1ee2829f3b54 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1651,6 +1651,17 @@ char *device_node_string(char *buf, char *end, struct 
device_node *dn,
return widen_string(buf, buf - buf_start, end, spec);
 }
 
+/* Make pointers available for printing early in the boot sequence. */
+static int debug_boot_weak_hash __ro_after_init;
+
+static int __init debug_boot_weak_hash_enable(char *str)
+{
+   debug_boot_weak_hash = 1;
+   pr_info("debug_boot_weak_hash enabled\n");
+   return 0;
+}
+early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
+
 static DEFINE_STATIC_KEY_TRUE(not_filled_random_ptr_key);
 static siphash_key_t ptr_key __read_mostly;
 
@@ -1703,6 +1714,12 @@ static char *ptr_to_id(char *buf, char *end, void *ptr, 
struct printf_spec spec)
const char *str = sizeof(ptr) == 8 ? "(ptrval)" : "(ptrval)";
unsigned long hashval;
 
+   /* When debugging early boot use non-cryptographically secure hash. */
+   if (unlikely(debug_boot_weak_hash)) {
+   hashval = hash_long((unsigned long)ptr, 32);
+   return pointer_string(buf, end, (const void *)hashval, spec);
+   }
+
if (static_branch_unlikely(¬_filled_random_ptr_key)) {
spec.field_width = 2 * sizeof(ptr);
/* string length must be less than default_width */
-- 
2.17.1

[PATCH v9 2/4] random: Return nbytes filled from hw RNG

[Tobin C. Harding]

Currently the function get_random_bytes_arch() has return value 'void'.
If the hw RNG fails we currently fall back to using get_random_bytes().
This defeats the purpose of requesting random material from the hw RNG
in the first place.

There are currently no intree users of get_random_bytes_arch().

Only get random bytes from the hw RNG, make function return the number
of bytes retrieved from the hw RNG.

Acked-by: Theodore Ts'o 
Reviewed-by: Steven Rostedt (VMware) 
Signed-off-by: Tobin C. Harding 
---
 drivers/char/random.c  | 16 +---
 include/linux/random.h |  2 +-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index ed679099afba..e98fa03cdb91 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1724,26 +1724,28 @@ EXPORT_SYMBOL(del_random_ready_callback);
  * key known by the NSA).  So it's useful if we need the speed, but
  * only if we're willing to trust the hardware manufacturer not to
  * have put in a back door.
+ *
+ * Return number of bytes filled in.
  */
-void get_random_bytes_arch(void *buf, int nbytes)
+int __must_check get_random_bytes_arch(void *buf, int nbytes)
 {
+   int left = nbytes;
char *p = buf;
 
-   trace_get_random_bytes_arch(nbytes, _RET_IP_);
-   while (nbytes) {
+   trace_get_random_bytes_arch(left, _RET_IP_);
+   while (left) {
unsigned long v;
-   int chunk = min(nbytes, (int)sizeof(unsigned long));
+   int chunk = min_t(int, left, sizeof(unsigned long));
 
if (!arch_get_random_long(&v))
break;
 
memcpy(p, &v, chunk);
p += chunk;
-   nbytes -= chunk;
+   left -= chunk;
}
 
-   if (nbytes)
-   get_random_bytes(p, nbytes);
+   return nbytes - left;
 }
 EXPORT_SYMBOL(get_random_bytes_arch);
 
diff --git a/include/linux/random.h b/include/linux/random.h
index 2ddf13b4281e..f1c9bc5cd231 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -38,7 +38,7 @@ extern void get_random_bytes(void *buf, int nbytes);
 extern int wait_for_random_bytes(void);
 extern int add_random_ready_callback(struct random_ready_callback *rdy);
 extern void del_random_ready_callback(struct random_ready_callback *rdy);
-extern void get_random_bytes_arch(void *buf, int nbytes);
+extern int __must_check get_random_bytes_arch(void *buf, int nbytes);
 
 #ifndef MODULE
 extern const struct file_operations random_fops, urandom_fops;
-- 
2.17.1

Re: [PATCH net-next 0/2] docs: net: Convert netdev-FAQ to RST

[Tobin C. Harding]

On Wed, Jul 25, 2018 at 12:50:03PM +1000, Tobin C. Harding wrote:

Please drop this.  I've forgotten to deal with the links from
Documentation/*.rst to Documentation/networking/netdev-FAQ.txt


Since I've already botched it can I ask for guidance here.  The problem
is updating the links means making changes that will cause merge
conflicts if they go through net-dev tree (since most references are in
Documentation/*.rst).  But we can't go through docs tree either since
that could lead to merge conflicts later as well.

My idea was to leave netdev-FAQ.txt in place but with all content
removed except

  'This file has moved to netdev-FAQ.rst. This file will be removed once
  netdev RST conversion is complete'

And then do the same for each file conversion under
Documentation/networking/.  Once all the files are converted a single
patch set updating all references into Documentation/networking/*.rst
could be posted to the docs tree.

One other idea was to leave a symlink netdev-FAQ.txt -> netdev-FAQ.rst
but I don't know how that would play with the build system (docs or
otherwise).


thanks,
Tobin.

Re: [RFC] doc: fix code snippet build warnings

[Tobin C. Harding]

On Wed, Jan 10, 2018 at 08:37:02AM -0800, Paul E. McKenney wrote:
> On Wed, Jan 10, 2018 at 03:04:53PM +1100, Tobin C. Harding wrote:
> > Posting as RFC in the hope that someone knows how to massage sphinx
> > correctly to fix this patch.
> 
> I would welcome that.  ;-)
> 
> > Currently function kernel-doc contains a multi-line code snippet. This
> > is causing sphinx to emit 5 build warnings
> > 
> > WARNING: Unexpected indentation.
> > WARNING: Unexpected indentation.
> > WARNING: Block quote ends without a blank line; unexpected unindent.
> > WARNING: Block quote ends without a blank line; unexpected unindent.
> > WARNING: Inline literal start-string without end-string.
> > 
> > And the snippet is not rendering correctly in HTML.
> > 
> > We can stop shpinx complaining by using '::' instead of the currently
> > used '``' however this still does not render correctly in HTML. The
> > rendering is [arguably] better but still incorrect. Sphinx renders two
> > function calls thus:
> > 
> > :c:func:`rcu_read_lock()`;
> > 
> > The rest of the snippet does however have correct spacing.
> > 
> > Use '::' to pre-fix code snippet. Clears build warnings but does not
> > render correctly.
> 
> If the usual docbook suspects ack this, I would be happy to carry it.
> 
> Cue debate over silent vs. noisy errors.  ;-)

Besides making me laugh out loud I did not think of this issue while
patching. FWIW, now you have mentioned it, I favour noisy errors :)

thanks,
Tobin.

Re: [RFC] doc: fix code snippet build warnings

[Tobin C. Harding]

On Wed, Jan 10, 2018 at 02:59:58PM -0700, Jonathan Corbet wrote:
> On Wed, 10 Jan 2018 15:04:53 +1100
> "Tobin C. Harding"  wrote:
> 
> > Posting as RFC in the hope that someone knows how to massage sphinx
> > correctly to fix this patch.
> > 
> > Currently function kernel-doc contains a multi-line code snippet. This
> > is causing sphinx to emit 5 build warnings
> > 
> > WARNING: Unexpected indentation.
> > WARNING: Unexpected indentation.
> > WARNING: Block quote ends without a blank line; unexpected unindent.
> > WARNING: Block quote ends without a blank line; unexpected unindent.
> > WARNING: Inline literal start-string without end-string.
> > 
> > And the snippet is not rendering correctly in HTML.
> > 
> > We can stop shpinx complaining by using '::' instead of the currently
> > used '``' however this still does not render correctly in HTML. The
> > rendering is [arguably] better but still incorrect. Sphinx renders two
> > function calls thus:
> > 
> > :c:func:`rcu_read_lock()`;
> > 
> > The rest of the snippet does however have correct spacing.
> 
> The behavior when `` was used is not surprising, that really just does a
> font change.  Once you went with a literal block (with "::") though, the
> situation changes a bit.  That really should work.
> 
> I looked a bit.  This isn't a sphinx (or "shpinx" :) problem, the bug is
> in kernel-doc.  Once we go into the literal mode, it shouldn't be
> screwing around with the text anymore.  Of course, kernel-doc doesn't
> understand enough RST to know that.  I'm a little nervous about trying to
> teach it more, but maybe we have to do that; we should certainly be able
> to put code snippets into the docs and have them come through unmolested.
> 
> I'll try to look more closely at that shortly.  Meanwhile, this patch
> makes things better than the were before.  That said...
> 
> > Use '::' to pre-fix code snippet. Clears build warnings but does not
> > render correctly.
> > 
> > Signed-off-by: Tobin C. Harding 
> > ---
> > 
> > To view current broken rendering see
> > 
> > https://www.kernel.org/doc/html/latest/core-api/kernel-api.html?highlight=rcu_pointer_handoff#c.rcu_pointer_handoff
> > 
> >  include/linux/rcupdate.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index a6ddc42f87a5..cc10e772e3e9 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -568,7 +568,8 @@ static inline void rcu_preempt_sleep_check(void) { }
> >   * is handed off from RCU to some other synchronization mechanism, for
> >   * example, reference counting or locking.  In C11, it would map to
> >   * kill_dependency().  It could be used as follows:
> > - * ``
> > + * ::
> 
> ...rather than adding a separate "::" line, you can just
> s/follows:/follows::/ and the Right Thing will happen (to the same extent
> that it does now, anyway.

Except that it will render as

kill_dependency().  It could be used as follows

instead of
kill_dependency().  It could be used as follows:

(note: final colon)

Also the diff will be bigger.  These two reasons led me to the patch as
it is.  I'm happy to re-spin with your suggested change though.

thanks,
Tobin.

Re: [RFC] doc: fix code snippet build warnings

[Tobin C. Harding]

On Thu, Jan 11, 2018 at 09:25:31AM +1100, Tobin C. Harding wrote:
> On Wed, Jan 10, 2018 at 02:59:58PM -0700, Jonathan Corbet wrote:
> > On Wed, 10 Jan 2018 15:04:53 +1100
> > "Tobin C. Harding"  wrote:
> > 
> > > Posting as RFC in the hope that someone knows how to massage sphinx
> > > correctly to fix this patch.
> > > 
> > > Currently function kernel-doc contains a multi-line code snippet. This
> > > is causing sphinx to emit 5 build warnings
> > > 
> > >   WARNING: Unexpected indentation.
> > >   WARNING: Unexpected indentation.
> > >   WARNING: Block quote ends without a blank line; unexpected unindent.
> > >   WARNING: Block quote ends without a blank line; unexpected unindent.
> > >   WARNING: Inline literal start-string without end-string.
> > > 
> > > And the snippet is not rendering correctly in HTML.
> > > 
> > > We can stop shpinx complaining by using '::' instead of the currently
> > > used '``' however this still does not render correctly in HTML. The
> > > rendering is [arguably] better but still incorrect. Sphinx renders two
> > > function calls thus:
> > > 
> > >   :c:func:`rcu_read_lock()`;
> > > 
> > > The rest of the snippet does however have correct spacing.
> > 
> > The behavior when `` was used is not surprising, that really just does a
> > font change.  Once you went with a literal block (with "::") though, the
> > situation changes a bit.  That really should work.
> > 
> > I looked a bit.  This isn't a sphinx (or "shpinx" :) problem, the bug is
> > in kernel-doc.  Once we go into the literal mode, it shouldn't be
> > screwing around with the text anymore.  Of course, kernel-doc doesn't
> > understand enough RST to know that.  I'm a little nervous about trying to
> > teach it more, but maybe we have to do that; we should certainly be able
> > to put code snippets into the docs and have them come through unmolested.
> > 
> > I'll try to look more closely at that shortly.  Meanwhile, this patch
> > makes things better than the were before.  That said...
> > 
> > > Use '::' to pre-fix code snippet. Clears build warnings but does not
> > > render correctly.
> > > 
> > > Signed-off-by: Tobin C. Harding 
> > > ---
> > > 
> > > To view current broken rendering see
> > > 
> > > https://www.kernel.org/doc/html/latest/core-api/kernel-api.html?highlight=rcu_pointer_handoff#c.rcu_pointer_handoff
> > > 
> > >  include/linux/rcupdate.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index a6ddc42f87a5..cc10e772e3e9 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -568,7 +568,8 @@ static inline void rcu_preempt_sleep_check(void) { }
> > >   * is handed off from RCU to some other synchronization mechanism, for
> > >   * example, reference counting or locking.  In C11, it would map to
> > >   * kill_dependency().  It could be used as follows:
> > > - * ``
> > > + * ::
> > 
> > ...rather than adding a separate "::" line, you can just
> > s/follows:/follows::/ and the Right Thing will happen (to the same extent
> > that it does now, anyway.
> 
> Except that it will render as
> 
>   kill_dependency().  It could be used as follows
> 
> instead of
>   kill_dependency().  It could be used as follows:
> 
> (note: final colon)
> 
> Also the diff will be bigger.  These two reasons led me to the patch as
> it is.  I'm happy to re-spin with your suggested change though.

Woops, not 're-spin' - we are on an RFC.  I'll wait for your response
then submit a PATCH

thanks,
Tobin.

Re: [PATCH v2] drivers/fbtft: Fix indentation

[Tobin C. Harding]

On Wed, Jan 10, 2018 at 06:30:35PM +0100, Jonny Schaefer wrote:
> From: Luis Gerhorst 
> 
> This fixes the checkpatch message:
> 
> CHECK: Alignment should match open parenthesis
> #1380: FILE: drivers/staging/fbtft/fbtft-core.c:1380:
> + dev_warn(dev,
> + "no default functions for regwidth=%d and 
> buswidth=%d\n",
> 

Perhaps:

Checkpatch emits CHECK: Alignment should match open parenthesis

Align code to open parenthesis.

Reasoning:

Patch description should describe the problem then describe what the
patch does (in imperative mood).  Better advice is given in
Documentation/process/submitting-patches.rst section 2

Hope this helps,
Tobin.

[GIT PULL] leaking_addresses.pl changes for 4.16-rc1

[Tobin C. Harding]

The following changes since commit d8a5b80568a9cb66810e75b182018e9edb68e8ff:

  Linux 4.15 (2018-01-28 13:20:33 -0800)

are available in the git repository at:

  git://github.com/tcharding/linux.git tags/leaks-4.16-rc1

for you to fetch changes up to 46753437945535271a557dbf9dcb3ea53f1755e5:

  leaking_addresses: add 32-bit support (2018-01-31 08:18:51 +1100)


leaking_addresses patches for 4.16-rc1

Here is the patch set for changes to scripts/leaking_addresses.pl for
the 4.16-rc1 merge window.  The first 4 patches are clean up.  Then we
add the following functionality:

  - check addresses against the vsyscall memory range instead of just
the first and last address (x86_64)
  - add support for getting config options from the kernel config file
  - add support for 5 page table levels (x86_64)
  - add support for scanning 32 bit kernels (based on the page offset)

Along the way we add some helper sub routines and use `uname -m` instead
of Perl for doing architecture detection.  All these patches, except the
trivial clean up ones, were posted to LKML.  The script with this set
applied has been tested on x86_64 (kernel 4.4 and 4.15), ppc64 (kernel
4.4) and ARM 32-bit (kernel 4.9).

Signed-off-by: Tobin C. Harding 


Tobin C. Harding (10):
  leaking_addresses: fix typo function not called
  leaking_addresses: remove mention of kptr_restrict
  leaking_addresses: remove command examples
  leaking_addresses: indent dependant options
  leaking_addresses: add range check for vsyscall memory
  leaking_addresses: add support for kernel config file
  leaking_addresses: add support for 5 page table levels
  leaking_addresses: use system command to get arch
  leaking_addresses: add is_arch() wrapper subroutine
  leaking_addresses: add 32-bit support

 scripts/leaking_addresses.pl | 261 +--
 1 file changed, 205 insertions(+), 56 deletions(-)

checkpatch changes for 4.16

[Tobin C. Harding]

Hi Joe,

Can I please bother you with a maintainer question.  I know everyone is
super busy right now, I'm asking for a smidgen of your time instead of
doing it wrong and taking up some of Linus' time since it's merge window
and all that.

I have the checkpatch set queued ready to do a GIT PULL to Linus.  My
concern is that the subject line may not be unique if checkpatch changes
are coming in from various trees this merge window. I have tagged the
pull request with

checkpatch-4.16-rc1

with the subject line

[GIT PULL] checkpatch changes for 4.16-rc1

If there are no other changes to checkpatch going in this merge window
then this issue dissolves.  If there are, should I add something to the
subject line to ensure it is unique?

Thanks and sorry for targeting you with this question, I'm sure you have
better things to do also.

Tobin

Re: checkpatch changes for 4.16

[Tobin C. Harding]

On Wed, Jan 31, 2018 at 02:48:56PM -0800, Joe Perches wrote:
> On Thu, 2018-02-01 at 08:46 +1100, Tobin C. Harding wrote:
> > Hi Joe,
> > 
> > Can I please bother you with a maintainer question.  I know everyone is
> > super busy right now, I'm asking for a smidgen of your time instead of
> > doing it wrong and taking up some of Linus' time since it's merge window
> > and all that.
> > 
> > I have the checkpatch set queued ready to do a GIT PULL to Linus.  My
> > concern is that the subject line may not be unique if checkpatch changes
> > are coming in from various trees this merge window. I have tagged the
> > pull request with
> 
> Personally, I think you could let Andrew Morton pick them
> up as very few patches for checkpatch go in any other way.

ok no worries.  I'll leave it until a few rc's are out then send to Andrew.

> What patches are you submitting again?
> 
> Is it just
> 
> commit 7b1924a1d930 ("vsprintf: add printk specifier %px")
> 
> or is there something else?

d94e97bc9ede checkpatch: add sub routine get_stat_real()
d0e27c47f5b9 checkpatch: add sub routine get_stat_here()
986b3b5b7d7a checkpatch: warn for use of %px
26c1cf882d79 checkpatch: add check for tag Co-Developed-by

I thought it was odd when you said they could go in through _any_
tree. Not to worry, it's not urgent. 

Thanks for the responce.

Tobin

Re: checkpatch changes for 4.16

[Tobin C. Harding]

On Wed, Jan 31, 2018 at 03:56:19PM -0800, Joe Perches wrote:
> On Thu, 2018-02-01 at 10:23 +1100, Tobin C. Harding wrote:
> > On Wed, Jan 31, 2018 at 02:48:56PM -0800, Joe Perches wrote:
> > > On Thu, 2018-02-01 at 08:46 +1100, Tobin C. Harding wrote:
> > > > Hi Joe,
> > > > 
> > > > Can I please bother you with a maintainer question.  I know everyone is
> > > > super busy right now, I'm asking for a smidgen of your time instead of
> > > > doing it wrong and taking up some of Linus' time since it's merge window
> > > > and all that.
> > > > 
> > > > I have the checkpatch set queued ready to do a GIT PULL to Linus.  My
> > > > concern is that the subject line may not be unique if checkpatch changes
> > > > are coming in from various trees this merge window. I have tagged the
> > > > pull request with
> > > 
> > > Personally, I think you could let Andrew Morton pick them
> > > up as very few patches for checkpatch go in any other way.
> > 
> > ok no worries.  I'll leave it until a few rc's are out then send to Andrew.
> > 
> > > What patches are you submitting again?
> > > 
> > > Is it just
> > > 
> > > commit 7b1924a1d930 ("vsprintf: add printk specifier %px")
> > > 
> > > or is there something else?
> > 
> > d94e97bc9ede checkpatch: add sub routine get_stat_real()
> > d0e27c47f5b9 checkpatch: add sub routine get_stat_here()
> > 986b3b5b7d7a checkpatch: warn for use of %px
> > 26c1cf882d79 checkpatch: add check for tag Co-Developed-by
> > 
> > I thought it was odd when you said they could go in through _any_
> > tree. Not to worry, it's not urgent. 
> 
> Stuff that is going to be pulled should at least
> marinate in -next for awhile first and not first
> be presented as a pull request.

Oh cheers, I forgot about that for leaking_addresses patches too :(

thanks,
Tobin.

Re: [GIT PULL] leaking_addresses.pl changes for 4.16-rc1

[Tobin C. Harding]

On Wed, Jan 31, 2018 at 01:42:36PM +1100, Tobin C. Harding wrote:
> The following changes since commit d8a5b80568a9cb66810e75b182018e9edb68e8ff:
> 
>   Linux 4.15 (2018-01-28 13:20:33 -0800)
> 
> are available in the git repository at:
> 
>   git://github.com/tcharding/linux.git tags/leaks-4.16-rc1
> 
> for you to fetch changes up to 46753437945535271a557dbf9dcb3ea53f1755e5:
> 
>   leaking_addresses: add 32-bit support (2018-01-31 08:18:51 +1100)

Hi Linus,

It has just come to my attention that I should have pushed these changes
to Linux next before requesting you to pull them.  Please feel free to
drop this request, I can try again next merge window after going through
linux next.

thanks,
Tobin.

Re: [PATCH v5] leaking_addresses: add generic 32-bit support

[Tobin C. Harding]

Hi Kaiwan,

Thanks for the patch

On Thu, Jan 04, 2018 at 05:51:25PM +0530, kaiwan.billimo...@gmail.com wrote:
> The script now attempts to detect the architecture it's running upon; as of 
> now,
> we explicitly support x86_64, PPC64, ARM64, MIPS64 and x86_32.

This is incorrect. We do not currently support ARM64, MIPS64 and x86_32.

> If it's one of them, we proceed "normally". If we fail to detect the arch,
> we fallback to 64-bit scanning, _unless_ the user has passed either of these
> option switches: "--opt-32bit" and/or "--page-offset-32bit=".
> 
> If so, we switch to scanning for leaked addresses based on the value of
> PAGE_OFFSET (via an auto-detected or fallback mechanism).
> 
> Currently, code (or "rules") to detect special cases for x86_64, PPC64 and 
> Aarch64
> (in the get_address_re sub) is present. Also, we now have also builtin 
> "stubs",
> for lack of a better term, where additional rules for other 64-bit arch's can 
> be
> plugged into the code, in future, as applicable.
> 
> This patch adds support for ix86 and generic 32 bit architectures.
>  - Add command line option for generic 32-bit checking.
>  - Add command line option for page offset.
>  - Add command line option for kernel configuration file.
>  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>  - Use page offset when checking for kernel virtual addresses.

This is all pretty confusing. It is hard to discern what is the
description of the problem being fixed and what is the description of
the new behaviour implemented by the patch. Also this patch does not add
kernel configuration file option. 

> The script has been lightly tested on the following systems:
>  x86_64
>  x86_32 (on a i686 running Debian 7 to be precise)
>  ARM32  (on a Yocto-based qemuarm32 ARM Versatile platform (ARM926EJ-S cpu))
> 
> In the first two cases, one just has to run the script - no parameters 
> required.
> In the ARM-32 case, it will, by design, fail to detect the architecture;
> (re)running it with the '--32-bit' and/or the '--page-offset-32bit='

This does not match the code. In the code you have defined the option as
'--opt-32bit'? '--32-bit' is better ;)

> option switch(es) has the desired effect: it now detects 32-bit leaking kernel
> addresses, if any.
> 
> Request more testing on the above and other platforms.
> 
> 
> Signed-off-by: Kaiwan N Billimoria 
> 
> ---
>  scripts/leaking_addresses.pl | 190 
> +++
>  1 file changed, 156 insertions(+), 34 deletions(-)
> 
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index a29e13e577a7..b0807b3a3c7c 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -1,10 +1,10 @@
>  #!/usr/bin/env perl
>  #
>  # (c) 2017 Tobin C. Harding 
> -

Please make only the minimum number of changes required.

> +# (c) 2017 Kaiwan N Billimoria 
>  # Licensed under the terms of the GNU GPL License version 2
>  #
> -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> +# leaking_addresses.pl: Scan kernel for potential leaking addresses.
>  #  - Scans dmesg output.
>  #  - Walks directory tree and parses each file (for each directory in @DIRS).
>  #
> @@ -32,11 +32,6 @@ my @DIRS = ('/proc', '/sys');
>  # Timer for parsing each file, in seconds.
>  my $TIMEOUT = 10;
>  
> -# Script can only grep for kernel addresses on the following architectures. 
> If
> -# your architecture is not listed here and has a grep'able kernel address 
> please
> -# consider submitting a patch.
> -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> -
>  # Command line options.
>  my $help = 0;
>  my $debug = 0;
> @@ -48,7 +43,9 @@ my $suppress_dmesg = 0; # Don't show dmesg in 
> output.
>  my $squash_by_path = 0;  # Summary report grouped by absolute 
> path.
>  my $squash_by_filename = 0;  # Summary report grouped by filename.
>  
> -my $kernel_config_file = ""; # Kernel configuration file.

No need for this to be removed and re-added (if you line up the comments).

> +my $opt_32_bit = 0;# Detect (only) 32-bit kernel leaking 
> addresses.

As previously discussed, please be consistent in naming: $opt_32bit

> +my $page_offset_32bit = 0; # 32-bit: value of CONFIG_PAGE_OFFSET.
> +my $kernel_config_file = "";   # Kernel configuration file.
>  
>  # Do not parse these files (absolute path).
>  my @skip_parse_files_abs = ('/proc/kmsg',
> @@ -104,10 +101,12 @@ Options:
> --squash-by-path  Sh

[PATCH] leaking_addresses: add files to skip

[Tobin C. Harding]

Script currently times out when parsing the following files:

/proc/kallsyms
/proc/sched_debug
/proc/PID/smaps

None of these files leak kernel addresses. We can skip parsing them.

Add entries to list of files to skip.

Signed-off-by: Tobin C. Harding 
---
 scripts/leaking_addresses.pl | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index ce5d58f3e619..32e2fc9fc8c3 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -58,7 +58,9 @@ my @skip_parse_files_abs = ('/proc/kmsg',
'/sys/firmware/devicetree',
'/proc/device-tree',
'/sys/kernel/debug/tracing/trace_pipe',
-   '/sys/kernel/security/apparmor/revision');
+   '/sys/kernel/security/apparmor/revision',
+   '/proc/kallsyms',
+   '/proc/sched_debug');
 
 # Do not parse these files under any subdirectory.
 my @skip_parse_files_any = ('0',
@@ -71,7 +73,8 @@ my @skip_parse_files_any = ('0',
'snapshot_raw',
'trace_pipe_raw',
'ptmx',
-   'trace_pipe');
+   'trace_pipe',
+   'smaps');
 
 # Do not walk these directories (absolute path).
 my @skip_walk_dirs_abs = ();
-- 
2.7.4

[PATCH 0/2] fix kernel-docs for struct iio_trigger

[Tobin C. Harding]

This set is a re-spin of

[PATCH] iio: add kernel-doc '@owner'

Patch 1 is the original patch. Adds a kernel-doc description for field
@owner clearing a sphinx build warning when building kernel documentation.

Patch 2 adds field identifier for @use_count. Currently this field has a
description but lacks a field identifier.

Tobin C. Harding (2):
  iio: add kernel-doc for field @owner
  iio: add field identifier for @use_count kernel-doc

 include/linux/iio/trigger.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH] docs: add index entry for networking/msg_zerocopy

[Tobin C. Harding]

Currently msg_zerocopy is not included in any toctree. Sphinx emits a
build warning to this effect. The other three rst files in
Documentation/networking are all indexed. We can add msg_zerocopy to the
toctree to enable navigation of the document via HTML kernel docs.

Add msg_zerocopy to the networking/ toctree.

Signed-off-by: Tobin C. Harding 
---
 Documentation/networking/index.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/networking/index.rst 
b/Documentation/networking/index.rst
index 66e620866245..19e8a927d79b 100644
--- a/Documentation/networking/index.rst
+++ b/Documentation/networking/index.rst
@@ -9,6 +9,7 @@ Contents:
batman-adv
kapi
z8530book
+   msg_zerocopy
 
 .. only::  subproject
 
-- 
2.7.4

[PATCH 2/2 RESEND] iio: add field identifier for @use_count kernel-doc

[Tobin C. Harding]

Kernel-doc for @use_count does not currently have a field
identifier. All the rest of the fields do. @use_count is used internally
and should not be accessed directly by the driver so it should be
marked as so.

Add [INTERN] identifier to @use_count field.

Signed-off-by: Tobin C. Harding 
---
 include/linux/iio/trigger.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
index 390789d43369..b19b7204ef84 100644
--- a/include/linux/iio/trigger.h
+++ b/include/linux/iio/trigger.h
@@ -49,7 +49,7 @@ struct iio_trigger_ops {
  * @dev:   [DRIVER] associated device (if relevant)
  * @list:  [INTERN] used in maintenance of global trigger list
  * @alloc_list:[DRIVER] used for driver specific trigger list
- * @use_count: use count for the trigger
+ * @use_count: [INTERN] use count for the trigger.
  * @subirq_chip:   [INTERN] associate 'virtual' irq chip.
  * @subirq_base:   [INTERN] base number for irqs provided by trigger.
  * @subirqs:   [INTERN] information about the 'child' irqs.
-- 
2.7.4

[PATCH 1/2 RESEND] iio: add kernel-doc for field @owner

[Tobin C. Harding]

When building kernel documentation sphinx emits the following warning

warning: No description found for parameter 'owner'

Add description for struct member 'owner'.

Signed-off-by: Tobin C. Harding 
---
 include/linux/iio/trigger.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
index 7d5e44518379..390789d43369 100644
--- a/include/linux/iio/trigger.h
+++ b/include/linux/iio/trigger.h
@@ -43,6 +43,7 @@ struct iio_trigger_ops {
 /**
  * struct iio_trigger - industrial I/O trigger device
  * @ops:   [DRIVER] operations structure
+ * @owner: [INTERN] owner of this driver module
  * @id:[INTERN] unique id number
  * @name:  [DRIVER] unique name
  * @dev:   [DRIVER] associated device (if relevant)
-- 
2.7.4

[PATCH 0/2 RESEND] fix kernel-docs for struct iio_trigger

[Tobin C. Harding]

This set is a re-spin of

[PATCH] iio: add kernel-doc '@owner'

Patch 1 is the original patch. Adds a kernel-doc description for field
@owner clearing a sphinx build warning when building kernel documentation.

Patch 2 adds field identifier for @use_count. Currently this field has a
description but lacks a field identifier.

Tobin C. Harding (2):
  iio: add kernel-doc for field @owner
  iio: add field identifier for @use_count kernel-doc

 include/linux/iio/trigger.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.7.4

Re: [PATCH] leaking_addresses: add files to skip

[Tobin C. Harding]

On Fri, Jan 05, 2018 at 04:11:07PM -0800, Kees Cook wrote:
> On Fri, Jan 5, 2018 at 2:59 PM, Tobin C. Harding  wrote:
> > Script currently times out when parsing the following files:
> >
> > /proc/kallsyms
> > /proc/sched_debug
> > /proc/PID/smaps
> 
> Seems like kallsyms would be one to absolutely scan... it shouldn't
> cause hangs either.

Haven't we fixed kallsyms now? Do you mean that we should be checking to
see if the scanned kernel has been patched to include the kallsysms
fixes in 4.14? If so perhaps we should add functionality to just check
the first line for an address and warn if one is found. No real reason
to include ever address in kallsyms in the output.

Script doesn't hang but it times out with the default timer (10 seconds). 

thanks,
Tobin.