Re: [Xen-devel] [PATCH] x86/svm: Adjust ModRM Mode check in is_invlpg()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.01.17 at 21:29, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 01/11/2017 12:33 PM, Andrew Cooper wrote:
>> Coverity points out that x86_insn_modrm() returns -EINVAL for instructions 
>> not
>> encoded with a ModRM byte.  A consequence is that checking != 3 is
>> insufficient to confirm that &ext was actually written to.
>>
>> In practice, this check is only used after decode has been successful, and
>> 0f01 will have a ModRM byte.
>>
>> Use an unsigned < comparison to exclude the -EINVAL case, guaranteeing that
>> ext is only read if it was filled in by x86_insn_modrm(), which should 
>> placate
>> Coverity.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>>
>> RFC.  I haven't actually checked that this fixes the issue.
>>
>> An alternative would be to ASSERT() that x86_insn_modrm() is non-negative, 
>> but
>> I can't nice way of integrating that into the existing logic (without using
>> the comma operator, and that isn't nice to read).
> 
> how about
> 
>       return ctxt->opcode == X86EMUL_OPC(0x0f, 0x01) &&
> -           x86_insn_modrm(state, NULL, &ext) != 3 &&
> +           (mod = x86_insn_modrm(state, NULL, &ext)) >= 0 && mod != 3 &&
>             (ext & 7) == 7;
> 
> 
> (in case x86_insn_modrm decides to return some other errors in the future.)

I had specifically avoided introduction of a "mod" local variable.

Jan


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