[PATCH] KVM: MIPS: Don't leak FPU/DSP to guest

[PATCH] KVM: MIPS: Don't leak FPU/DSP to guest

[James Hogan]

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@imgtec.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Sanjay Lal <sanjayl@kymasys.com>
Cc: Gleb Natapov <gleb@kernel.org>
Cc: kvm@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: <stable@vger.kernel.org> # v3.10+: 044f0f03eca0: MIPS: KVM: Deliver guest interrupts
Cc: <stable@vger.kernel.org> # v3.10+
---
 arch/mips/kvm/locore.S | 2 +-
 arch/mips/kvm/mips.c   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/mips/kvm/locore.S b/arch/mips/kvm/locore.S
index d7279c03c517..4a68b176d6e4 100644
--- a/arch/mips/kvm/locore.S
+++ b/arch/mips/kvm/locore.S
@@ -434,7 +434,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
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index dd133ccecec4..270bbd41769e 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/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>
@@ -379,6 +380,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		vcpu->mmio_needed = 0;
 	}
 
+	lose_fpu(1);
+
 	local_irq_disable();
 	/* Check if we have any exceptions/interrupts pending */
 	kvm_mips_deliver_interrupts(vcpu,
@@ -987,9 +990,6 @@ static 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);
 
-- 
2.0.5

[PATCH] KVM: MIPS: Don't leak FPU/DSP to guest

[James Hogan]

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@imgtec.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Sanjay Lal <sanjayl@kymasys.com>
Cc: Gleb Natapov <gleb@kernel.org>
Cc: kvm@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: <stable@vger.kernel.org> # v3.10+: 044f0f03eca0: MIPS: KVM: Deliver guest interrupts
Cc: <stable@vger.kernel.org> # v3.10+
---
 arch/mips/kvm/locore.S | 2 +-
 arch/mips/kvm/mips.c   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/mips/kvm/locore.S b/arch/mips/kvm/locore.S
index d7279c03c517..4a68b176d6e4 100644
--- a/arch/mips/kvm/locore.S
+++ b/arch/mips/kvm/locore.S
@@ -434,7 +434,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
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index dd133ccecec4..270bbd41769e 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/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>
@@ -379,6 +380,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		vcpu->mmio_needed = 0;
 	}
 
+	lose_fpu(1);
+
 	local_irq_disable();
 	/* Check if we have any exceptions/interrupts pending */
 	kvm_mips_deliver_interrupts(vcpu,
@@ -987,9 +990,6 @@ static 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);
 
-- 
2.0.5

Re: [PATCH] KVM: MIPS: Don't leak FPU/DSP to guest

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 3813 bytes --]

Hi Paolo,

On Wed, Feb 04, 2015 at 05:06:37PM +0000, James Hogan wrote:
> 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)

Please don't apply this patch yet. lose_fpu() uses function symbols
which aren't exported for modules to use yet, so that'll need fixing
first or KVM won't build as a module.

Thanks
James

> 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@imgtec.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Sanjay Lal <sanjayl@kymasys.com>
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: kvm@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: <stable@vger.kernel.org> # v3.10+: 044f0f03eca0: MIPS: KVM: Deliver guest interrupts
> Cc: <stable@vger.kernel.org> # v3.10+
> ---
>  arch/mips/kvm/locore.S | 2 +-
>  arch/mips/kvm/mips.c   | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/mips/kvm/locore.S b/arch/mips/kvm/locore.S
> index d7279c03c517..4a68b176d6e4 100644
> --- a/arch/mips/kvm/locore.S
> +++ b/arch/mips/kvm/locore.S
> @@ -434,7 +434,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
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index dd133ccecec4..270bbd41769e 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/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>
> @@ -379,6 +380,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		vcpu->mmio_needed = 0;
>  	}
>  
> +	lose_fpu(1);
> +
>  	local_irq_disable();
>  	/* Check if we have any exceptions/interrupts pending */
>  	kvm_mips_deliver_interrupts(vcpu,
> @@ -987,9 +990,6 @@ static 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);
>  
> -- 
> 2.0.5
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] KVM: MIPS: Don't leak FPU/DSP to guest

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 3813 bytes --]

Hi Paolo,

On Wed, Feb 04, 2015 at 05:06:37PM +0000, James Hogan wrote:
> 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)

Please don't apply this patch yet. lose_fpu() uses function symbols
which aren't exported for modules to use yet, so that'll need fixing
first or KVM won't build as a module.

Thanks
James

> 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@imgtec.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Sanjay Lal <sanjayl@kymasys.com>
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: kvm@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: <stable@vger.kernel.org> # v3.10+: 044f0f03eca0: MIPS: KVM: Deliver guest interrupts
> Cc: <stable@vger.kernel.org> # v3.10+
> ---
>  arch/mips/kvm/locore.S | 2 +-
>  arch/mips/kvm/mips.c   | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/mips/kvm/locore.S b/arch/mips/kvm/locore.S
> index d7279c03c517..4a68b176d6e4 100644
> --- a/arch/mips/kvm/locore.S
> +++ b/arch/mips/kvm/locore.S
> @@ -434,7 +434,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
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index dd133ccecec4..270bbd41769e 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/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>
> @@ -379,6 +380,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		vcpu->mmio_needed = 0;
>  	}
>  
> +	lose_fpu(1);
> +
>  	local_irq_disable();
>  	/* Check if we have any exceptions/interrupts pending */
>  	kvm_mips_deliver_interrupts(vcpu,
> @@ -987,9 +990,6 @@ static 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);
>  
> -- 
> 2.0.5
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] KVM: MIPS: Don't leak FPU/DSP to guest

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 09/02/2015 23:58, James Hogan wrote:
>> First lets save and disable the FPU (and MSA) state with
>> lose_fpu(1)
> 
> Please don't apply this patch yet. lose_fpu() uses function
> symbols which aren't exported for modules to use yet, so that'll
> need fixing first or KVM won't build as a module.

Well, too late. :)

James/Ralf, should I revert, or can that be fixed during the RC period?

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBAgAGBQJU2brAAAoJEL/70l94x66DdXIIAImur1pdSKYWw1+FzZH+H8Xo
86j9EfptORk554o0a62LG9dOTY+5sJfAV9CoB7Q+8IfdLDKxpk1sLjMkiS0E0EWU
2ilQfjYEXLTgCW38p03ype4m6g4uSfT16dnizrwnUviFk/EvVgCWHy88tA3+Vfn/
WgoxcXkd+hguyNaLR2oAVqyNhAETLTo4kQQqKwGbXFXf0GLno44pj7bJprCR/jlO
4+sUzuV5dno/GI6z8dyMmASo0QEy+IoXJ+aSw+IoRED9nlBMAS4+7uD4XfocGpca
En5KmXVnyJoazgV3Y6w2ymS606S0JNGRcOzqr8ZbOHtjJmAsZxjuVxP6PVzZqQg=
=ozzu
-----END PGP SIGNATURE-----

Re: [PATCH] KVM: MIPS: Don't leak FPU/DSP to guest

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On Tue, Feb 10, 2015 at 09:01:07AM +0100, Paolo Bonzini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> 
> On 09/02/2015 23:58, James Hogan wrote:
> >> First lets save and disable the FPU (and MSA) state with
> >> lose_fpu(1)
> > 
> > Please don't apply this patch yet. lose_fpu() uses function
> > symbols which aren't exported for modules to use yet, so that'll
> > need fixing first or KVM won't build as a module.
> 
> Well, too late. :)
> 
> James/Ralf, should I revert, or can that be fixed during the RC period?

Okay no problem. I have patches ready so I'll submit today.

Sorry about that!

Cheers
James

> 
> Paolo
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQEcBAEBAgAGBQJU2brAAAoJEL/70l94x66DdXIIAImur1pdSKYWw1+FzZH+H8Xo
> 86j9EfptORk554o0a62LG9dOTY+5sJfAV9CoB7Q+8IfdLDKxpk1sLjMkiS0E0EWU
> 2ilQfjYEXLTgCW38p03ype4m6g4uSfT16dnizrwnUviFk/EvVgCWHy88tA3+Vfn/
> WgoxcXkd+hguyNaLR2oAVqyNhAETLTo4kQQqKwGbXFXf0GLno44pj7bJprCR/jlO
> 4+sUzuV5dno/GI6z8dyMmASo0QEy+IoXJ+aSw+IoRED9nlBMAS4+7uD4XfocGpca
> En5KmXVnyJoazgV3Y6w2ymS606S0JNGRcOzqr8ZbOHtjJmAsZxjuVxP6PVzZqQg=
> =ozzu
> -----END PGP SIGNATURE-----

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] KVM: MIPS: Don't leak FPU/DSP to guest

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On Tue, Feb 10, 2015 at 09:01:07AM +0100, Paolo Bonzini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> 
> On 09/02/2015 23:58, James Hogan wrote:
> >> First lets save and disable the FPU (and MSA) state with
> >> lose_fpu(1)
> > 
> > Please don't apply this patch yet. lose_fpu() uses function
> > symbols which aren't exported for modules to use yet, so that'll
> > need fixing first or KVM won't build as a module.
> 
> Well, too late. :)
> 
> James/Ralf, should I revert, or can that be fixed during the RC period?

Okay no problem. I have patches ready so I'll submit today.

Sorry about that!

Cheers
James

> 
> Paolo
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQEcBAEBAgAGBQJU2brAAAoJEL/70l94x66DdXIIAImur1pdSKYWw1+FzZH+H8Xo
> 86j9EfptORk554o0a62LG9dOTY+5sJfAV9CoB7Q+8IfdLDKxpk1sLjMkiS0E0EWU
> 2ilQfjYEXLTgCW38p03ype4m6g4uSfT16dnizrwnUviFk/EvVgCWHy88tA3+Vfn/
> WgoxcXkd+hguyNaLR2oAVqyNhAETLTo4kQQqKwGbXFXf0GLno44pj7bJprCR/jlO
> 4+sUzuV5dno/GI6z8dyMmASo0QEy+IoXJ+aSw+IoRED9nlBMAS4+7uD4XfocGpca
> En5KmXVnyJoazgV3Y6w2ymS606S0JNGRcOzqr8ZbOHtjJmAsZxjuVxP6PVzZqQg=
> =ozzu
> -----END PGP SIGNATURE-----

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]