Re: [Xen-devel] [PATCH for-4.8] x86/svm: Don't clobber eax and edx if an RDMSR intercept fails

[Wei Liu]

On Wed, Nov 09, 2016 at 12:28:27PM +, Andrew Cooper wrote:
> The original code has a bug; eax and edx get unconditionally updated even when
> hvm_msr_read_intercept() doesn't return X86EMUL_OKAY.
> 
> It is only by blind luck (vmce_rdmsr() eagerly initialising its msr_content
> pointer) that this isn't an information leak into guests.
> 
> While fixing this bug, reduce the scope of msr_content and initialise it to 0.
> This makes it obvious that a stack leak won't occur, even if there were to be
> a buggy codepath in hvm_msr_read_intercept().
> 
> Also make some non-functional improvements.  Make the insn_len calculation
> common, and reduce the quantity of explicit casting by making better use of
> the existing register names.
> 
> Signed-off-by: Andrew Cooper 

Release-acked-by: Wei Liu 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.8] x86/svm: Don't clobber eax and edx if an RDMSR intercept fails

[Andrew Cooper]

On 09/11/16 14:14, Jan Beulich wrote:
 On 09.11.16 at 13:28,  wrote:
>> The original code has a bug; eax and edx get unconditionally updated even 
>> when
>> hvm_msr_read_intercept() doesn't return X86EMUL_OKAY.
>>
>> It is only by blind luck (vmce_rdmsr() eagerly initialising its msr_content
>> pointer) that this isn't an information leak into guests.
>>
>> While fixing this bug, reduce the scope of msr_content and initialise it to 
>> 0.
>> This makes it obvious that a stack leak won't occur, even if there were to be
>> a buggy codepath in hvm_msr_read_intercept().
>>
>> Also make some non-functional improvements.  Make the insn_len calculation
>> common, and reduce the quantity of explicit casting by making better use of
>> the existing register names.
>>
>> Signed-off-by: Andrew Cooper 
> Reviewed-by: Jan Beulich 
> with two possible further adjustments:
>
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1948,26 +1948,28 @@ static int svm_msr_write_intercept(unsigned int msr, 
>> uint64_t msr_content)
>>  
>>  static void svm_do_msr_access(struct cpu_user_regs *regs)
>>  {
>> -int rc, inst_len;
>>  struct vcpu *v = current;
>> -struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>> -uint64_t msr_content;
>> +bool rdmsr = v->arch.hvm_svm.vmcb->exitinfo1 == 0;
>> +int rc, inst_len = __get_instruction_length(
>> +v, rdmsr ? INSTR_RDMSR : INSTR_WRMSR);
> With all of this I think it wouldn't make the patch worse to look at if
> you also renamed v -> curr.

I am sure at least one version of this patch had that adjustment.  I
will fix it.

>
>> +if ( inst_len == 0 )
>> +return;
>>  
>> -if ( vmcb->exitinfo1 == 0 )
>> +if ( rdmsr )
>>  {
>> -if ( (inst_len = __get_instruction_length(v, INSTR_RDMSR)) == 0 )
>> -return;
>> -rc = hvm_msr_read_intercept(regs->ecx, _content);
>> -regs->eax = (uint32_t)msr_content;
>> -regs->edx = (uint32_t)(msr_content >> 32);
>> +uint64_t msr_content = 0;
>> +
>> +rc = hvm_msr_read_intercept(regs->_ecx, _content);
>> +if ( rc == X86EMUL_OKAY )
>> +{
>> +regs->rax = (uint32_t)msr_content;
>> +regs->rdx = (uint32_t)(msr_content >> 32);
> While the first of the casts is needed, the second one isn't.

Right, but removing the cast make the code harder to read.  This at
least is visually obvious that msr_content is being split in half.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.8] x86/svm: Don't clobber eax and edx if an RDMSR intercept fails

[Boris Ostrovsky]

On 11/09/2016 07:28 AM, Andrew Cooper wrote:
> The original code has a bug; eax and edx get unconditionally updated even when
> hvm_msr_read_intercept() doesn't return X86EMUL_OKAY.
>
> It is only by blind luck (vmce_rdmsr() eagerly initialising its msr_content
> pointer) that this isn't an information leak into guests.
>
> While fixing this bug, reduce the scope of msr_content and initialise it to 0.
> This makes it obvious that a stack leak won't occur, even if there were to be
> a buggy codepath in hvm_msr_read_intercept().
>
> Also make some non-functional improvements.  Make the insn_len calculation
> common, and reduce the quantity of explicit casting by making better use of
> the existing register names.
>
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Boris Ostrovsky 
> CC: Suravee Suthikulpanit 
> CC: Wei Liu 

Reviewed-by: Boris Ostrovsky 

(with or without changes suggested by Jan)


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.8] x86/svm: Don't clobber eax and edx if an RDMSR intercept fails

[Jan Beulich]

>>> On 09.11.16 at 13:28,  wrote:
> The original code has a bug; eax and edx get unconditionally updated even when
> hvm_msr_read_intercept() doesn't return X86EMUL_OKAY.
> 
> It is only by blind luck (vmce_rdmsr() eagerly initialising its msr_content
> pointer) that this isn't an information leak into guests.
> 
> While fixing this bug, reduce the scope of msr_content and initialise it to 0.
> This makes it obvious that a stack leak won't occur, even if there were to be
> a buggy codepath in hvm_msr_read_intercept().
> 
> Also make some non-functional improvements.  Make the insn_len calculation
> common, and reduce the quantity of explicit casting by making better use of
> the existing register names.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
with two possible further adjustments:

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1948,26 +1948,28 @@ static int svm_msr_write_intercept(unsigned int msr, 
> uint64_t msr_content)
>  
>  static void svm_do_msr_access(struct cpu_user_regs *regs)
>  {
> -int rc, inst_len;
>  struct vcpu *v = current;
> -struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> -uint64_t msr_content;
> +bool rdmsr = v->arch.hvm_svm.vmcb->exitinfo1 == 0;
> +int rc, inst_len = __get_instruction_length(
> +v, rdmsr ? INSTR_RDMSR : INSTR_WRMSR);

With all of this I think it wouldn't make the patch worse to look at if
you also renamed v -> curr.

> +if ( inst_len == 0 )
> +return;
>  
> -if ( vmcb->exitinfo1 == 0 )
> +if ( rdmsr )
>  {
> -if ( (inst_len = __get_instruction_length(v, INSTR_RDMSR)) == 0 )
> -return;
> -rc = hvm_msr_read_intercept(regs->ecx, _content);
> -regs->eax = (uint32_t)msr_content;
> -regs->edx = (uint32_t)(msr_content >> 32);
> +uint64_t msr_content = 0;
> +
> +rc = hvm_msr_read_intercept(regs->_ecx, _content);
> +if ( rc == X86EMUL_OKAY )
> +{
> +regs->rax = (uint32_t)msr_content;
> +regs->rdx = (uint32_t)(msr_content >> 32);

While the first of the casts is needed, the second one isn't.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH for-4.8] x86/svm: Don't clobber eax and edx if an RDMSR intercept fails

[Andrew Cooper]

The original code has a bug; eax and edx get unconditionally updated even when
hvm_msr_read_intercept() doesn't return X86EMUL_OKAY.

It is only by blind luck (vmce_rdmsr() eagerly initialising its msr_content
pointer) that this isn't an information leak into guests.

While fixing this bug, reduce the scope of msr_content and initialise it to 0.
This makes it obvious that a stack leak won't occur, even if there were to be
a buggy codepath in hvm_msr_read_intercept().

Also make some non-functional improvements.  Make the insn_len calculation
common, and reduce the quantity of explicit casting by making better use of
the existing register names.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Boris Ostrovsky 
CC: Suravee Suthikulpanit 
CC: Wei Liu 
---
 xen/arch/x86/hvm/svm/svm.c | 32 +---
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 16427f6..6530e22 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1948,26 +1948,28 @@ static int svm_msr_write_intercept(unsigned int msr, 
uint64_t msr_content)
 
 static void svm_do_msr_access(struct cpu_user_regs *regs)
 {
-int rc, inst_len;
 struct vcpu *v = current;
-struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-uint64_t msr_content;
+bool rdmsr = v->arch.hvm_svm.vmcb->exitinfo1 == 0;
+int rc, inst_len = __get_instruction_length(
+v, rdmsr ? INSTR_RDMSR : INSTR_WRMSR);
+
+if ( inst_len == 0 )
+return;
 
-if ( vmcb->exitinfo1 == 0 )
+if ( rdmsr )
 {
-if ( (inst_len = __get_instruction_length(v, INSTR_RDMSR)) == 0 )
-return;
-rc = hvm_msr_read_intercept(regs->ecx, _content);
-regs->eax = (uint32_t)msr_content;
-regs->edx = (uint32_t)(msr_content >> 32);
+uint64_t msr_content = 0;
+
+rc = hvm_msr_read_intercept(regs->_ecx, _content);
+if ( rc == X86EMUL_OKAY )
+{
+regs->rax = (uint32_t)msr_content;
+regs->rdx = (uint32_t)(msr_content >> 32);
+}
 }
 else
-{
-if ( (inst_len = __get_instruction_length(v, INSTR_WRMSR)) == 0 )
-return;
-msr_content = ((uint64_t)regs->edx << 32) | (uint32_t)regs->eax;
-rc = hvm_msr_write_intercept(regs->ecx, msr_content, 1);
-}
+rc = hvm_msr_write_intercept(regs->_ecx,
+ (regs->rdx << 32) | regs->_eax, 1);
 
 if ( rc == X86EMUL_OKAY )
 __update_guest_eip(regs, inst_len);
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel