linux-kernel - Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in 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]

Date:   Mon, 15 Jul 2019 15:03:23 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Arnd Bergmann <arnd@...db.de>, Jann Horn <jannh@...gle.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Radim Krčmář <rkrcmar@...hat.com>
Subject: Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()

On 15/07/19 14:37, Josh Poimboeuf wrote:
> On Mon, Jul 15, 2019 at 11:04:03AM +0200, Paolo Bonzini wrote:
>> On 15/07/19 02:36, Josh Poimboeuf wrote:
>>> With CONFIG_FRAME_POINTER, vmx_vmenter() needs to do frame pointer setup
>>> before calling kvm_spurious_fault().
>>>
>>> Fixes the following warning:
>>>
>>>   arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
>>>
>>> Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
>>
>> This is not enough, because the RSP value must match what is computed at
>> this place:
>>
>>         /* Adjust RSP to account for the CALL to vmx_vmenter(). */
>>         lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
>>         call vmx_update_host_rsp
> 
> Ah, that is surprising :-)
> 
> And then there's this, which overwrites the frame pointer anyway:
> 
> 	mov VCPU_RBP(%_ASM_AX), %_ASM_BP
> 
> Would it make sense to remove the call to vmx_vmenter() altogether, and
> just either embed it in __vmx_vcpu_run(), or jmp back and forth to it
> from __vmx_vcpu_run()?

Unfortunately there's another use of it in nested_vmx_check_vmentry_hw.

The problem is that storing RSP (no matter if adjusted or not) needs a
scratch register.  And vmx_vmenter is exactly the part of the vmentry
that doesn't have any scratch registers available.

Therefore, you must arrange for the caller to store RSP.  You cannot for
example remove it from __vmx_vcpu_run and nested_vmx_check_vmentry_hw in
favor of vmx_vmenter. :(

>> Is this important since kvm_spurious_fault is just BUG()?
> 
> It's probably only important if you care about the stack trace for the
> BUG() case.  But BP is clobbered anyway so I guess it doesn't matter.

Yes, BP is the guest RBP at this point of the code.

Paolo

>> There is no macro currently to support CONFIG_DEBUG_BUGVERBOSE in
>> assembly code, but it's also fine if you just change the call to ud2.
> 
> That would be one way to make objtool happy.
>