linux-kernel - Re: [PATCH v3 00/35] x86/bugs: Attack vector controls

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20250110153948.GAZ4E_RAz7aK7Otv5W@fat_crate.local>
Date: Fri, 10 Jan 2025 16:39:48 +0100
From: Borislav Petkov <bp@...en8.de>
To: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
	David Kaplan <david.kaplan@....com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H . Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 00/35] x86/bugs: Attack vector controls

On Fri, Jan 10, 2025 at 12:36:27AM -0800, Pawan Gupta wrote:
> Below patch does some of the above for spectre_v1 mitigation. Please share
> your feedback if this is a good direction to take.

This has come up in the past. My problem with looping over an array of
function pointers and calling them is debuggability: it is not as easy as it
is currently with plain functions.

So we'd need a well-intergrated way to enable debugging of what gets called
when.

> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index ad63b5678250..d719450f89c2 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -53,8 +53,24 @@
>   * mitigation option.
>   */
>  
> -static void __init spectre_v1_select_mitigation(void);
> -static void __init spectre_v1_apply_mitigation(void);
> +struct cpu_mitigation {
> +	unsigned int x86_bug;			/* X86_BUG_* */
> +	int mitigation;				/* AUTO, FULL, NONE etc. */
> +	int mitigates;				/* Attack vectors to mitigate e.g. user->kernel */

This should be an enum.

Even better: you can group those arrays by attack vectors so you simply run
the respective array when you want to enable an attack vector. And then it is
obvious and self-documenting.

> +	char **strings;				/* sysfs status strings */
> +	void (*parse_cmdline)		(struct cpu_mitigation *m);
> +	void (*select_mitigation)	(struct cpu_mitigation *m);
> +	void (*update_mitigation)	(struct cpu_mitigation *m);
> +	void (*apply_mitigation)	(struct cpu_mitigation *m);
> +	void (*ap_init_mitigation)	(struct cpu_mitigation *m); /* Mitigation during secondary CPU init e.g. MSR writes */
> +	void (*smt_update_mitigation)	(struct cpu_mitigation *m); /* Mitigation update on SMT toggle */
> +	void (*sysfs_show_mitigation)	(char *buf);
> +	void (*s3_suspend)		(struct cpu_mitigation *m); /* Mitigation quirks on S3 suspend */
> +	void (*s3_resume)		(struct cpu_mitigation *m); /* Mitigation quirks on S3 resume */

Too many "mitigation" words in there. Perhaps drop "mitigation" from the
function ptr names.

And no side comments pls - all ontop.

Otherwise, once the dust settles here, this could be a nice cleanup.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette