Re: [Qemu-devel] [BUG] user-to-root privesc inside VM via bad translation caching

[Paolo Bonzini]

On 23/03/2017 17:50, Pranith Kumar wrote:
> On Thu, Mar 23, 2017 at 6:27 AM, Paolo Bonzini <address@hidden> wrote:
>>
>>
>> On 22/03/2017 21:01, Richard Henderson wrote:
>>>>
>>>> Ah, OK. Thanks for the explanation. May be we should check the size of
>>>> the instruction while decoding the prefixes and error out once we
>>>> exceed the limit. We would not generate any IR code.
>>>
>>> Yes.
>>>
>>> It would not enforce a true limit of 15 bytes, since you can't know that
>>> until you've done the rest of the decode.  But you'd be able to say that
>>> no more than 14 prefix + 1 opc + 6 modrm+sib+ofs + 4 immediate = 25
>>> bytes is used.
>>>
>>> Which does fix the bug.
>>
>> Yeah, that would work for 2.9 if somebody wants to put together a patch.
>>  Ensuring that all instruction fetching happens before translation side
>> effects is a little harder, but perhaps it's also the opportunity to get
>> rid of s->rip_offset which is a little ugly.
> 
> How about the following?
> 
> diff --git a/target/i386/translate.c b/target/i386/translate.c
> index 72c1b03a2a..67c58b8900 100644
> --- a/target/i386/translate.c
> +++ b/target/i386/translate.c
> @@ -4418,6 +4418,11 @@ static target_ulong disas_insn(CPUX86State
> *env, DisasContext *s,
>      s->vex_l = 0;
>      s->vex_v = 0;
>   next_byte:
> +    /* The prefixes can atmost be 14 bytes since x86 has an upper
> +       limit of 15 bytes for the instruction */
> +    if (s->pc - pc_start > 14) {
> +        goto illegal_op;
> +    }
>      b = cpu_ldub_code(env, s->pc);
>      s->pc++;
>      /* Collect prefixes.  */

Please make the comment more verbose, based on Richard's remark.  We
should apply it to 2.9.

Also, QEMU usually formats comments with stars on every line.

Paolo