Re: [PATCH 3.14 58/73] KVM: MIPS: Dont leak FPU/DSP to guest

[James Hogan]

Hi Greg,

On Tue, Mar 03, 2015 at 10:13:26PM -0800, Greg Kroah-Hartman wrote:
> 3.14-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: James Hogan <james.hogan@xxxxxxxxxx>
> 
> commit f798217dfd038af981a18bbe4bc57027a08bb182 upstream.
> 
> The FPU and DSP are enabled via the CP0 Status CU1 and MX bits by
> kvm_mips_set_c0_status() on a guest exit, presumably in case there is
> active state that needs saving if pre-emption occurs. However neither of
> these bits are cleared again when returning to the guest.
> 
> This effectively gives the guest access to the FPU/DSP hardware after
> the first guest exit even though it is not aware of its presence,
> allowing FP instructions in guest user code to intermittently actually
> execute instead of trapping into the guest OS for emulation. It will
> then read & manipulate the hardware FP registers which technically
> belong to the user process (e.g. QEMU), or are stale from another user
> process. It can also crash the guest OS by causing an FP exception, for
> which a guest exception handler won't have been registered.
> 
> First lets save and disable the FPU (and MSA) state with lose_fpu(1)
> before entering the guest. This simplifies the problem, especially for
> when guest FPU/MSA support is added in the future, and prevents FR=1 FPU
> state being live when the FR bit gets cleared for the guest, which
> according to the architecture causes the contents of the FPU and vector
> registers to become UNPREDICTABLE.
> 
> We can then safely remove the enabling of the FPU in
> kvm_mips_set_c0_status(), since there should never be any active FPU or
> MSA state to save at pre-emption, which should plug the FPU leak.
> 
> DSP state is always live rather than being lazily restored, so for that
> it is simpler to just clear the MX bit again when re-entering the guest.
> 
> Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> Cc: Sanjay Lal <sanjayl@xxxxxxxxxxx>
> Cc: Gleb Natapov <gleb@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: linux-mips@xxxxxxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx> # v3.10+: 044f0f03eca0: MIPS: KVM: Deliver guest interrupts

The original 3.10 and 3.12/3.14 backports had this added:
Cc: <stable@xxxxxxxxxxxxxxx> # v3.10+: 3ce465e04bfd: MIPS: Export FP functions used by lose_fpu(1) for KVM                                         
Which I can't see included in the v3.10 stable queue or branch. It fixes
a build error with MIPS malta_kvm_defconfig (MIPS=y, KVM=m) after this
patch is applied.

Same applies to the 3.14 queue too I think.

Cheers
James

> Cc: <stable@xxxxxxxxxxxxxxx> # v3.10+
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
> This should apply to stable trees 3.12 and 3.14, but not 3.10. The files
> had been renamed since v3.14 so it cherry-picked cleanly but the patch
> didn't apply cleanly. I've also added a reference to the "MIPS: Export
> FP functions used by lose_fpu(1) for KVM" commit which is itself marked
> for stable, but is needed to avoid a build failure when KVM=m.
> ---
>  arch/mips/kvm/kvm_locore.S |    2 +-
>  arch/mips/kvm/kvm_mips.c   |    6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> --- a/arch/mips/kvm/kvm_locore.S
> +++ b/arch/mips/kvm/kvm_locore.S
> @@ -428,7 +428,7 @@ __kvm_mips_return_to_guest:
>  	/* Setup status register for running guest in UM */
>  	.set	at
>  	or	v1, v1, (ST0_EXL | KSU_USER | ST0_IE)
> -	and	v1, v1, ~ST0_CU0
> +	and	v1, v1, ~(ST0_CU0 | ST0_MX)
>  	.set	noat
>  	mtc0	v1, CP0_STATUS
>  	ehb
> --- a/arch/mips/kvm/kvm_mips.c
> +++ b/arch/mips/kvm/kvm_mips.c
> @@ -15,6 +15,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/fs.h>
>  #include <linux/bootmem.h>
> +#include <asm/fpu.h>
>  #include <asm/page.h>
>  #include <asm/cacheflush.h>
>  #include <asm/mmu_context.h>
> @@ -418,6 +419,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_v
>  		vcpu->mmio_needed = 0;
>  	}
>  
> +	lose_fpu(1);
> +
>  	local_irq_disable();
>  	/* Check if we have any exceptions/interrupts pending */
>  	kvm_mips_deliver_interrupts(vcpu,
> @@ -1021,9 +1024,6 @@ void kvm_mips_set_c0_status(void)
>  {
>  	uint32_t status = read_c0_status();
>  
> -	if (cpu_has_fpu)
> -		status |= (ST0_CU1);
> -
>  	if (cpu_has_dsp)
>  		status |= (ST0_MX);
>  
> 
>

Attachment: signature.asc
Description: Digital signature