linux-kernel - Re: [PATCH] kvm: add proper frame pointer logic for vmx

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: <20190115190617.GD21622@linux.intel.com>
Date:   Tue, 15 Jan 2019 11:06:17 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Qian Cai <cai@....pw>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, rkrcmar@...hat.com,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com,
        x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kvm: add proper frame pointer logic for vmx

+cc Josh

Josh,

The issue we're encountering is that vmx_vcpu_run() is marked with
STACK_FRAME_NON_STANDARD because %rbp is saved/modified/restored when
transitioning to/from the guest.  But vmx_vcpu_run() also happens to
be a giant blob of code that gcc will sometime split into separate
.part.* functions, which means that objtool's symbol lookup to whitelist
the function doesn't work.

On Tue, Jan 15, 2019 at 11:43:20AM -0500, Qian Cai wrote:
> 
> 
> On 1/15/19 2:13 AM, Paolo Bonzini wrote:
> > Hmm, maybe like this:
> > 
> > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> > index bcef2c7e9bc4..33122fa9d4bd 100644
> > --- a/arch/x86/kvm/vmx/vmenter.S
> > +++ b/arch/x86/kvm/vmx/vmenter.S
> > @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
> >  	ret
> > 
> >  2:	vmlaunch
> > +3:
> >  	ret
> > 
> > -3:	cmpb $0, kvm_rebooting
> > -	jne 4f
> > -	call kvm_spurious_fault
> > -4:	ret
> > -
> >  	.pushsection .fixup, "ax"
> > -5:	jmp 3b
> > +4:	cmpb $0, kvm_rebooting
> > +	jne 3b
> > +	jmp kvm_spurious_fault
> >  	.popsection
> > 
> > -	_ASM_EXTABLE(1b, 5b)
> > -	_ASM_EXTABLE(2b, 5b)
> > +	_ASM_EXTABLE(1b, 4b)
> > +	_ASM_EXTABLE(2b, 4b)
> > 
> >  ENDPROC(vmx_vmenter)
> 
> No, that will not work. The problem is in vmx.o where I just sent another patch
> for it.
> 
> I can see there are five options to solve it.
> 
> 1) always inline vmx_vcpu_run()
> 2) always noinline vmx_vcpu_run()
> 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
> 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> 
> Option 1) and 2) seems give away the decision for user with
> CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> 
> Option 3) prevents other functions there for splitting for optimization.
> 
> Option 4) and 5) seems tricky to implement.
> 
> I am not more leaning to 3) as only other fuction will miss splitting is
> vmx_segment_access_rights().

Option 4) is the most correct, but "tricky" is an understatement.  Unless
Josh is willing to pick up the task it'll likely have to wait.

There's actually a few more options:

 6) Replace "pop %rbp" in the vmx_vmenter() asm blob with an open-coded
    equivalent, e.g. "mov [%rsp], %rbp; add $8, %rsp".  This runs an end-
    around on objtool since objtool explicitly keys off "pop %rbp" and NOT
    "mov ..., %rbp" (which is probably an objtool checking flaw?").

 7) Move the vmx_vmenter() asm blob and a few other lines of code into a
    separate helper, e.g. __vmx_vcpu_run(), and mark that as having a
    non-standard stack frame.

Option 6) is an ugly (but amusing) hack.

Arguably we should do option 7) regardless of whether or not objtool gets
updated to support fnsplit behavior, as it it allows objtool to do proper
stack checking on the bulk of vmx_vcpu_run().  And it's a pretty trivial
change.

I'll send a patch for option 7), which in theory should fix the warning
for all .configs.