Re: [PATCH] tty: sysrq: Introduce compile-time crash-only mode

[Jiri Slaby <jirislaby@kernel.org>]

On 07. 06. 25, 17:19, Marwan Seliem wrote:
> This commit introduces a new Kconfig option, CONFIG_MAGIC_SYSRQ_CRASH_ONLY,
> which allows for a significant hardening of the system by restricting
> the Magic SysRq functionality at compile time.
> 
> Security Impact:
> - Reduces attack surface by disabling non-essential SysRq commands
> - Maintains critical crash-dump capability required for debugging
> - Eliminates runtime configuration vulnerabilities
> 
> When CONFIG_MAGIC_SYSRQ_CRASH_ONLY is enabled:
> 
> 1.  Restricted Commands: Only the 'c' (trigger a system crash/dump)
>      SysRq command remains operational. All other built-in SysRq commands
>      (e.g., reboot, sync, show-memory, SAK) are disabled.

I must admit I don't much understand the purpose of this. It can be 
spelled as: you can crash the system only by sysrq-c from now on. Don't 
use sysrq-r or others. Who did ask for this?

...

> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c

> @@ -584,7 +620,6 @@ void __handle_sysrq(u8 key, bool check_mask)
>   {
>   	const struct sysrq_key_op *op_p;
>   	int orig_suppress_printk;
> -	int i;
>   
>   	orig_suppress_printk = suppress_printk;
>   	suppress_printk = 0;
> @@ -599,7 +634,15 @@ void __handle_sysrq(u8 key, bool check_mask)
>   	 */
>   	printk_force_console_enter();
>   
> +#ifdef CONFIG_MAGIC_SYSRQ_CRASH_ONLY
> +	if (key != 'c') { /* In CRASH_ONLY mode, only 'c' is considered */
> +		op_p = NULL;
> +	} else {
> +		op_p = __sysrq_get_key_op(key);
> +	}
> +#else
>   	op_p = __sysrq_get_key_op(key);
> +#endif

These inline #ifdefs are horrid.

>   	if (op_p) {
>   		/*
>   		 * Should we check for enabled operations (/proc/sysrq-trigger
...
> @@ -1104,6 +1157,10 @@ static inline void sysrq_unregister_handler(void)
>   
>   int sysrq_toggle_support(int enable_mask)
>   {
> +#ifdef CONFIG_MAGIC_SYSRQ_CRASH_ONLY
> +	pr_warn("SysRq: CONFIG_MAGIC_SYSRQ_CRASH_ONLY is set. Runtime toggle is not allowed.\n");

This can be invoked from userspace. So you can nicely DoS the machine by 
the added warn, right? Hint: use ratelimiting.

> +	return -EPERM;
> +#else
>   	bool was_enabled = sysrq_on();
>   
>   	sysrq_enabled = enable_mask;
...
> @@ -1145,12 +1203,30 @@ static int __sysrq_swap_key_ops(u8 key, const struct sysrq_key_op *insert_op_p,
>   
>   int register_sysrq_key(u8 key, const struct sysrq_key_op *op_p)
>   {
> +#ifdef CONFIG_MAGIC_SYSRQ_CRASH_ONLY
> +	/*
> +	 * In CRASH_ONLY mode, do not allow registering new SysRq ops.
> +	 */
> +	pr_warn("SysRq: CONFIG_MAGIC_SYSRQ_CRASH_ONLY is set. Cannot register new SysRq key '%c'.\n", key);
> +	return -EPERM;
> +#endif
>   	return __sysrq_swap_key_ops(key, op_p, NULL);
>   }
>   EXPORT_SYMBOL(register_sysrq_key);
>   
>   int unregister_sysrq_key(u8 key, const struct sysrq_key_op *op_p)
>   {
> +#ifdef CONFIG_MAGIC_SYSRQ_CRASH_ONLY
> +	/*
> +	 * In CRASH_ONLY mode, do not allow unregistering the crash op.
> +	 * Other ops should be NULL anyway due to sysrq_init_crash_only_table.
> +	 */
> +	if (op_p == &sysrq_crash_op) {
> +		pr_warn("SysRq: CONFIG_MAGIC_SYSRQ_CRASH_ONLY is set. Cannot unregister the crash SysRq key '%c'.\n", key);
> +		return -EPERM;

No need for this return ^^.

> +	}
> +	return -EPERM; /* Attempt to unregister anything else is also an error */
> +#endif
>   	return __sysrq_swap_key_ops(key, NULL, op_p);
>   }
>   EXPORT_SYMBOL(unregister_sysrq_key);
> @@ -1209,6 +1285,7 @@ static inline void sysrq_init_procfs(void)
>   static int __init sysrq_init(void)
>   {
>   	sysrq_init_procfs();
> +	sysrq_init_crash_only_table();
>   
>   	if (sysrq_on())
>   		sysrq_register_handler();
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ebe33181b6e6..c05b80cfb8aa 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -640,6 +640,19 @@ config MAGIC_SYSRQ_DEFAULT_ENABLE
>   	  This may be set to 1 or 0 to enable or disable them all, or
>   	  to a bitmask as described in Documentation/admin-guide/sysrq.rst.
>   
> +config MAGIC_SYSRQ_CRASH_ONLY
> +	bool "Restrict Magic SysRq to crash command only"
> +	depends on MAGIC_SYSRQ
> +	default n
> +	help
> +	  If you say Y here, the Magic SysRq key functionality will be
> +	  severely restricted at compile time. Only the 'c' command (trigger
> +	  a system crash) will be available. All other SysRq commands will be
> +	  disabled, and no new SysRq commands can be registered at runtime.
> +	  The /proc/sys/kernel/sysrq setting will be ineffective for
> +	  non-crash commands, and attempts to change it may be blocked.
> +	  This is a security hardening option.

Is it for real?

thanks,
-- 
js
suse labs