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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +    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.

Jan


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