Message-ID: <20250826120752.GW4067720@noisy.programming.kicks-ass.net>
Date: Tue, 26 Aug 2025 14:07:52 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Naman Jain <namjain@...ux.microsoft.com>
Cc: "K . Y . Srinivasan" <kys@...rosoft.com>,
	Haiyang Zhang <haiyangz@...rosoft.com>,
	Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H . Peter Anvin" <hpa@...or.com>, linux-hyperv@...r.kernel.org,
	linux-kernel@...r.kernel.org, mhklinux@...look.com
Subject: Re: [PATCH] x86/hyperv: Export hv_hypercall_pg unconditionally

On Tue, Aug 26, 2025 at 05:00:31PM +0530, Naman Jain wrote:
> 
> 
> On 8/25/2025 3:12 PM, Peter Zijlstra wrote:
> > On Mon, Aug 25, 2025 at 11:22:08AM +0530, Naman Jain wrote:
> > > With commit 0e20f1f4c2cb ("x86/hyperv: Clean up hv_do_hypercall()"),
> > > config checks were added to conditionally restrict export
> > > of hv_hypercall_pg symbol at the same time when a usage of that symbol
> > > was added in mshv_vtl_main.c driver. This results in missing symbol
> > > warning when mshv_vtl_main is compiled. Change the logic to
> > > export it unconditionally.
> > > 
> > > Fixes: 96a1d2495c2f ("Drivers: hv: Introduce mshv_vtl driver")
> > > Signed-off-by: Naman Jain <namjain@...ux.microsoft.com>
> > 
> > Oh gawd, that commit is terrible and adds yet another hypercall
> > interface.
> > 
> > I would argue the proper fix is moving the whole of mshv_vtl_return()
> > into the kernel proper and doing it like hv_std_hypercall() on x86_64.
> 
> Thanks for the review comments.
> 
> This is doable, I can move the hypercall part of it to
> arch/x86/hyperv/hv_init.c if I understand correctly.
> 
> > 
> > Additionally, how is that function not utterly broken? What happens if
> > an interrupt or NMI comes in after native_write_cr2() and before the
> > actual hypercall does VMEXIT and trips a #PF?
> 
> mshv_vtl driver is used for OpenHCL paravisor. The interrupts are
> disabled, and NMIs aren't sent to the paravisor by the virt stack.

I do not know what OpenHCL is. Nor is it clear from the code what NMIs
can't happen. Anyway, same can be achieved with breakpoints / kprobes.
You can get a trap after setting CR2 and scribble it.

You simply cannot use CR2 this way.

> > And an rax:rcx return, I though the canonical pair was AX,DX !?!?
> 
> Here, the code uses rax and rcx not as a means to return one 128 bit
> value. The code uses them in that way as an ABI.

Still daft. Creating an ABI that goes against pre-existing conventions
is weird.

> > Also, that STACK_FRAME_NON_STANDARD() annotation is broken, this must
> > not be used for anything that can end up in vmlinux.o -- that is, the
> > moment you built-in this driver (=y) this comes unstuck.
> > 
> > The reason you're getting warnings is because you're violating the
> > normal calling convention and scribbling BP, we yelled at the TDX guys
> > for doing this, now you're getting yelled at. WTF !?!
> > 
> > Please explain how just shutting up objtool makes the unwind work when
> > the NMI hits your BP scribble?
> 
> Returning to a lower VTL treats the base pointer register as a general
> purpose one and this VTL transition function makes sure to preserve the
> bp register due to which the objtool trips over on the assembly touching
> the bp register. We considered this warning harmless and thus we are
> using this macro. Moreover NMIs are not an issue here as they won't be
> coming as mentioned other. If there are alternate approaches that I should
> be using, please suggest.

Using BP in an ABI like that is ridiculous and broken. We told the same
to the TDX folks when they tried, IIRC TDX was fixed.

It is simply not acceptable to break the regular calling convention with
a new ABI.

Again, I can put a breakpoint or kprobe in the region where BP is
scribbled.

Basically the argument is really simple: you run in Linux, you play by
the Linux rules. Using BP as argument is simply not possible. If your
ABI requires that, your ABI is broken and will not be supported. Rev the
ABI and try again. Same for CR2, that is not an available register in
any way.

> I now understand that as part of your effort to enable IBT config on
> x64, you changed the indirect calls to direct calls in Hyper-V.

Yeah, I was cleaning up indirect calls, and this really didn't need to
be one.

> As of today, there is no requirement to enable IBT in OpenHCL kernel
> as this runs as a paravisor in VTL2 and it does not effect the guest
> VMs running
> in VTL0.

I do not know what OpenHCL or VTLn means and as such pretty much the
whole of your statement makes no sense.

Anyway, AFAICT the whole idea of a hypercall page is to 'abtract' out
the VMCALL vs VMMCALL nonsense Intel/AMD inflicted on us. Surely you
don't actually need that. HyperV already knows about all the gazillion
of ways to do hypercalls.

> I can disable CONFIG_X86_KERNEL_IBT when CONFIG_MSHV_VTL is enabled in
> Kconfig in next version.

Or you can just straight up say: "We at microsoft don't care about
security." :-/

Doing that will ensure no distro will build your module, most build bots
will not build your module, nobody cares about your module.

And no, the problems with BP and CR2 are not related to IBT, that is
separate and no less broken. They violate the basic rules of x86_64.

Powered by blists - more mailing lists