[PATCH 0/3][RESEND] add support for never printing hashed addresses

[PATCH 0/3][RESEND] add support for never printing hashed addresses

[Timur Tabi]

[accidentally sent from the wrong email address, so resending]

[The list of email addresses on CC: is getting quite lengthy,
so I hope I've included everyone.]

Although hashing addresses printed via printk does make the
kernel more secure, it interferes with debugging, especially
with some functions like print_hex_dump() which always uses
hashed addresses.

To avoid having to choose between %p and %px, it's easier to
add a kernel command line that treats all %p as %px.  This
encourages developers to use %p more without making debugging
more difficult.

Patches #1 and #2 upgrade the kselftest framework so that
it can report on tests that were skipped outright.  This
is needed for the test_printf module which will now skip
%p hashing tests if hashing is disabled.

Patch #2 upgrades the printf library to check the command
line.  It also updates test_printf().

Timur Tabi (3):
  lib/test_printf: use KSTM_MODULE_GLOBALS macro
  kselftest: add support for skipped tests
  [v2] lib/vsprintf: make-printk-non-secret printks all addresses as
    unhashed

 .../admin-guide/kernel-parameters.txt         | 15 +++++++
 lib/test_printf.c                             | 12 +++++-
 lib/vsprintf.c                                | 40 ++++++++++++++++++-
 tools/testing/selftests/kselftest_module.h    | 18 ++++++---
 4 files changed, 75 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH 1/3] lib/test_printf: use KSTM_MODULE_GLOBALS macro

[Timur Tabi]

Instead of defining the total/failed test counters manually,
test_printf should use the kselftest macro created for this
purpose.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 lib/test_printf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..ad2bcfa8caa1 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -30,8 +30,8 @@
 #define PAD_SIZE 16
 #define FILL_CHAR '$'
 
-static unsigned total_tests __initdata;
-static unsigned failed_tests __initdata;
+KSTM_MODULE_GLOBALS();
+
 static char *test_buffer __initdata;
 static char *alloced_buffer __initdata;
 
-- 
2.25.1

[PATCH 2/3] kselftest: add support for skipped tests

[Timur Tabi]

Update the kselftest framework to all testing clients to
specify that some tests were skipped.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 tools/testing/selftests/kselftest_module.h | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kselftest_module.h b/tools/testing/selftests/kselftest_module.h
index e8eafaf0941a..e2ea41de3f35 100644
--- a/tools/testing/selftests/kselftest_module.h
+++ b/tools/testing/selftests/kselftest_module.h
@@ -11,7 +11,8 @@
 
 #define KSTM_MODULE_GLOBALS()			\
 static unsigned int total_tests __initdata;	\
-static unsigned int failed_tests __initdata
+static unsigned int failed_tests __initdata;	\
+static unsigned int skipped_tests __initdata
 
 #define KSTM_CHECK_ZERO(x) do {						\
 	total_tests++;							\
@@ -21,11 +22,16 @@ static unsigned int failed_tests __initdata
 	}								\
 } while (0)
 
