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

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

[Jason A. Donenfeld]

On Wed, Oct 25, 2017 at 5:49 AM, Tobin C. Harding <me@tobin.cc> 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.

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.

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 <me@tobin.cc> 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 06:00:21AM +0200, Jason A. Donenfeld wrote:
> On Wed, Oct 25, 2017 at 5:49 AM, Tobin C. Harding <me@tobin.cc> 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

[Jason A. Donenfeld]

On Thu, Oct 26, 2017 at 12:27 AM, Tobin C. Harding <me@tobin.cc> 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.)

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 <me@tobin.cc> 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.

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

[Greg KH]

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 <me@tobin.cc> 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.

thanks,

greg k-h

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 <me@tobin.cc> 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 v7] printk: hash addresses printed with %p

[Jason A. Donenfeld]

On Mon, Oct 30, 2017 at 9:22 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 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

I had already suggested that much earlier in the thread, but
discounted it in the very same suggestion, because that branch turns
out to be not that expensive anyway, and I think it's more important
that we're able to print meaningful values as soon as we can, rather
than waiting for the scheduler.

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" <me@tobin.cc> 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 v7] printk: hash addresses printed with %p

[Steven Rostedt]

On Wed, 25 Oct 2017 14:49:34 +1100
"Tobin C. Harding" <me@tobin.cc> 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

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 <me@tobin.cc> 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

[Rasmus Villemoes]

On 25 October 2017 at 01:57, Tobin C. Harding <me@tobin.cc> 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.

> 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.

Rasmus

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 <me@tobin.cc> 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 <insert explanation about being a newbie here>.

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 <me@tobin.cc> 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

[Jason A. Donenfeld]

On Tue, Oct 24, 2017 at 2:31 AM, Tobin C. Harding <me@tobin.cc> 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.

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

[Tobin C. Harding]

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.

> for the crypto/rng/siphash aspects:
> 
> Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>

thanks,
Tobin.

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

[Jason A. Donenfeld]

Provided you've tested this and the static_key guard stuff actually
works as intended, for the crypto/rng/siphash aspects:

Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>

[PATCH v7] 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 <me@tobin.cc>
---

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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..3faecf219412 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,7 @@
 #include <linux/uuid.h>
 #include <linux/of.h>
 #include <net/addrconf.h>
+#include <linux/siphash.h>
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
@@ -1591,6 +1592,55 @@ 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 DEFINE_STATIC_KEY_TRUE(no_ptr_secret);
+
+static void fill_random_ptr_key(struct random_ready_callback *rdy)
+{
+	get_random_bytes(&ptr_secret, sizeof(ptr_secret));
+	static_branch_disable(&no_ptr_secret);
+}
+
+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;
+
+	if (static_branch_unlikely(&no_ptr_secret))
+		return "(pointer value)";
+
+#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 * sizeof(unsigned int);
+	spec.flags = SMALL;
+	spec.base = 16;
+
+	return number(buf, end, hashval, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,6 +1753,9 @@ int kptr_restrict __read_mostly;
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
+ *
+ * Note: The 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,
@@ -1857,7 +1910,11 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		case 'F':
 			return device_node_string(buf, end, ptr, spec, fmt + 1);
 		}
+	default:  /* default is to _not_ leak addresses, hash before printing */
+		return ptr_to_id(buf, end, ptr, spec);
 	}
+
+	/* OK, let's print the address */
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
 		spec.field_width = default_width;
-- 
2.7.4