linux-kernel - Re: [PATCH RFC v1 16/20] KVM: x86: Decode REX2 prefix in the emulator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <25c8c533-73a3-4cc1-9fbf-4301b2155f11@intel.com>
Date: Thu, 13 Nov 2025 15:30:37 -0800
From: "Chang S. Bae" <chang.seok.bae@...el.com>
To: Paolo Bonzini <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
CC: <seanjc@...gle.com>, <chao.gao@...el.com>, <zhao1.liu@...el.com>
Subject: Re: [PATCH RFC v1 16/20] KVM: x86: Decode REX2 prefix in the emulator

On 11/11/2025 9:55 AM, Paolo Bonzini wrote:
> On 11/10/25 19:01, Chang S. Bae wrote:
>>
>>           case 0x40 ... 0x4f: /* REX */
>>               if (mode != X86EMUL_MODE_PROT64)
>>                   goto done_prefixes;
>> +            if (ctxt->rex_prefix == REX2_PREFIX)
>> +                break;
>>               ctxt->rex_prefix = REX_PREFIX;
>>               ctxt->rex.raw    = 0x0f & ctxt->b;
>>               continue;
>> +        case 0xd5: /* REX2 */
>> +            if (mode != X86EMUL_MODE_PROT64)
>> +                goto done_prefixes;
> Here you should also check
> 
>      if (ctxt->rex_prefix == REX_PREFIX) {
>          ctxt->rex_prefix = REX2_INVALID;
>          goto done_prefixes;
>      }

You're right. Section 3.1.2.1 states:
| A REX prefix (0x4*) immediately preceding the REX2 prefix is not
| allowed and triggers #UD.

Now I think REX2_INVALID would just add another condition to handle
later. Instead, for such invalid case, it might be simpler to mark the
opcode as undefined and jump all the way after the lookup. See the diff
-- please let me know if you dislike it.

>> +            if (ctxt->rex_prefix == REX2_PREFIX &&
>> +                ctxt->rex.bits.m0 == 0)
>> +                break;
>> +            ctxt->rex_prefix = REX2_PREFIX;
>> +            ctxt->rex.raw    = insn_fetch(u8, ctxt);
>> +            continue;
> After REX2 always comes the main opcode byte, so you can "goto 
> done_prefixes" here.  Or even jump here already; in pseudocode:
> 
>      ctxt->b = insn_fetch(u8, ctxt);
>      if (rex2 & REX_M)
>          goto decode_twobyte;
>      else
>          goto decode_onebyte;

Yes, agreed. I think this makes the control flow more explicit.

>> +        if (ctxt->rex_prefix == REX2_PREFIX) {
>> +            /*
>> +             * A legacy or REX prefix following a REX2 prefix
>> +             * forms an invalid byte sequences. Likewise,
>> +             * a second REX2 prefix following a REX2 prefix
>> +             * with M0=0 is invalid.
>> +             */
>> +            ctxt->rex_prefix = REX2_INVALID;
>> +            goto done_prefixes;
>> +        }
> 
> ... and this is not needed.

I really like that this can go away.
View attachment "PATCH16.diff" of type "text/plain" (2699 bytes)