-static inline int kstm_report(unsigned int total_tests, unsigned int failed_tests)
+static inline int kstm_report(unsigned int total_tests, unsigned int failed_tests,
+			      unsigned int skipped_tests)
 {
-	if (failed_tests == 0)
-		pr_info("all %u tests passed\n", total_tests);
-	else
+	if (failed_tests == 0) {
+		if (skipped_tests) {
+			pr_info("skipped %u tests\n", skipped_tests);
+			pr_info("remaining %u tests passed\n", total_tests);
+		} else
+			pr_info("all %u tests passed\n", total_tests);
+	} else
 		pr_warn("failed %u out of %u tests\n", failed_tests, total_tests);
 
 	return failed_tests ? -EINVAL : 0;
@@ -36,7 +42,7 @@ static int __init __module##_init(void)			\
 {							\
 	pr_info("loaded.\n");				\
 	selftest();					\
-	return kstm_report(total_tests, failed_tests);	\
+	return kstm_report(total_tests, failed_tests, skipped_tests);	\
 }							\
 static void __exit __module##_exit(void)		\
 {							\
-- 
2.25.1

[PATCH 3/3] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Timur Tabi]

If the make-printk-non-secret command line parameter is set, then
printk("%p") will print pointers as unhashed.  This is useful for
debugging purposes.

A large warning message is displayed if this option is enabled.
Unhashed pointers, while useful for debugging, expose kernel
addresses which can be a security risk.

Also update test_printf to skip the hashed pointer tests if the
command-line option is set.

Signed-off-by: Timur Tabi <timur@kernel.org>
Acked-by: Petr Mladek <pmladek@suse.com>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 .../admin-guide/kernel-parameters.txt         | 15 ++++++++
 lib/test_printf.c                             |  8 ++++
 lib/vsprintf.c                                | 38 ++++++++++++++++++-
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a10b545c2070..6962379469e4 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2613,6 +2613,21 @@
 			different yeeloong laptops.
 			Example: machtype=lemote-yeeloong-2f-7inch
 
+        make-printk-non-secret
+			Force pointers printed to the console to be unhashed.
+			By default, when a pointer is printed to the kernel
+			console (via %p format string), that pointer is
+			"hashed", i.e. obscured by hashing the pointer value.
+			This is a security feature that hides actual kernel
+			addresses from unprivileged users, but it also makes
+			debugging the kernel more difficult since unequal
+			pointers can no longer be compared.  If this option is
+			specified, then all normal pointers will have their
+			true value printed.  Pointers printed via %pK may
+			still be hashed.  This option should only be specified
+			when debugging the kernel.  Please do not use on
+			production kernels.
+
 	max_addr=nn[KMG]	[KNL,BOOT,ia64] All physical memory greater
 			than or equal to this physical address is ignored.
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ad2bcfa8caa1..b0b62d76e598 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -35,6 +35,8 @@ KSTM_MODULE_GLOBALS();
 static char *test_buffer __initdata;
 static char *alloced_buffer __initdata;
 
+extern bool debug_never_hash_pointers;
+
 static int __printf(4, 0) __init
 do_test(int bufsize, const char *expect, int elen,
 	const char *fmt, va_list ap)
@@ -301,6 +303,12 @@ plain(void)
 {
 	int err;
 
+	if (debug_never_hash_pointers) {
+		pr_warn("skipping plain 'p' tests");
+		skipped_tests += 2;
+		return;
+	}
+
 	err = plain_hash();
 	if (err) {
 		pr_warn("plain 'p' does not appear to be hashed\n");
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..1296d9b0b328 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,34 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+/* Disable pointer hashing if requested */
+bool debug_never_hash_pointers __ro_after_init;
+EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
+
+static int __init debug_never_hash_pointers_enable(char *str)
+{
+	debug_never_hash_pointers = true;
+
+	pr_warn("**********************************************************\n");
+	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
+	pr_warn("**                                                      **\n");
+	pr_warn("** All pointers that are printed to the console will    **\n");
+	pr_warn("** be printed as unhashed.                              **\n");
+	pr_warn("**                                                      **\n");
+	pr_warn("** Kernel memory addresses are exposed, which may       **\n");
+	pr_warn("** reduce the security of your system.                  **\n");
+	pr_warn("**                                                      **\n");
+	pr_warn("** If you see this message and you are not debugging    **\n");
+	pr_warn("** the kernel, report this immediately to your system   **\n");
+	pr_warn("** administrator!                                       **\n");
+	pr_warn("**                                                      **\n");
+	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
+	pr_warn("**********************************************************\n");
+
+	return 0;
+}
+early_param("make-printk-non-secret", debug_never_hash_pointers_enable);
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -2297,8 +2325,14 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		}
 	}
 
-	/* default is to _not_ leak addresses, hash before printing */
-	return ptr_to_id(buf, end, ptr, spec);
+	/*
+	 * default is to _not_ leak addresses, so hash before printing unless
+	 * make-printk-non-secret is specified on the command line.
+	 */
+	if (unlikely(debug_never_hash_pointers))
+		return pointer_string(buf, end, ptr, spec);
+	else
+		return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*
-- 
2.25.1

Re: [PATCH 1/3] lib/test_printf: use KSTM_MODULE_GLOBALS macro

[Timur Tabi]

On 2/9/21 11:18 PM, Timur Tabi wrote:
> Instead of defining the total/failed test counters manually,
> test_printf should use the kselftest macro created for this
> purpose.
> 
> Signed-off-by: Timur Tabi<ttabi@nvidia.com>

Ugh, I really need to stop sending patches late at night.  This is again 
the wrong email address.

I'm sure I'll need to post another version of these patches, so I'll 
just fix it then.

Re: [PATCH 2/3] kselftest: add support for skipped tests

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3476 bytes --]

Hi Timur,

I love your patch! Yet something to improve:

[auto build test ERROR on kselftest/next]
[also build test ERROR on linus/master hnaz-linux-mm/master v5.11-rc7 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Timur-Tabi/add-support-for-never-printing-hashed-addresses/20210210-131927
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
config: h8300-randconfig-s031-20210209 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-215-g0fb77bb6-dirty
        # https://github.com/0day-ci/linux/commit/4387344bd9c51be401880f17c193e4956036c067
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Timur-Tabi/add-support-for-never-printing-hashed-addresses/20210210-131927
        git checkout 4387344bd9c51be401880f17c193e4956036c067
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from lib/test_bitmap.c:17:
   lib/test_bitmap.c: In function 'test_bitmap_init':
>> lib/../tools/testing/selftests/kselftest_module.h:45:48: error: 'skipped_tests' undeclared (first use in this function); did you mean 'failed_tests'?
      45 |  return kstm_report(total_tests, failed_tests, skipped_tests); \
         |                                                ^~~~~~~~~~~~~
   lib/test_bitmap.c:637:1: note: in expansion of macro 'KSTM_MODULE_LOADERS'
     637 | KSTM_MODULE_LOADERS(test_bitmap);
         | ^~~~~~~~~~~~~~~~~~~
   lib/../tools/testing/selftests/kselftest_module.h:45:48: note: each undeclared identifier is reported only once for each function it appears in
      45 |  return kstm_report(total_tests, failed_tests, skipped_tests); \
         |                                                ^~~~~~~~~~~~~
   lib/test_bitmap.c:637:1: note: in expansion of macro 'KSTM_MODULE_LOADERS'
     637 | KSTM_MODULE_LOADERS(test_bitmap);
         | ^~~~~~~~~~~~~~~~~~~
   lib/test_bitmap.c:637:1: error: control reaches end of non-void function [-Werror=return-type]
     637 | KSTM_MODULE_LOADERS(test_bitmap);
         | ^~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +45 lib/../tools/testing/selftests/kselftest_module.h

    39	
    40	#define KSTM_MODULE_LOADERS(__module)			\
    41	static int __init __module##_init(void)			\
    42	{							\
    43		pr_info("loaded.\n");				\
    44		selftest();					\
  > 45		return kstm_report(total_tests, failed_tests, skipped_tests);	\
    46	}							\
    47	static void __exit __module##_exit(void)		\
    48	{							\
    49		pr_info("unloaded.\n");				\
    50	}							\
    51	module_init(__module##_init);				\
    52	module_exit(__module##_exit)
    53	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23128 bytes --]

Re: [PATCH 3/3] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Vlastimil Babka]

On 2/10/21 6:18 AM, Timur Tabi wrote:
> If the make-printk-non-secret command line parameter is set, then
> printk("%p") will print pointers as unhashed.  This is useful for
> debugging purposes.
> 
> A large warning message is displayed if this option is enabled.
> Unhashed pointers, while useful for debugging, expose kernel
> addresses which can be a security risk.
> 
> Also update test_printf to skip the hashed pointer tests if the
> command-line option is set.
> 
> Signed-off-by: Timur Tabi <timur@kernel.org>
> Acked-by: Petr Mladek <pmladek@suse.com>
> Acked-by: Randy Dunlap <rdunlap@infradead.org>
> Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks!

> ---
>  .../admin-guide/kernel-parameters.txt         | 15 ++++++++
>  lib/test_printf.c                             |  8 ++++
>  lib/vsprintf.c                                | 38 ++++++++++++++++++-
>  3 files changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a10b545c2070..6962379469e4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2613,6 +2613,21 @@
>  			different yeeloong laptops.
>  			Example: machtype=lemote-yeeloong-2f-7inch
>  
> +        make-printk-non-secret
> +			Force pointers printed to the console to be unhashed.
> +			By default, when a pointer is printed to the kernel
> +			console (via %p format string), that pointer is
> +			"hashed", i.e. obscured by hashing the pointer value.
> +			This is a security feature that hides actual kernel
> +			addresses from unprivileged users, but it also makes
> +			debugging the kernel more difficult since unequal
> +			pointers can no longer be compared.  If this option is
> +			specified, then all normal pointers will have their
> +			true value printed.  Pointers printed via %pK may
> +			still be hashed.  This option should only be specified
> +			when debugging the kernel.  Please do not use on
> +			production kernels.
> +
>  	max_addr=nn[KMG]	[KNL,BOOT,ia64] All physical memory greater
>  			than or equal to this physical address is ignored.
>  
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index ad2bcfa8caa1..b0b62d76e598 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -35,6 +35,8 @@ KSTM_MODULE_GLOBALS();
>  static char *test_buffer __initdata;
>  static char *alloced_buffer __initdata;
>  
> +extern bool debug_never_hash_pointers;
> +
>  static int __printf(4, 0) __init
>  do_test(int bufsize, const char *expect, int elen,
>  	const char *fmt, va_list ap)
> @@ -301,6 +303,12 @@ plain(void)
>  {
>  	int err;
>  
> +	if (debug_never_hash_pointers) {
> +		pr_warn("skipping plain 'p' tests");
> +		skipped_tests += 2;
> +		return;
> +	}
> +
>  	err = plain_hash();
>  	if (err) {
>  		pr_warn("plain 'p' does not appear to be hashed\n");
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..1296d9b0b328 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,34 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
>  	return widen_string(buf, buf - buf_start, end, spec);
>  }
>  
> +/* Disable pointer hashing if requested */
> +bool debug_never_hash_pointers __ro_after_init;
> +EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
> +
> +static int __init debug_never_hash_pointers_enable(char *str)
> +{
> +	debug_never_hash_pointers = true;
> +
> +	pr_warn("**********************************************************\n");
> +	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("** All pointers that are printed to the console will    **\n");
> +	pr_warn("** be printed as unhashed.                              **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("** Kernel memory addresses are exposed, which may       **\n");
> +	pr_warn("** reduce the security of your system.                  **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("** If you see this message and you are not debugging    **\n");
> +	pr_warn("** the kernel, report this immediately to your system   **\n");
> +	pr_warn("** administrator!                                       **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> +	pr_warn("**********************************************************\n");
> +
> +	return 0;
> +}
> +early_param("make-printk-non-secret", debug_never_hash_pointers_enable);
> +
>  /*
>   * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>   * by an extra set of alphanumeric characters that are extended format
> @@ -2297,8 +2325,14 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		}
>  	}
>  
> -	/* default is to _not_ leak addresses, hash before printing */
> -	return ptr_to_id(buf, end, ptr, spec);
> +	/*
> +	 * default is to _not_ leak addresses, so hash before printing unless
> +	 * make-printk-non-secret is specified on the command line.
> +	 */
> +	if (unlikely(debug_never_hash_pointers))
> +		return pointer_string(buf, end, ptr, spec);
> +	else
> +		return ptr_to_id(buf, end, ptr, spec);
>  }
>  
>  /*
> 

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Marco Elver]

On Tue, Feb 09, 2021 at 11:18PM -0600, Timur Tabi wrote:
> [accidentally sent from the wrong email address, so resending]
> 
> [The list of email addresses on CC: is getting quite lengthy,
> so I hope I've included everyone.]
> 
> Although hashing addresses printed via printk does make the
> kernel more secure, it interferes with debugging, especially
> with some functions like print_hex_dump() which always uses
> hashed addresses.
> 
> To avoid having to choose between %p and %px, it's easier to
> add a kernel command line that treats all %p as %px.  This
> encourages developers to use %p more without making debugging
> more difficult.
> 
> Patches #1 and #2 upgrade the kselftest framework so that
> it can report on tests that were skipped outright.  This
> is needed for the test_printf module which will now skip
> %p hashing tests if hashing is disabled.
> 
> Patch #2 upgrades the printf library to check the command
> line.  It also updates test_printf().
> 
> Timur Tabi (3):
>   lib/test_printf: use KSTM_MODULE_GLOBALS macro
>   kselftest: add support for skipped tests
>   [v2] lib/vsprintf: make-printk-non-secret printks all addresses as
>     unhashed
> 
>  .../admin-guide/kernel-parameters.txt         | 15 +++++++
>  lib/test_printf.c                             | 12 +++++-
>  lib/vsprintf.c                                | 40 ++++++++++++++++++-
>  tools/testing/selftests/kselftest_module.h    | 18 ++++++---
>  4 files changed, 75 insertions(+), 10 deletions(-)

I wanted to test this for deciding if we can show sensitive info in
KFENCE reports, which works just fine now that debug_never_hash_pointers
is non-static. FWIW,

	Acked-by: Marco Elver <elver@google.com>

But unfortunately this series broke some other test:

| In file included from lib/test_bitmap.c:17:
| lib/test_bitmap.c: In function ‘test_bitmap_init’:
| lib/../tools/testing/selftests/kselftest_module.h:45:48: error: ‘skipped_tests’ undeclared (first use in this function); did you mean ‘failed_tests’?
|    45 |  return kstm_report(total_tests, failed_tests, skipped_tests); \
|       |                                                ^~~~~~~~~~~~~
| lib/test_bitmap.c:637:1: note: in expansion of macro ‘KSTM_MODULE_LOADERS’
|   637 | KSTM_MODULE_LOADERS(test_bitmap);
|       | ^~~~~~~~~~~~~~~~~~~
| lib/../tools/testing/selftests/kselftest_module.h:45:48: note: each undeclared identifier is reported only once for each function it appears in
|    45 |  return kstm_report(total_tests, failed_tests, skipped_tests); \
|       |                                                ^~~~~~~~~~~~~
| lib/test_bitmap.c:637:1: note: in expansion of macro ‘KSTM_MODULE_LOADERS’
|   637 | KSTM_MODULE_LOADERS(test_bitmap);
|       | ^~~~~~~~~~~~~~~~~~~
| lib/../tools/testing/selftests/kselftest_module.h:46:1: error: control reaches end of non-void function [-Werror=return-type]
|    46 | }       \
|       | ^
| lib/test_bitmap.c:637:1: note: in expansion of macro ‘KSTM_MODULE_LOADERS’
|   637 | KSTM_MODULE_LOADERS(test_bitmap);
|       | ^~~~~~~~~~~~~~~~~~~

My allyesconfig build suggests test_bitmap.c is the only one, so it
should probably be fixed up in this series.
 
Thanks,
-- Marco

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Andy Shevchenko]

On Wed, Feb 10, 2021 at 10:33 AM Timur Tabi <timur@kernel.org> wrote:
>
> [accidentally sent from the wrong email address, so resending]
>
> [The list of email addresses on CC: is getting quite lengthy,
> so I hope I've included everyone.]
>
> Although hashing addresses printed via printk does make the
> kernel more secure, it interferes with debugging, especially
> with some functions like print_hex_dump() which always uses
> hashed addresses.
>
> To avoid having to choose between %p and %px, it's easier to
> add a kernel command line that treats all %p as %px.  This
> encourages developers to use %p more without making debugging
> more difficult.
>
> Patches #1 and #2 upgrade the kselftest framework so that
> it can report on tests that were skipped outright.  This
> is needed for the test_printf module which will now skip
> %p hashing tests if hashing is disabled.
>
> Patch #2 upgrades the printf library to check the command
> line.  It also updates test_printf().

It's a bit hard in some mailers (like Gmail) to see the different
versions of your patches.
Can you use in the future
 - either `git format-patch -v<N> ...`, where <N> is a version
 - or `git format-patch --subject-prefix="PATCH vX / RESEND / etc" ...`
?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] lib/test_printf: use KSTM_MODULE_GLOBALS macro

[Petr Mladek]

On Tue 2021-02-09 23:18:12, Timur Tabi wrote:
> Instead of defining the total/failed test counters manually,
> test_printf should use the kselftest macro created for this
> purpose.
> 
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>

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

Best Regards,
Petr

Re: [PATCH 3/3] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Petr Mladek]

On Tue 2021-02-09 23:18:14, Timur Tabi wrote:
> If the make-printk-non-secret command line parameter is set, then
> printk("%p") will print pointers as unhashed.  This is useful for
> debugging purposes.
> 
> A large warning message is displayed if this option is enabled.
> Unhashed pointers, while useful for debugging, expose kernel
> addresses which can be a security risk.
> 
> Also update test_printf to skip the hashed pointer tests if the
> command-line option is set.
> 
> Signed-off-by: Timur Tabi <timur@kernel.org>
> Acked-by: Petr Mladek <pmladek@suse.com>
> Acked-by: Randy Dunlap <rdunlap@infradead.org>
> Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  .../admin-guide/kernel-parameters.txt         | 15 ++++++++
>  lib/test_printf.c                             |  8 ++++
>  lib/vsprintf.c                                | 38 ++++++++++++++++++-
>  3 files changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a10b545c2070..6962379469e4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2613,6 +2613,21 @@
>  			different yeeloong laptops.
>  			Example: machtype=lemote-yeeloong-2f-7inch
>  
> +        make-printk-non-secret
> +			Force pointers printed to the console to be unhashed.
> +			By default, when a pointer is printed to the kernel
> +			console (via %p format string), that pointer is
> +			"hashed", i.e. obscured by hashing the pointer value.
> +			This is a security feature that hides actual kernel
> +			addresses from unprivileged users, but it also makes
> +			debugging the kernel more difficult since unequal
> +			pointers can no longer be compared.  If this option is
> +			specified, then all normal pointers will have their
> +			true value printed.  Pointers printed via %pK may
> +			still be hashed.  This option should only be specified
> +			when debugging the kernel.  Please do not use on
> +			production kernels.
> +
>  	max_addr=nn[KMG]	[KNL,BOOT,ia64] All physical memory greater
>  			than or equal to this physical address is ignored.
>  
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index ad2bcfa8caa1..b0b62d76e598 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -35,6 +35,8 @@ KSTM_MODULE_GLOBALS();
>  static char *test_buffer __initdata;
>  static char *alloced_buffer __initdata;
>  
> +extern bool debug_never_hash_pointers;
> +
>  static int __printf(4, 0) __init
>  do_test(int bufsize, const char *expect, int elen,
>  	const char *fmt, va_list ap)
> @@ -301,6 +303,12 @@ plain(void)
>  {
>  	int err;
>  
> +	if (debug_never_hash_pointers) {
> +		pr_warn("skipping plain 'p' tests");
> +		skipped_tests += 2;
> +		return;
> +	}
> +
>  	err = plain_hash();
>  	if (err) {
>  		pr_warn("plain 'p' does not appear to be hashed\n");
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..1296d9b0b328 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,34 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
>  	return widen_string(buf, buf - buf_start, end, spec);
>  }
>  
> +/* Disable pointer hashing if requested */
> +bool debug_never_hash_pointers __ro_after_init;
> +EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
> +
> +static int __init debug_never_hash_pointers_enable(char *str)
> +{
> +	debug_never_hash_pointers = true;
> +
> +	pr_warn("**********************************************************\n");
> +	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("** All pointers that are printed to the console will    **\n");
> +	pr_warn("** be printed as unhashed.                              **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("** Kernel memory addresses are exposed, which may       **\n");
> +	pr_warn("** reduce the security of your system.                  **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("** If you see this message and you are not debugging    **\n");
> +	pr_warn("** the kernel, report this immediately to your system   **\n");
> +	pr_warn("** administrator!                                       **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> +	pr_warn("**********************************************************\n");
> +
> +	return 0;
> +}
> +early_param("make-printk-non-secret", debug_never_hash_pointers_enable);

The mail about EFENCE, and the different names of the parameter and the
variable, made  realized one important thing. The kernel parameter and
the message does not describe the full effect.

The option causes that vsprintf() will not hash pointers. Yes, it is
primary used by printk(). But it is used also in some other
interfaces, especially trace_printk(), seq_buf() API. The naked
pointers might appear more or less anywhere, including procfs,
sysfs, debugfs.

IMHO, we should fix this. The long discussion was about how to make
this option safe. Users should be aware that it is not only about
the kernel log.

I suggest to rename the parameter "debug_never_hash_pointer" and use
the same name for the parameter and the variable.

We also should make the warning more generic. I suggest to replace the
first paragraph with something like:

	pr_warn("** The hashing of printed pointers has been disabled   **\n");
	pr_warn("** for debugging purposes.                             **\n");

Feel free to use a better wording. I am not a native speaker.

Of course, also kernel-parameters.txt has to be updated accordingly.

Best Regards,
Petr

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Tetsuo Handa]

On 2021/02/10 14:18, Timur Tabi wrote:
> [accidentally sent from the wrong email address, so resending]
> 
> [The list of email addresses on CC: is getting quite lengthy,
> so I hope I've included everyone.]
> 
> Although hashing addresses printed via printk does make the
> kernel more secure, it interferes with debugging, especially
> with some functions like print_hex_dump() which always uses
> hashed addresses.

Oh, I was wishing

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..34c7e145ac3c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -802,7 +802,7 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
 	 * Print the real pointer value for NULL and error pointers,
 	 * as they are not actual addresses.
 	 */
-	if (IS_ERR_OR_NULL(ptr))
+	if (IS_ERR_OR_NULL(ptr) || IS_ENABLED(CONFIG_DEBUG_DONT_HASH_POINTERS))
 		return pointer_string(buf, end, ptr, spec);
 
 	/* When debugging early boot use non-cryptographically secure hash. */

change as a kernel config option, for more we try to switch using kernel command line options,
more we likely make errors with sharing appropriate kernel command line options
(e.g. https://github.com/google/syzkaller/commit/99c64d5c672700d6c0de63d11db25a0678e47a75 ).

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Steven Rostedt]

On Thu, 11 Feb 2021 00:46:15 +0900
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Oh, I was wishing
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..34c7e145ac3c 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -802,7 +802,7 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
>  	 * Print the real pointer value for NULL and error pointers,
>  	 * as they are not actual addresses.
>  	 */
> -	if (IS_ERR_OR_NULL(ptr))
> +	if (IS_ERR_OR_NULL(ptr) || IS_ENABLED(CONFIG_DEBUG_DONT_HASH_POINTERS))
>  		return pointer_string(buf, end, ptr, spec);
>  
>  	/* When debugging early boot use non-cryptographically secure hash. */
> 
> change as a kernel config option, for more we try to switch using kernel command line options,
> more we likely make errors with sharing appropriate kernel command line options
> (e.g. https://github.com/google/syzkaller/commit/99c64d5c672700d6c0de63d11db25a0678e47a75 ).

The entire point of this exercise is not to make it easy to add this
feature. Linus was absolutely against a config option, and I am too.

The point of this exercise is to be able to debug the *same* kernel that
someone is having issues with. And this is to facilitate that debugging.
Whereas the example you show, the command line modifies how the kernel
works. This command line only modifies what the kernel displays. Big
difference.

-- Steve

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Tetsuo Handa]

On 2021/02/11 1:18, Steven Rostedt wrote:
> The point of this exercise is to be able to debug the *same* kernel that
> someone is having issues with. And this is to facilitate that debugging.

That's too difficult to use. If a problem is not reproducible, we will have
no choice but always specify "never hash pointers" command line option. If a
problem is reproducible, we can rebuild that kernel with "never hash pointers"
config option turned on.

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Steven Rostedt]

On Thu, 11 Feb 2021 01:39:41 +0900
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> On 2021/02/11 1:18, Steven Rostedt wrote:
> > The point of this exercise is to be able to debug the *same* kernel that
> > someone is having issues with. And this is to facilitate that debugging.  
> 
> That's too difficult to use. If a problem is not reproducible, we will have
> no choice but always specify "never hash pointers" command line option. If a
> problem is reproducible, we can rebuild that kernel with "never hash pointers"
> config option turned on.

Now the question is, why do you need the unhashed pointer?

Currently, the instruction pointer is what is fine right? You get the
a function and its offset. If there's something that is needed, perhaps we
should look at how to fix that, instead of just unhashing all pointers by
default.

-- Steve

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Andy Shevchenko]

On Thu, Feb 11, 2021 at 01:39:41AM +0900, Tetsuo Handa wrote:
> On 2021/02/11 1:18, Steven Rostedt wrote:
> > The point of this exercise is to be able to debug the *same* kernel that
> > someone is having issues with. And this is to facilitate that debugging.
> 
> That's too difficult to use. If a problem is not reproducible, we will have
> no choice but always specify "never hash pointers" command line option. If a
> problem is reproducible, we can rebuild that kernel with "never hash pointers"
> config option turned on.

I think what you are targeting is something like dynamic debug approach where
you can choose which prints to enable/disable and what enable/disable in them.

In that case you specifically apply a command line option and enable only files
/ lines in the files.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Timur Tabi]

On 2/10/21 5:47 AM, Andy Shevchenko wrote:
> It's a bit hard in some mailers (like Gmail) to see the different
> versions of your patches.
> Can you use in the future
>   - either `git format-patch -v<N> ...`, where <N> is a version
>   - or `git format-patch --subject-prefix="PATCH vX / RESEND / etc" ...`
> ?

Yes, of course.  Sorry about that.  Like I said earlier, I really 
shouldn't be posting patches late at night, when I will just forget 
important details.

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Tetsuo Handa]

On 2021/02/11 1:46, Steven Rostedt wrote:
> On Thu, 11 Feb 2021 01:39:41 +0900
> Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> 
>> On 2021/02/11 1:18, Steven Rostedt wrote:
>>> The point of this exercise is to be able to debug the *same* kernel that
>>> someone is having issues with. And this is to facilitate that debugging.  
>>
>> That's too difficult to use. If a problem is not reproducible, we will have
>> no choice but always specify "never hash pointers" command line option. If a
>> problem is reproducible, we can rebuild that kernel with "never hash pointers"
>> config option turned on.
> 
> Now the question is, why do you need the unhashed pointer?

Because unhashed pointers might give some clue. We can rebuild the same kernel
using the same kernel config / compiler etc. and compare unhashed pointers with
addresses in System.map / kallsyms files without reproducing the problem.

> 
> Currently, the instruction pointer is what is fine right? You get the
> a function and its offset. If there's something that is needed, perhaps we
> should look at how to fix that, instead of just unhashing all pointers by
> default.

I'm not refusing to use kernel command line options. I'm expecting that we can
also hardcode using kernel config options. Since boot-time switching via kernel
command line options makes the kernel behave differently, less boot-time
switching is better for avoiding unexpected problems (e.g. unintended LSM was
enabled).

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Timur Tabi]

On 2/10/21 10:46 AM, Steven Rostedt wrote:
> Now the question is, why do you need the unhashed pointer?
> 
> Currently, the instruction pointer is what is fine right? You get the
> a function and its offset. If there's something that is needed, perhaps we
> should look at how to fix that, instead of just unhashing all pointers by
> default.

The original version of this patch only fixed print_hex_dump(), because 
hashed addresses didn't make any sense for that.  Each address is 
incremented by 16 or 32, but since they were all hashed, they may as 
well have been random numbers.

Re: [PATCH 3/3] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Timur Tabi]

On 2/10/21 7:41 AM, Petr Mladek wrote:
> 
> The option causes that vsprintf() will not hash pointers. Yes, it is
> primary used by printk(). But it is used also in some other
> interfaces, especially trace_printk(), seq_buf() API. The naked
> pointers might appear more or less anywhere, including procfs,
> sysfs, debugfs.

Fair point.  Shouldn't calls to seq_buf_printf() (and any printk usage 
that always exists in the context of a user-space process) use %pK anyway?

