diff mbox

[3.13.y.z,extended,stable] Patch "KVM: x86: Emulator does not decode clflush well" has been added to staging queue

Message ID 1414788825-11577-1-git-send-email-kamal@canonical.com
State New
Headers show

Commit Message

Kamal Mostafa Oct. 31, 2014, 8:53 p.m. UTC 
This is a note to let you know that I have just added a patch titled

    KVM: x86: Emulator does not decode clflush well

to the linux-3.13.y-queue branch of the 3.13.y.z extended stable tree 
which can be found at:

 http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=shortlog;h=refs/heads/linux-3.13.y-queue

This patch is scheduled to be released in version 3.13.11.11.

If you, or anyone else, feels it should not be added to this tree, please 
reply to this email.

For more information about the 3.13.y.z tree, see
https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable

Thanks.
-Kamal

------

From 3cf1cc997f89242c852dca2469ca4303348d29a5 Mon Sep 17 00:00:00 2001
From: Nadav Amit <namit@cs.technion.ac.il>
Date: Mon, 13 Oct 2014 13:04:13 +0300
Subject: KVM: x86: Emulator does not decode clflush well

commit 13e457e0eebf0a0c82c38ceb890d93eb826d62a6 upstream.

Currently, all group15 instructions are decoded as clflush (e.g., mfence,
xsave).  In addition, the clflush instruction requires no prefix (66/f2/f3)
would exist. If prefix exists it may encode a different instruction (e.g.,
clflushopt).

Creating a group for clflush, and different group for each prefix.

This has been the case forever, but the next patch needs the cflush group
in order to fix a bug introduced in 3.17.

Fixes: 41061cdb98a0bec464278b4db8e894a3121671f5
Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
---
 arch/x86/kvm/emulate.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

--
1.9.1

Comments

Luis Henriques Nov. 3, 2014, 5:36 p.m. UTC | #1 
On Fri, Oct 31, 2014 at 01:53:45PM -0700, Kamal Mostafa wrote:
> This is a note to let you know that I have just added a patch titled
> 
>     KVM: x86: Emulator does not decode clflush well
> 
> to the linux-3.13.y-queue branch of the 3.13.y.z extended stable tree 
> which can be found at:
> 
>  http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=shortlog;h=refs/heads/linux-3.13.y-queue
> 
> This patch is scheduled to be released in version 3.13.11.11.
> 
> If you, or anyone else, feels it should not be added to this tree, please 
> reply to this email.
> 
> For more information about the 3.13.y.z tree, see
> https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable
> 
> Thanks.
> -Kamal
> 
> ------
> 
> From 3cf1cc997f89242c852dca2469ca4303348d29a5 Mon Sep 17 00:00:00 2001
> From: Nadav Amit <namit@cs.technion.ac.il>
> Date: Mon, 13 Oct 2014 13:04:13 +0300
> Subject: KVM: x86: Emulator does not decode clflush well
> 
> commit 13e457e0eebf0a0c82c38ceb890d93eb826d62a6 upstream.
> 
> Currently, all group15 instructions are decoded as clflush (e.g., mfence,
> xsave).  In addition, the clflush instruction requires no prefix (66/f2/f3)
> would exist. If prefix exists it may encode a different instruction (e.g.,
> clflushopt).
> 
> Creating a group for clflush, and different group for each prefix.
> 
> This has been the case forever, but the next patch needs the cflush group
> in order to fix a bug introduced in 3.17.

Given the above ^^^

> Fixes: 41061cdb98a0bec464278b4db8e894a3121671f5

And the fact that this commit isn't present in 3.13, I'm not sure this
patch is relevant for this stable kernel.  Could someone confirm this
please?

Cheers,
--
Luís

> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> ---
>  arch/x86/kvm/emulate.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 38d3751..ed2db60 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3375,6 +3375,12 @@ static int em_bswap(struct x86_emulate_ctxt *ctxt)
>  	return X86EMUL_CONTINUE;
>  }
> 
> +static int em_clflush(struct x86_emulate_ctxt *ctxt)
> +{
> +	/* emulating clflush regardless of cpuid */
> +	return X86EMUL_CONTINUE;
> +}
> +
>  static bool valid_cr(int nr)
>  {
>  	switch (nr) {
> @@ -3711,6 +3717,16 @@ static const struct opcode group11[] = {
>  	X7(D(Undefined)),
>  };
> 
> +static const struct gprefix pfx_0f_ae_7 = {
> +	I(0, em_clflush), N, N, N,
> +};
> +
> +static const struct group_dual group15 = { {
> +	N, N, N, N, N, N, N, GP(0, &pfx_0f_ae_7),
> +}, {
> +	N, N, N, N, N, N, N, N,
> +} };
> +
>  static const struct gprefix pfx_0f_6f_0f_7f = {
>  	I(Mmx, em_mov), I(Sse | Aligned, em_mov), N, I(Sse | Unaligned, em_mov),
>  };
> @@ -3962,7 +3978,7 @@ static const struct opcode twobyte_table[256] = {
>  	F(DstMem | SrcReg | ModRM | BitOp | Lock | PageTable, em_bts),
>  	F(DstMem | SrcReg | Src2ImmByte | ModRM, em_shrd),
>  	F(DstMem | SrcReg | Src2CL | ModRM, em_shrd),
> -	D(ModRM), F(DstReg | SrcMem | ModRM, em_imul),
> +	GD(0, &group15), F(DstReg | SrcMem | ModRM, em_imul),
>  	/* 0xB0 - 0xB7 */
>  	I2bv(DstMem | SrcReg | ModRM | Lock | PageTable, em_cmpxchg),
>  	I(DstReg | SrcMemFAddr | ModRM | Src2SS, em_lseg),
> @@ -4838,8 +4854,6 @@ twobyte_insn:
>  	case 0x90 ... 0x9f:     /* setcc r/m8 */
>  		ctxt->dst.val = test_cc(ctxt->b, ctxt->eflags);
>  		break;
> -	case 0xae:              /* clflush */
> -		break;
>  	case 0xb6 ... 0xb7:	/* movzx */
>  		ctxt->dst.bytes = ctxt->op_bytes;
>  		ctxt->dst.val = (ctxt->src.bytes == 1) ? (u8) ctxt->src.val
> --
> 1.9.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Nadav Amit Nov. 3, 2014, 8:22 p.m. UTC | #2 
> On Nov 3, 2014, at 19:36, Luis Henriques <luis.henriques@canonical.com> wrote:
> 
> On Fri, Oct 31, 2014 at 01:53:45PM -0700, Kamal Mostafa wrote:
>> This is a note to let you know that I have just added a patch titled
>> 
>>    KVM: x86: Emulator does not decode clflush well
>> 
>> to the linux-3.13.y-queue branch of the 3.13.y.z extended stable tree 
>> which can be found at:
>> 
>> http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=shortlog;h=refs/heads/linux-3.13.y-queue
>> 
>> This patch is scheduled to be released in version 3.13.11.11.
>> 
>> If you, or anyone else, feels it should not be added to this tree, please 
>> reply to this email.
>> 
>> For more information about the 3.13.y.z tree, see
>> https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable
>> 
>> Thanks.
>> -Kamal
>> 
>> ------
>> 
>> From 3cf1cc997f89242c852dca2469ca4303348d29a5 Mon Sep 17 00:00:00 2001
>> From: Nadav Amit <namit@cs.technion.ac.il>
>> Date: Mon, 13 Oct 2014 13:04:13 +0300
>> Subject: KVM: x86: Emulator does not decode clflush well
>> 
>> commit 13e457e0eebf0a0c82c38ceb890d93eb826d62a6 upstream.
>> 
>> Currently, all group15 instructions are decoded as clflush (e.g., mfence,
>> xsave).  In addition, the clflush instruction requires no prefix (66/f2/f3)
>> would exist. If prefix exists it may encode a different instruction (e.g.,
>> clflushopt).
>> 
>> Creating a group for clflush, and different group for each prefix.
>> 
>> This has been the case forever, but the next patch needs the cflush group
>> in order to fix a bug introduced in 3.17.
> 
> Given the above ^^^
> 
>> Fixes: 41061cdb98a0bec464278b4db8e894a3121671f5
> 
> And the fact that this commit isn't present in 3.13, I'm not sure this
> patch is relevant for this stable kernel.  Could someone confirm this
> please?

Yes. IMO this patch should not be required for 3.13 as a security patch.
Note that it still improves the emulation of guest code, but we did not see a real-life scenario that should be affected.

Nadav
Paolo Bonzini Nov. 4, 2014, 9:56 a.m. UTC | #3 
>> From 3cf1cc997f89242c852dca2469ca4303348d29a5 Mon Sep 17 00:00:00 2001
>> From: Nadav Amit <namit@cs.technion.ac.il>
>> Date: Mon, 13 Oct 2014 13:04:13 +0300
>> Subject: KVM: x86: Emulator does not decode clflush well
>>
>> commit 13e457e0eebf0a0c82c38ceb890d93eb826d62a6 upstream.
>>
>> Currently, all group15 instructions are decoded as clflush (e.g., mfence,
>> xsave).  In addition, the clflush instruction requires no prefix (66/f2/f3)
>> would exist. If prefix exists it may encode a different instruction (e.g.,
>> clflushopt).
>>
>> Creating a group for clflush, and different group for each prefix.
>>
>> This has been the case forever, but the next patch needs the cflush group
>> in order to fix a bug introduced in 3.17.
> 
> Given the above ^^^
> 
>> Fixes: 41061cdb98a0bec464278b4db8e894a3121671f5
> 
> And the fact that this commit isn't present in 3.13, I'm not sure this
> patch is relevant for this stable kernel.  Could someone confirm this
> please?

Yes, indeed I thought the "Fixes:" header was also used to find which
commits should go in which stable kernel.

Paolo
Luis Henriques Nov. 4, 2014, 11:15 a.m. UTC | #4 
On Mon, Nov 03, 2014 at 10:22:58PM +0200, Nadav Amit wrote:
> 
> > On Nov 3, 2014, at 19:36, Luis Henriques <luis.henriques@canonical.com> wrote:
> > 
> > On Fri, Oct 31, 2014 at 01:53:45PM -0700, Kamal Mostafa wrote:
> >> This is a note to let you know that I have just added a patch titled
> >> 
> >>    KVM: x86: Emulator does not decode clflush well
> >> 
> >> to the linux-3.13.y-queue branch of the 3.13.y.z extended stable tree 
> >> which can be found at:
> >> 
> >> http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=shortlog;h=refs/heads/linux-3.13.y-queue
> >> 
> >> This patch is scheduled to be released in version 3.13.11.11.
> >> 
> >> If you, or anyone else, feels it should not be added to this tree, please 
> >> reply to this email.
> >> 
> >> For more information about the 3.13.y.z tree, see
> >> https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable
> >> 
> >> Thanks.
> >> -Kamal
> >> 
> >> ------
> >> 
> >> From 3cf1cc997f89242c852dca2469ca4303348d29a5 Mon Sep 17 00:00:00 2001
> >> From: Nadav Amit <namit@cs.technion.ac.il>
> >> Date: Mon, 13 Oct 2014 13:04:13 +0300
> >> Subject: KVM: x86: Emulator does not decode clflush well
> >> 
> >> commit 13e457e0eebf0a0c82c38ceb890d93eb826d62a6 upstream.
> >> 
> >> Currently, all group15 instructions are decoded as clflush (e.g., mfence,
> >> xsave).  In addition, the clflush instruction requires no prefix (66/f2/f3)
> >> would exist. If prefix exists it may encode a different instruction (e.g.,
> >> clflushopt).
> >> 
> >> Creating a group for clflush, and different group for each prefix.
> >> 
> >> This has been the case forever, but the next patch needs the cflush group
> >> in order to fix a bug introduced in 3.17.
> > 
> > Given the above ^^^
> > 
> >> Fixes: 41061cdb98a0bec464278b4db8e894a3121671f5
> > 
> > And the fact that this commit isn't present in 3.13, I'm not sure this
> > patch is relevant for this stable kernel.  Could someone confirm this
> > please?
> 
> Yes. IMO this patch should not be required for 3.13 as a security patch.
> Note that it still improves the emulation of guest code, but we did
> not see a real-life scenario that should be affected.
> 
> Nadav
> 

Thanks a lot for looking.  I had the same question regarding the 3.16
kernel :-)

Cheers,
--
Luís
Kamal Mostafa Nov. 5, 2014, 6:17 p.m. UTC | #5 
On Tue, 2014-11-04 at 10:56 +0100, Paolo Bonzini wrote:
> >> From 3cf1cc997f89242c852dca2469ca4303348d29a5 Mon Sep 17 00:00:00 2001
> >> From: Nadav Amit <namit@cs.technion.ac.il>
> >> Date: Mon, 13 Oct 2014 13:04:13 +0300
> >> Subject: KVM: x86: Emulator does not decode clflush well
> >>
> >> commit 13e457e0eebf0a0c82c38ceb890d93eb826d62a6 upstream.
> >>
> >> Currently, all group15 instructions are decoded as clflush (e.g., mfence,
> >> xsave).  In addition, the clflush instruction requires no prefix (66/f2/f3)
> >> would exist. If prefix exists it may encode a different instruction (e.g.,
> >> clflushopt).
> >>
> >> Creating a group for clflush, and different group for each prefix.
> >>
> >> This has been the case forever, but the next patch needs the cflush group
> >> in order to fix a bug introduced in 3.17.
> > 
> > Given the above ^^^
> > 
> >> Fixes: 41061cdb98a0bec464278b4db8e894a3121671f5
> > 
> > And the fact that this commit isn't present in 3.13, I'm not sure this
> > patch is relevant for this stable kernel.  Could someone confirm this
> > please?
> 
> Yes, indeed I thought the "Fixes:" header was also used to find which
> commits should go in which stable kernel.

My (mis-)reading of this one led me to think it was fixing an additional
useful thing, in addition to the bug introduced in 3.17.  I'll drop it
from the 3.13-stable queue.

Thanks for the correction Luis, Nadav, and Paolo!

 -Kamal
diff mbox

Patch

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 38d3751..ed2db60 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3375,6 +3375,12 @@  static int em_bswap(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }

+static int em_clflush(struct x86_emulate_ctxt *ctxt)
+{
+	/* emulating clflush regardless of cpuid */
+	return X86EMUL_CONTINUE;
+}
+
 static bool valid_cr(int nr)
 {
 	switch (nr) {
@@ -3711,6 +3717,16 @@  static const struct opcode group11[] = {
 	X7(D(Undefined)),
 };

+static const struct gprefix pfx_0f_ae_7 = {
+	I(0, em_clflush), N, N, N,
+};
+
+static const struct group_dual group15 = { {
+	N, N, N, N, N, N, N, GP(0, &pfx_0f_ae_7),
+}, {
+	N, N, N, N, N, N, N, N,
+} };
+
 static const struct gprefix pfx_0f_6f_0f_7f = {
 	I(Mmx, em_mov), I(Sse | Aligned, em_mov), N, I(Sse | Unaligned, em_mov),
 };
@@ -3962,7 +3978,7 @@  static const struct opcode twobyte_table[256] = {
 	F(DstMem | SrcReg | ModRM | BitOp | Lock | PageTable, em_bts),
 	F(DstMem | SrcReg | Src2ImmByte | ModRM, em_shrd),
 	F(DstMem | SrcReg | Src2CL | ModRM, em_shrd),
-	D(ModRM), F(DstReg | SrcMem | ModRM, em_imul),
+	GD(0, &group15), F(DstReg | SrcMem | ModRM, em_imul),
 	/* 0xB0 - 0xB7 */
 	I2bv(DstMem | SrcReg | ModRM | Lock | PageTable, em_cmpxchg),
 	I(DstReg | SrcMemFAddr | ModRM | Src2SS, em_lseg),
@@ -4838,8 +4854,6 @@  twobyte_insn:
 	case 0x90 ... 0x9f:     /* setcc r/m8 */
 		ctxt->dst.val = test_cc(ctxt->b, ctxt->eflags);
 		break;
-	case 0xae:              /* clflush */
-		break;
 	case 0xb6 ... 0xb7:	/* movzx */
 		ctxt->dst.bytes = ctxt->op_bytes;
 		ctxt->dst.val = (ctxt->src.bytes == 1) ? (u8) ctxt->src.val