[PATCH 0/3] lib/vsprintf: Hash remaining raw addresses

[PATCH 0/3] lib/vsprintf: Hash remaining raw addresses

[Geert Uytterhoeven]

	Hi all,

There are still two format specifiers that print unhanced kernel
addresses, potentially leaking sensitive information regarding the
kernel layout in memory.

This patch series fixes this by printing hashed addresses instead.
    
Thanks!

Geert Uytterhoeven (3):
  lib/vsprintf: Prepare for more general use of ptr_to_id()
  lib/vsprintf: Hash legacy clock addresses
  lib/vsprintf: Hash printed address for netdev bits fallback

 Documentation/core-api/printk-formats.rst |  5 ++---
 lib/vsprintf.c                            | 17 ++++++++++-------
 2 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/3] lib/vsprintf: Prepare for more general use of ptr_to_id()

[Geert Uytterhoeven]

  - Make the ptr argument const, to avoid adding casts in future
    callers,
  - Add a forward declaration, to avoid moving large blocks of code.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 lib/vsprintf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 095a677f89c02442..b5bf90731c5a5277 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -407,6 +407,9 @@ struct printf_spec {
 #define FIELD_WIDTH_MAX ((1 << 23) - 1)
 #define PRECISION_MAX ((1 << 15) - 1)
 
+static char *ptr_to_id(char *buf, char *end, const void *ptr,
+		       struct printf_spec spec);
+
 static noinline_for_stack
 char *number(char *buf, char *end, unsigned long long num,
 	     struct printf_spec spec)
@@ -1714,7 +1717,8 @@ static int __init initialize_ptr_random(void)
 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)
+static char *ptr_to_id(char *buf, char *end, const void *ptr,
+		       struct printf_spec spec)
 {
 	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
 	unsigned long hashval;
-- 
2.17.1

Re: [PATCH 1/3] lib/vsprintf: Prepare for more general use of ptr_to_id()

[Andy Shevchenko]

On Mon, Oct 08, 2018 at 01:05:02PM +0200, Geert Uytterhoeven wrote:
>   - Make the ptr argument const, to avoid adding casts in future
>     callers,


>   - Add a forward declaration, to avoid moving large blocks of code.

How big it would be? ptr_to_id() itself plus...

> +static char *ptr_to_id(char *buf, char *end, const void *ptr,
> +		       struct printf_spec spec);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] lib/vsprintf: Prepare for more general use of ptr_to_id()

[Geert Uytterhoeven]

Hi Andy,

On Mon, Oct 8, 2018 at 4:25 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Oct 08, 2018 at 01:05:02PM +0200, Geert Uytterhoeven wrote:
> >   - Make the ptr argument const, to avoid adding casts in future
> >     callers,
>
> >   - Add a forward declaration, to avoid moving large blocks of code.
>
> How big it would be? ptr_to_id() itself plus...

... all the randomization helpers.
And ptr_to_id() needs pointer_string(), string(), widen_string(), number(),
and move_right().

118 insertions(+), 120 deletions(-)

Is that acceptable?

> > +static char *ptr_to_id(char *buf, char *end, const void *ptr,
> > +                    struct printf_spec spec);
-
Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] lib/vsprintf: Prepare for more general use of ptr_to_id()

[Andy Shevchenko]

On Mon, Oct 08, 2018 at 04:37:29PM +0200, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> On Mon, Oct 8, 2018 at 4:25 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Oct 08, 2018 at 01:05:02PM +0200, Geert Uytterhoeven wrote:
> > >   - Make the ptr argument const, to avoid adding casts in future
> > >     callers,
> >
> > >   - Add a forward declaration, to avoid moving large blocks of code.
> >
> > How big it would be? ptr_to_id() itself plus...
> 
> ... all the randomization helpers.
> And ptr_to_id() needs pointer_string(), string(), widen_string(), number(),
> and move_right().
> 
> 118 insertions(+), 120 deletions(-)
> 
> Is that acceptable?

Better to ask Peter and others, my personal opinion I would go "forward
declaration"-less solution.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] lib/vsprintf: Prepare for more general use of ptr_to_id()

[Petr Mladek]

On Mon 2018-10-08 16:37:29, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> On Mon, Oct 8, 2018 at 4:25 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Oct 08, 2018 at 01:05:02PM +0200, Geert Uytterhoeven wrote:
> > >   - Make the ptr argument const, to avoid adding casts in future
> > >     callers,
> >
> > >   - Add a forward declaration, to avoid moving large blocks of code.
> >
> > How big it would be? ptr_to_id() itself plus...
> 
> ... all the randomization helpers.
> And ptr_to_id() needs pointer_string(), string(), widen_string(), number(),
> and move_right().
> 
> 118 insertions(+), 120 deletions(-)
> 
> Is that acceptable?

Yes, it is acceptable. And my feeling is that it is the preferred
solution is kernel because it helps to keep the code cleaner
in the long term.

I am for moving the code if there is no cyclic dependency.

Best Regards,
Petr

Re: [PATCH 1/3] lib/vsprintf: Prepare for more general use of ptr_to_id()

[Geert Uytterhoeven]

Hi Petr,

On Tue, Oct 9, 2018 at 3:56 PM Petr Mladek <pmladek@suse.com> wrote:
> On Mon 2018-10-08 16:37:29, Geert Uytterhoeven wrote:
> > On Mon, Oct 8, 2018 at 4:25 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Oct 08, 2018 at 01:05:02PM +0200, Geert Uytterhoeven wrote:
> > > >   - Make the ptr argument const, to avoid adding casts in future
> > > >     callers,
> > >
> > > >   - Add a forward declaration, to avoid moving large blocks of code.
> > >
> > > How big it would be? ptr_to_id() itself plus...
> >
> > ... all the randomization helpers.
> > And ptr_to_id() needs pointer_string(), string(), widen_string(), number(),
> > and move_right().
> >
> > 118 insertions(+), 120 deletions(-)
> >
> > Is that acceptable?
>
> Yes, it is acceptable. And my feeling is that it is the preferred
> solution is kernel because it helps to keep the code cleaner
> in the long term.
>
> I am for moving the code if there is no cyclic dependency.

OK, will do so for v2.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 2/3] lib/vsprintf: Hash legacy clock addresses

[Geert Uytterhoeven]

On platforms using the Common Clock Framework, "%pC" prints the clock's
name. On legacy platforms, it prints the unhashed clock's address,
potentially leaking sensitive information regarding the kernel layout in
memory.

Avoid this leak by printing the hashed address instead.  To distinguish
between clocks, a 32-bit unique identifier is as good as an actual
pointer value.

Fixes: ad67b74d2469d9b8 ("printk: hash addresses printed with %p")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/core-api/printk-formats.rst | 5 ++---
 lib/vsprintf.c                            | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 25dc591cb1108790..d39798c2558500d5 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -420,9 +420,8 @@ struct clk
 	%pC	pll1
 	%pCn	pll1
 
-For printing struct clk structures. %pC and %pCn print the name
-(Common Clock Framework) or address (legacy clock framework) of the
-structure.
+For printing struct clk structures. %pC and %pCn print the name of the clock
+(Common Clock Framework) or a unique 32-bit ID (legacy clock framework).
 
 Passed by reference.
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b5bf90731c5a5277..1fb36260a44289e8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1477,7 +1477,7 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 #ifdef CONFIG_COMMON_CLK
 		return string(buf, end, __clk_get_name(clk), spec);
 #else
-		return special_hex_number(buf, end, (unsigned long)clk, sizeof(unsigned long));
+		return ptr_to_id(buf, end, clk, spec);
 #endif
 	}
 }
-- 
2.17.1

Re: [PATCH 2/3] lib/vsprintf: Hash legacy clock addresses

[Petr Mladek]

On Mon 2018-10-08 13:05:03, Geert Uytterhoeven wrote:
> On platforms using the Common Clock Framework, "%pC" prints the clock's
> name. On legacy platforms, it prints the unhashed clock's address,
> potentially leaking sensitive information regarding the kernel layout in
> memory.
> 
> Avoid this leak by printing the hashed address instead.  To distinguish
> between clocks, a 32-bit unique identifier is as good as an actual
> pointer value.
> 
> Fixes: ad67b74d2469d9b8 ("printk: hash addresses printed with %p")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

It makes sense.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH 3/3] lib/vsprintf: Hash printed address for netdev bits fallback

[Geert Uytterhoeven]

The handler for "%pN" falls back to printing the raw pointer value when
using a different format than the (sole supported) special format
"%pNF", potentially leaking sensitive information regarding the kernel
layout in memory.

Avoid this leak by printing the hashed address instead.
Note that there are no in-tree users of the fallback.

Fixes: ad67b74d2469d9b8 ("printk: hash addresses printed with %p")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 lib/vsprintf.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1fb36260a44289e8..a3dc15c89c217b79 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1424,7 +1424,8 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
 }
 
 static noinline_for_stack
-char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
+char *netdev_bits(char *buf, char *end, const void *addr,
+		  struct printf_spec spec,  const char *fmt)
 {
 	unsigned long long num;
 	int size;
@@ -1435,9 +1436,7 @@ char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 		size = sizeof(netdev_features_t);
 		break;
 	default:
-		num = (unsigned long)addr;
-		size = sizeof(unsigned long);
-		break;
+		return ptr_to_id(buf, end, addr, spec);
 	}
 
 	return special_hex_number(buf, end, num, size);
@@ -1952,7 +1951,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			break;
 		return restricted_pointer(buf, end, ptr, spec);
 	case 'N':
-		return netdev_bits(buf, end, ptr, fmt);
+		return netdev_bits(buf, end, ptr, spec, fmt);
 	case 'a':
 		return address_val(buf, end, ptr, fmt);
 	case 'd':
-- 
2.17.1

Re: [PATCH 3/3] lib/vsprintf: Hash printed address for netdev bits fallback

[Petr Mladek]

On Mon 2018-10-08 13:05:04, Geert Uytterhoeven wrote:
> The handler for "%pN" falls back to printing the raw pointer value when
> using a different format than the (sole supported) special format
> "%pNF", potentially leaking sensitive information regarding the kernel
> layout in memory.
> 
> Avoid this leak by printing the hashed address instead.
> Note that there are no in-tree users of the fallback.
> 
> Fixes: ad67b74d2469d9b8 ("printk: hash addresses printed with %p")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Great catch!

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr