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

[Marwan Seliem <marwanmhks@gmail.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 2417 bytes --]

Hi Jiri,

Thank you for your review and feedback. Let me address your comments and provide more context about the use case for this change.

> 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?

This change was created with embedded systems that have external subsystems in mind (like modems/co-processors) where we need:
    - The ability to trigger a full system crash (via sysrq-c) to collect subsystem crash dumps
    - While explicitly disabling all other sysrq functionality for security reasons

In these environments:
    - Crash dumps are essential for debugging
	- Other sysrq commands pose unnecessary security risks

> These inline #ifdefs are horrid.

Agreed. I will restructure this.

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

Good point. I'll add ratelimiting to the pr_warn() calls or we can consider reducing these to pr_debug()?

> No need for this return ^^.

You're right, the second return is redundant. I'll clean this up.

> Is it for real?

From a pure security viewpoint, expert advice is to remove this Magic Sysrq functionality, 
either with kernel.sysrq=0 in sysctl config file, or with a full kernel rebuild 
with n value for CONFIG_MAGIC_SYSRQ parameter.
This patch provides a middle ground that:
    1) Resolves the Core Security Conflict
		The CRASH_ONLY mode provides the minimal debug capability while eliminating:
            - Register dumps (disables 'p' command)
            - Filesystem operations (disables 'u'/sync commands)
            - All other privileged operations
    2) Security Architecture Benefits
	
		Traditional: All-or-nothing  
		│─────────────┬─────────────│  
			Full disable			Full enable  

		Our Approach: Principle of Least Privilege  
		│─────┬───────┬─────────────│  
			Off	Crash-only		Full enable  

For v2 of the patch, I'll make these improvements:
    1) Restructure to minimize #ifdef fiasco
	2) Add proper rate limiting on pr_warn/change it to pr_debug
	3) Clean up redundant return

Thank you again for your valuable feedback. I appreciate you taking the time to review this.

Warmest regards,
Marwan Seliem