Re: [Xen-devel] [PATCH 1/3] x86/HVM: restrict emulation in hvm_descriptor_access_intercept()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/04/17 15:51, Jan Beulich wrote:
> While I did review d0a699a389 ("x86/monitor: add support for descriptor
> access events") it didn't really occur to me that somone could be this
> blunt and add unguarded emulation again just a few weeks after we
> guarded all special purpose emulator invocations. Fix this.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Oops - I also omitted that point from my review checklist.  I will
endeavour not to make the same mistake again.

>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3598,6 +3598,28 @@ gp_fault:
>      return X86EMUL_EXCEPTION;
>  }
>  
> +static bool is_sysdesc_access(const struct x86_emulate_state *state,
> +                              const struct x86_emulate_ctxt *ctxt)
> +{
> +    unsigned int ext;
> +    int mode = x86_insn_modrm(state, NULL, &ext);

Unfortunately, this is another example which Coverity will pick up on,
along with the use of x86_insn_modrm() in is_invlpg().

In the case that we return -EINVAL, ext is uninitialised when it gets
used below.

Other than that, the change looks good.

~Andrew

> +
> +    switch ( ctxt->opcode )
> +    {
> +    case X86EMUL_OPC(0x0f, 0x00):
> +        if ( !(ext & 4) ) /* SLDT / STR / LLDT / LTR */
> +            return true;
> +        break;
> +
> +    case X86EMUL_OPC(0x0f, 0x01):
> +        if ( mode != 3 && !(ext & 4) ) /* SGDT / SIDT / LGDT / LIDT */
> +            return true;
> +        break;
> +    }
> +
> +    return false;
> +}
> +
>  int hvm_descriptor_access_intercept(uint64_t exit_info,
>                                      uint64_t vmx_exit_qualification,
>                                      unsigned int descriptor, bool is_write)


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