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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/11/16 14:14, Jan Beulich wrote:
>>>> On 09.11.16 at 13:28, <andrew.cooper3@xxxxxxxxxx> 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 <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 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, &msr_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, &msr_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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel