Re: [PATCH V11 0/5] hash addresses printed with %p

[Sergey Senozhatsky]

On (11/30/17 19:26), Sergey Senozhatsky wrote:
> On (11/30/17 10:23), David Laight wrote:
> [..]
> > > Maybe I'm being thick, but...  if we're rendering these addresses
> > > unusable by hashing them, why not just print something like
> > > "" in their place?  That loses the uniqueness thing but I
> > > wonder how valuable that is in practice?
> > 
> > My worry is that is you get a kernel 'oops' print with actual register
> > values you have no easy way of tying an address or address+offset to
> > the corresponding hash(address) printed elsewhere.
> 
> print the existing hash:pointer mappings in panic()? [if we can do that]

by this I meant
"when oops_in_progress == 1 then print hash:pointer for %p,
 not just hash".

-ss

Re: [PATCH V11 0/5] hash addresses printed with %p

[Sergey Senozhatsky]

On (11/30/17 10:23), David Laight wrote:
[..]
> > Maybe I'm being thick, but...  if we're rendering these addresses
> > unusable by hashing them, why not just print something like
> > "" in their place?  That loses the uniqueness thing but I
> > wonder how valuable that is in practice?
> 
> My worry is that is you get a kernel 'oops' print with actual register
> values you have no easy way of tying an address or address+offset to
> the corresponding hash(address) printed elsewhere.

print the existing hash:pointer mappings in panic()? [if we can do that]

-ss

RE: [PATCH V11 0/5] hash addresses printed with %p

[David Laight]

From: Andrew Morton
> Sent: 29 November 2017 23:21
> >
> > The added advantage of hashing %p is that security is now opt-out, if
> > you _really_ want the address you have to work a little harder and use
> > %px.

You need a system-wide opt-out that prints the actual values.
Otherwise developers will use something else to print addresses and
the code will remain in the released drivers.

> > The idea for creating the printk specifier %px to print the actual
> > address was suggested by Kees Cook (see below for email threads by
> > subject).
> 
> Maybe I'm being thick, but...  if we're rendering these addresses
> unusable by hashing them, why not just print something like
> "" in their place?  That loses the uniqueness thing but I
> wonder how valuable that is in practice?

My worry is that is you get a kernel 'oops' print with actual register
values you have no easy way of tying an address or address+offset to
the corresponding hash(address) printed elsewhere.

David

Re: [PATCH V11 0/5] hash addresses printed with %p

[Tobin C. Harding]

On Wed, Nov 29, 2017 at 03:20:40PM -0800, Andrew Morton wrote:
> On Wed, 29 Nov 2017 13:05:00 +1100 "Tobin C. Harding"  wrote:
> 
> > Currently there exist approximately 14 000 places in the Kernel where
> > addresses are being printed using an unadorned %p. This potentially
> > leaks sensitive information regarding the Kernel layout in memory. Many
> > of these calls are stale, instead of fixing every call lets hash the
> > address by default before printing. This will of course break some
> > users, forcing code printing needed addresses to be updated. We can add
> > a printk specifier for this purpose (%px) to give developers a clear
> > upgrade path for breakages caused by applying this patch set.
> > 
> > The added advantage of hashing %p is that security is now opt-out, if
> > you _really_ want the address you have to work a little harder and use
> > %px.
> > 
> > The idea for creating the printk specifier %px to print the actual
> > address was suggested by Kees Cook (see below for email threads by
> > subject).
> 
> Maybe I'm being thick, but...  if we're rendering these addresses
> unusable by hashing them, why not just print something like
> "" in their place?  That loses the uniqueness thing but I
> wonder how valuable that is in practice?

The discussion on this has been fragmented over _at least_ 5 patch sets
with totally different subjects. And I only just added you to the CC
list, my apologies if this is a bit confusing.

Consensus was that if we provide a unique identifier (the hashed
address) then this is useful for debugging (i.e differentiating between
structs when you have a list of them).

The first 32 bits (on 64 bit machines) were zeroed out because

1. they are unnecessary in achieving the aim.
2. it reduces noise.
3. makes explicit some funny business was going on.

And bonus points, hopefully we don't break userland tools that parse
addresses if the format is still the same.

thanks,
Tobin.

Re: [PATCH V11 0/5] hash addresses printed with %p

[Andrew Morton]

On Wed, 29 Nov 2017 13:05:00 +1100 "Tobin C. Harding"  wrote:

> Currently there exist approximately 14 000 places in the Kernel where
> addresses are being printed using an unadorned %p. This potentially
> leaks sensitive information regarding the Kernel layout in memory. Many
> of these calls are stale, instead of fixing every call lets hash the
> address by default before printing. This will of course break some
> users, forcing code printing needed addresses to be updated. We can add
> a printk specifier for this purpose (%px) to give developers a clear
> upgrade path for breakages caused by applying this patch set.
> 
> The added advantage of hashing %p is that security is now opt-out, if
> you _really_ want the address you have to work a little harder and use
> %px.
> 
> The idea for creating the printk specifier %px to print the actual
> address was suggested by Kees Cook (see below for email threads by
> subject).

Maybe I'm being thick, but...  if we're rendering these addresses
unusable by hashing them, why not just print something like
"" in their place?  That loses the uniqueness thing but I
wonder how valuable that is in practice?

[PATCH V11 0/5] hash addresses printed with %p

[Tobin C. Harding]

Currently there exist approximately 14 000 places in the Kernel where
addresses are being printed using an unadorned %p. This potentially
leaks sensitive information regarding the Kernel layout in memory. Many
of these calls are stale, instead of fixing every call lets hash the
address by default before printing. This will of course break some
users, forcing code printing needed addresses to be updated. We can add
a printk specifier for this purpose (%px) to give developers a clear
upgrade path for breakages caused by applying this patch set.

The added advantage of hashing %p is that security is now opt-out, if
you _really_ want the address you have to work a little harder and use
%px.

The idea for creating the printk specifier %px to print the actual
address was suggested by Kees Cook (see below for email threads by
subject).

Newbie question: I don't know who is potentially going to want to apply
this, I've CC'd Andrew Morton. I'm guessing this should go into
linux-next so we can see what breaks? I do not know exactly how code
gets into linux-next. I've CC'd Stephen Rothwell.

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

V11:
 - Add patch fixing %pK documentation.
 - Refactor %pK as a separate patch.
 - Add patch adding printk specifier %px, which prints the actual
   address (i.e replaces original %p behaviour). 
 - Use %px for KASAN patch.

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 0/5] add printk specifier %px, unique identifier
[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 (5):
  docs: correct documentation for %pK
  vsprintf: refactor %pK code out of pointer()
  printk: hash addresses printed with %p
  vsprintf: add printk specifier %px
  kasan: use %px to print addresses instead of %p

 Documentation/printk-formats.txt |  31 ++-
 lib/test_printf.c| 108 ++
 lib/vsprintf.c   | 194 +--
 mm/kasan/report.c|   8 +-
 scripts/checkpatch.pl|   2 +-
 5 files changed, 248 insertions(+), 95 deletions(-)

-- 
2.7.4