linux-kernel - Re: [PATCH v2 08/12] x86/extable: Add support for immediate form MSR instructions

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250930081442.GG3245006@noisy.programming.kicks-ass.net>
Date: Tue, 30 Sep 2025 10:14:42 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Juergen Gross <jgross@...e.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, xin@...or.com,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH v2 08/12] x86/extable: Add support for immediate form MSR
 instructions

On Tue, Sep 30, 2025 at 09:03:52AM +0200, Juergen Gross wrote:
> From: "Xin Li (Intel)" <xin@...or.com>
> 
> Signed-off-by: Xin Li (Intel) <xin@...or.com>
> Signed-off-by: Juergen Gross <jgross@...e.com>
> ---
> V2:
> - new patch, taken from the RFC v2 MSR refactor series by Xin Li
> ---
>  arch/x86/include/asm/msr.h | 18 ++++++++++++++++++
>  arch/x86/mm/extable.c      | 39 +++++++++++++++++++++++++++++++++-----
>  2 files changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index 71f41af11591..cf5205300266 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -56,6 +56,24 @@ static inline void do_trace_read_msr(u32 msr, u64 val, int failed) {}
>  static inline void do_trace_rdpmc(u32 msr, u64 val, int failed) {}
>  #endif
>  
> +/*
> + * Called only from an MSR fault handler, the instruction pointer points to
> + * the MSR access instruction that caused the fault.
> + */
> +static __always_inline bool is_msr_imm_insn(void *ip)
> +{
> +	/*
> +	 * A full decoder for immediate form MSR instructions appears excessive.
> +	 */
> +#ifdef CONFIG_X86_64
> +	const u8 msr_imm_insn_prefix[] = { 0xc4, 0xe7 };
> +
> +	return !memcmp(ip, msr_imm_insn_prefix, sizeof(msr_imm_insn_prefix));

This seems fragile. Those two bytes are basically the first two bytes of
VEX3 and only indicate VEX3.map7. Which is not very specific, but when
combined with the fact that this is an MSR exception, might just work.

Trouble is that it is also possible to encode the immediate form using
EVEX. If a toolchain were to do that, we'd fail to detect it.

(And there is segment prefix stuffing possible I suppose)

> +#else
> +	return false;
> +#endif
> +}
> +
>  /*
>   * __rdmsr() and __wrmsr() are the two primitives which are the bare minimum MSR
>   * accessors and should not have any tracing or other functionality piggybacking
> diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
> index 2fdc1f1f5adb..c021e4dbc69d 100644
> --- a/arch/x86/mm/extable.c
> +++ b/arch/x86/mm/extable.c
> @@ -166,23 +166,52 @@ static bool ex_handler_uaccess(const struct exception_table_entry *fixup,
>  static bool ex_handler_msr(const struct exception_table_entry *fixup,
>  			   struct pt_regs *regs, bool wrmsr, bool safe, int reg)
>  {
> +	bool imm_insn = is_msr_imm_insn((void *)regs->ip);
> +	u32 msr;
> +
> +	if (imm_insn)
> +		/*
> +		 * The 32-bit immediate specifying a MSR is encoded into
> +		 * byte 5 ~ 8 of an immediate form MSR instruction.
> +		 */
> +		msr = *(u32 *)(regs->ip + 5);
> +	else
> +		msr = (u32)regs->cx;

This seems to have fallen subject to the US tariff induced {} shortage
or something.

Also, EVEX form will have them in other bytes (one further, on account
of EVEX being 4 bytes, rather than 3).

Given this really isn't a fast path or anything, how about we just use
the instruction decoder? Something like:

	struct insn insn;
	u32 msr = (u32)regs->cx;

	ret = insn_decode_kernel(&insn, (void *)regs->ip);
	if (!ret && insn.vex_prefix.nbytes)
		msr = insn.immediate.value;

should do, I suppose. Isn't that both simpler and more robust?