Re: [PATCH v4 08/18] x86/extable: Add support for immediate form MSR instructions

[Jürgen Groß]

On 29.06.26 19:04, H. Peter Anvin wrote:
> On 2026-06-28 23:55, Juergen Gross wrote:
> > Signed-off-by: Xin Li (Intel) <xin@xxxxxxxxx>
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V2:
> > - new patch, taken from the RFC v2 MSR refactor series by Xin Li
> > V3:
> > - use instruction decoder (Peter Zijlstra)
> > V4:
> > - don't assume %rax for immediate form (Andrew Cooper)
> > ---
> >   arch/x86/mm/extable.c | 46 +++++++++++++++++++++++++++++++++++++------
> >   1 file changed, 40 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
> > index ceb8d03191ab..9974785c8968 100644
> > --- a/arch/x86/mm/extable.c
> > +++ b/arch/x86/mm/extable.c
> > @@ -166,25 +166,59 @@ 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)
> >   {
> > +	unsigned long *regptr;
> > +	struct insn insn;
> > +	bool imm_insn;
> > +	u32 msr;
> > +
> > +	imm_insn = insn_decode_kernel(&insn, (void *)regs->ip) &&
> > +		   insn.vex_prefix.nbytes;
> > +	msr = imm_insn ? insn.immediate.value : (u32)regs->cx;
> > +	regptr = imm_insn ? insn_get_modrm_reg_ptr(&insn, regs) : &regs->ax;
> > +	if (unlikely(!regptr)) {
> > +		pr_err("Inconsistent %sMSR access instruction data at rIP: 0x%lx (%pS)!\n",
> > +		       wrmsr ? "WR" : "RD", regs->ip, (void *)regs->ip);
> > +		show_stack_regs(regs);
> > +		goto out;
> > +	}
> > +
> >   	if (__ONCE_LITE_IF(!safe && wrmsr)) {
> > -		pr_warn("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n",
> > -			(unsigned int)regs->cx, (unsigned int)regs->dx,
> > -			(unsigned int)regs->ax,  regs->ip, (void *)regs->ip);
> > +		/*
> > +		 * To maintain consistency with existing RDMSR and WRMSR(NS) instructions,
> > +		 * the register operand for immediate form MSR instructions is ALWAYS
> > +		 * encoded as RAX in <asm/msr.h> for reading or writing the MSR value.
> > +		 */
> > +		u64 msr_val = *regptr;
>
> This comment is inconsistent with the use of a *regptr. The code is valid
> without this assumption, since you do insn_get_modrm_reg_ptr(), so I think you
> can simply omit the comment.

Indeed, thanks for spotting.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature