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: <79ca36a3-6d2c-4851-b29a-24eacc6c32c0@suse.com>
Date: Tue, 30 Sep 2025 10:31:17 +0200
From: Jürgen Groß <jgross@...e.com>
To: Peter Zijlstra <peterz@...radead.org>
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 30.09.25 10:14, Peter Zijlstra wrote:
> 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?
> 

Works for me.


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)