Hmmm.... maybe vsprintf() should automatically replace %p with %pK if it 
detects a user-space context?

> IMHO, we should fix this. The long discussion was about how to make
> this option safe. Users should be aware that it is not only about
> the kernel log.

Agreed.

> I suggest to rename the parameter "debug_never_hash_pointer" and use
> the same name for the parameter and the variable.

Will do.

> We also should make the warning more generic. I suggest to replace the
> first paragraph with something like:
> 
> 	pr_warn("** The hashing of printed pointers has been disabled   **\n");
> 	pr_warn("** for debugging purposes.                             **\n");
> 
> Feel free to use a better wording. I am not a native speaker.

You could have fooled me.

> Of course, also kernel-parameters.txt has to be updated accordingly.

Ok.

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Steven Rostedt]

On Thu, 11 Feb 2021 02:07:21 +0900
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> I'm not refusing to use kernel command line options. I'm expecting that we can
> also hardcode using kernel config options. Since boot-time switching via kernel
> command line options makes the kernel behave differently, less boot-time
> switching is better for avoiding unexpected problems (e.g. unintended LSM was
> enabled).

You could just add:

 CONFIG_CMDLINE_BOOL=y
 CONFIG_CMDLINE="make-printk-non-secret"

If you want it part of your config.

-- Steve

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Tetsuo Handa]

On 2021/02/11 1:54, Andy Shevchenko wrote:
> On Thu, Feb 11, 2021 at 01:39:41AM +0900, Tetsuo Handa wrote:
>> On 2021/02/11 1:18, Steven Rostedt wrote:
>>> The point of this exercise is to be able to debug the *same* kernel that
>>> someone is having issues with. And this is to facilitate that debugging.
>>
>> That's too difficult to use. If a problem is not reproducible, we will have
>> no choice but always specify "never hash pointers" command line option. If a
>> problem is reproducible, we can rebuild that kernel with "never hash pointers"
>> config option turned on.
> 
> I think what you are targeting is something like dynamic debug approach where
> you can choose which prints to enable/disable and what enable/disable in them.

What I'm targeting is "zero interaction from kernel command line options" like
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/usb/usbip?id=f1bdf414e7dd0cbc26460425719fc3ea479947a2 .

> 
> In that case you specifically apply a command line option and enable only files
> / lines in the files.

While there is boot-config feature for specifying very long kernel command line
options, I can't enforce syzkaller users (including syzbot) to switch what to
enable/disable via kernel command line options. Let alone defining a kernel
command line option for single-purpose debug printk() changes like shown above.

Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses

[Timur Tabi]

On 2/10/21 5:11 AM, Marco Elver wrote:
> I wanted to test this for deciding if we can show sensitive info in
> KFENCE reports, which works just fine now that debug_never_hash_pointers
> is non-static. FWIW,
> 
> 	Acked-by: Marco Elver<elver@google.com>

