| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z_0AFjai6Bvg-YLD@pathway.suse.cz>
Date: Mon, 14 Apr 2025 14:31:42 +0200
From: Petr Mladek <pmladek@...e.com>
To: Kees Cook <kees@...nel.org>
Cc: Vlastimil Babka <vbabka@...e.cz>,
Sergio Perez Gonzalez <sperezglz@...il.com>,
Jonathan Corbet <corbet@....net>,
Steven Rostedt <rostedt@...dmis.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
David Rientjes <rientjes@...gle.com>,
Joonsoo Kim <iamjoonsoo.kim@....com>,
Roman Gushchin <roman.gushchin@...ux.dev>,
Harry Yoo <harry.yoo@...cle.com>,
"Paul E. McKenney" <paulmck@...nel.org>,
Randy Dunlap <rdunlap@...radead.org>,
Tamir Duberstein <tamird@...il.com>,
Miguel Ojeda <ojeda@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
linux-doc@...r.kernel.org, linux-mm@...ck.org,
Thomas Huth <thuth@...hat.com>,
"Borislav Petkov (AMD)" <bp@...en8.de>,
Ard Biesheuvel <ardb@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andreas Hindborg <a.hindborg@...nel.org>,
Stephen Boyd <swboyd@...omium.org>, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH] slab: Decouple slab_debug and no_hash_pointers
On Thu 2025-04-10 10:44:31, Kees Cook wrote:
> Some system owners use slab_debug=FPZ (or similar) as a hardening option,
> but do not want to be forced into having kernel addresses exposed due
> to the implicit "no_hash_pointers" boot param setting.[1]
>
> Introduce the "hash_pointers" boot param, which defaults to "auto"
> (the current behavior), but also includes "always" (forcing on hashing
> even when "slab_debug=..." is defined), and "never". The existing
> "no_hash_pointers" boot param becomes an alias for "hash_pointers=never".
>
> This makes it possible to boot with "slab_debug=FPZ hash_pointers=always".
The idea makes sense. But it seems that the patch did not handle
the "always" mode correctly, see below.
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -60,6 +60,20 @@
> bool no_hash_pointers __ro_after_init;
> EXPORT_SYMBOL_GPL(no_hash_pointers);
>
> +/*
> + * Hashed pointers policy selected by "hash_pointers=..." boot param
> + *
> + * `auto` - Hashed pointers enabled unless disabled by slub_debug_enabled=true
> + * `always` - Hashed pointers enabled unconditionally
> + * `never` - Hashed pointers disabled unconditionally
> + */
> +enum hash_pointers_policy {
> + HASH_PTR_AUTO = 0,
> + HASH_PTR_ALWAYS,
> + HASH_PTR_NEVER
> +};
> +static enum hash_pointers_policy hash_pointers_mode __initdata;
> +
> noinline
> static unsigned long long simple_strntoull(const char *startp, char **endp, unsigned int base, size_t max_chars)
> {
> @@ -2271,12 +2285,13 @@ char *resource_or_range(const char *fmt, char *buf, char *end, void *ptr,
> return resource_string(buf, end, ptr, spec, fmt);
> }
>
> -int __init no_hash_pointers_enable(char *str)
> +void __init hash_pointers_finalize(bool slub_debug)
> {
> - if (no_hash_pointers)
> - return 0;
> + if (hash_pointers_mode == HASH_PTR_AUTO && slub_debug)
> + no_hash_pointers = true;
>
> - no_hash_pointers = true;
> + if (!no_hash_pointers)
> + return;
>
> pr_warn("**********************************************************\n");
> pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
The mode/policy is generic but this function is ready to be called
only once. And we might actually want to call it twice, see below.
I would suggest to use a generic names and allow to call it more
times, something like:
/**
* hash_pointers_update() - update the decision whether to hash
* printed pointers
* @auto_disable: Disable hashing in auto mode
*
* The function allows to disable hashing printed pointers either
* when the global mode is HASH_PTR_NEVER or when the caller
* wants to disable it and the mode is HASH_PTR_AUTO.
*/
void __init hash_pointers_update(bool auto_disable)
{
bool disable_hashing = false;
switch(hash_pointers_mode) {
case HASH_PTR_AUTO:
disable_hashing = auto_disable;
break;
case HASH_PTR_ALWAYS:
disable_hashing = true;
break;
case HASH_PTR_NEVER:
if (no_hash_pointers) {
pr_warn("Pointers were temporary printed without hashing. Force hashing again.\n");
no_hash_pointers = false;
}
break;
default:
pr_warn("Unknown hash_pointers mode '%d' specified; assuming auto.\n",
hash_pointers_mode);
disable_hashing = auto_disable;
}
/* Nope when no change requested. */
if (no_hash_pointers || !disable_hashing)
return;
no_hash_pointers = true;
pr_warn("**********************************************************\n");
pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
pr_warn("** **\n");
pr_warn("** This system shows unhashed kernel memory addresses **\n");
pr_warn("** via the console, logs, and other interfaces. This **\n");
pr_warn("** might 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("** Use hash_pointers=always to force this mode off **\n");
pr_warn("** **\n");
pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
pr_warn("**********************************************************\n");
}
> @@ -2289,11 +2304,39 @@ int __init no_hash_pointers_enable(char *str)
> pr_warn("** the kernel, report this immediately to your system **\n");
> pr_warn("** administrator! **\n");
> pr_warn("** **\n");
> + pr_warn("** Use hash_pointers=always to force this mode off **\n");
> + pr_warn("** **\n");
> pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> pr_warn("**********************************************************\n");
> +}
> +
> +static int __init hash_pointers_mode_parse(char *str)
> +{
> + if (!str) {
> + pr_warn("Hash pointers mode empty; falling back to auto.\n");
> + hash_pointers_mode = HASH_PTR_AUTO;
> + } else if (strncmp(str, "auto", 4) == 0) {
> + pr_info("Hash pointers mode set to auto.\n");
> + hash_pointers_mode = HASH_PTR_AUTO;
> + } else if (strncmp(str, "never", 5) == 0) {
> + pr_info("Hash pointers mode set to never.\n");
> + hash_pointers_mode = HASH_PTR_NEVER;
> + } else if (strncmp(str, "always", 6) == 0) {
> + pr_info("Hash pointers mode set to always.\n");
> + hash_pointers_mode = HASH_PTR_ALWAYS;
This mode is not handled anywhere, see below.
> + } else {
> + pr_warn("Unknown hash_pointers mode '%s' specified; assuming auto.\n", str);
> + hash_pointers_mode = HASH_PTR_AUTO;
> + }
We might handle HASH_PTR_ALWAYS by calling:
hash_pointers_update(false);
> return 0;
> }
> +early_param("hash_pointers", hash_pointers_mode_parse);
> +
> +static int __init no_hash_pointers_enable(char *str)
> +{
> + return hash_pointers_mode_parse("never");
> +}
> early_param("no_hash_pointers", no_hash_pointers_enable);
>
> /*
Best Regards,
Petr
Powered by blists - more mailing lists