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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.04.17 at 18:00, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Come on - such a fundamental bug would have worse
consequences than uninitialized variables here. I agree this being
a tool limitation is a matter of perspective, as the tool can't possibly
know what we know. But the tool also doesn't offer a way to give
it the missing piece of information.

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

I don't see how this would help - the variables we're talking about
here would still be uninitialized. Instead, how about this as a prereq
patch to deal with the problem once an for all?

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -8017,8 +8017,14 @@ x86_insn_modrm(const struct x86_emulate_
 {
     check_state(state);
 
-    if ( state->modrm_mod > 3 )
+    if ( unlikely(state->modrm_mod > 3) )
+    {
+        if ( rm )
+            *rm = ~0U;
+        if ( reg )
+            *reg = ~0U;
         return -EINVAL;
+    }
 
     if ( rm )
         *rm = state->modrm_rm;

Jan


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