Thank you.

> But unfortunately this series broke some other test:
> 
> | In file included from lib/test_bitmap.c:17:
> | lib/test_bitmap.c: In function ‘test_bitmap_init’:
> | lib/../tools/testing/selftests/kselftest_module.h:45:48: error: ‘skipped_tests’ undeclared (first use in this function); did you mean ‘failed_tests’?

This will be fixed in v3.  Thanks.

Re: [PATCH 3/3] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Petr Mladek]

Hi,

I have realized that I did not comment the two ideas.

On Wed 2021-02-10 11:27:45, Timur Tabi wrote:
> 
> 
> On 2/10/21 7:41 AM, Petr Mladek wrote:
> > 
> > The option causes that vsprintf() will not hash pointers. Yes, it is
> > primary used by printk(). But it is used also in some other
> > interfaces, especially trace_printk(), seq_buf() API. The naked
> > pointers might appear more or less anywhere, including procfs,
> > sysfs, debugfs.
>
> Fair point.  Shouldn't calls to seq_buf_printf() (and any printk usage that
> always exists in the context of a user-space process) use %pK anyway?

seq_buf is a handy API that might be used for different purpose.
For example, it seems to be used ftrace where people might want
to see real pointers when debugging.

> Hmmm.... maybe vsprintf() should automatically replace %p with %pK if it
> detects a user-space context?

I am not sure if there is an easy and reliable way how to detect the
user-space context. On some architectures, it might be possible to
guess it by the address of the buffer. But it will not work when
the message is temporary printed into a local buffer and copied
later.

Let's keep it simple. Heuristics often become very complex over time.

Best Regards,
Petr

[PATCH 1/3] lib/test_printf: use KSTM_MODULE_GLOBALS macro

[Timur Tabi]

Instead of defining the total/failed test counters manually,
test_printf should use the kselftest macro created for this
purpose.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 lib/test_printf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..ad2bcfa8caa1 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -30,8 +30,8 @@
 #define PAD_SIZE 16
 #define FILL_CHAR '$'
 
-static unsigned total_tests __initdata;
-static unsigned failed_tests __initdata;
+KSTM_MODULE_GLOBALS();
+
 static char *test_buffer __initdata;
 static char *alloced_buffer __initdata;
 
-- 
2.25.1