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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/04/17 16:28, Jan Beulich wrote:
>>>> On 13.04.17 at 16:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 13/04/17 15:51, Jan Beulich wrote:
>>> --- 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.
> Sigh. Yes, we can work around this tool limitation

It is not a tools limitation.  It is an entirely legitimate complaint
about the state of the current code.

ext being not undefined depends on all 8k lines of emulator code not
being buggy and accidentally clearing d & ModRM (or is it Vsib for that
matter), or accidentally clobbering ->modrm_mod.

> , but honestly I
> don't really agree doing so in cases like this (where the code is
> clearly correct, as -EINVAL can't come back). Plus we have
> precedent to the above other than just in is_invlpg():
> priv_op_validate() does the same, as does emulate_gate_op().
> If there was a way to work around the warnings without growing
> generated code, I'd be less opposed (but still not entirely in
> agreement), but all I can see is either making the return value
> check more involved or adding initializers. In neither case the
> compiler will be able to eliminate the resulting insns.

How about returning

enum {
    modrm_reg,
    modrm_mem,
    modrm_none
};

The users of x86_insn_modrm() don't care about values between 0 and 2. 
They are only looking for "does it have a memory reference", or "does it
have extra opcode encoded in the reg field".

~Andrew

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