Re: [PATCH] x86emul: don't call ->read_segment() with x86_seg_none

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/08/2024 3:05 pm, Jan Beulich wrote:
> On 12.08.2024 15:04, Andrew Cooper wrote:
>> On 05/08/2024 2:26 pm, Jan Beulich wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -839,7 +839,8 @@ protmode_load_seg(
>>>          case x86_seg_tr:
>>>              goto raise_exn;
>>>          }
>>> -        if ( !_amd_like(cp) || vcpu_has_nscb() || !ops->read_segment ||
>>> +        if ( seg == x86_seg_none || !_amd_like(cp) || vcpu_has_nscb() ||
>>> +             !ops->read_segment ||
>>>               ops->read_segment(seg, sreg, ctxt) != X86EMUL_OKAY )
>>>              memset(sreg, 0, sizeof(*sreg));
>>>          else
>> While this fixes the crash, I'm not sure it will behave correctly for
>> VERR/VERW.
>>
>> protmode_load_seg() is unconditionally X86EMUL_OKAY for a NULL selector,
>> and VERW checks for type != 0x8 which an empty attr will pass.
> That's past an sreg.s check, which will have failed (for sreg coming back
> all clear).

Oh, so it is.

Any chance I could talk you into folding this hunk in too?

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
b/xen/arch/x86/x86_emulate/x86_emulate.c
index 902538267051..d4d02c3e2eb3 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2852,7 +2852,7 @@ x86_emulate(
                                             &sreg, ctxt, ops) )
             {
             case X86EMUL_OKAY:
-                if ( sreg.s &&
+                if ( sreg.s /* Excludes NULL selector too */ &&
                      ((modrm_reg & 1) ? ((sreg.type & 0xa) == 0x2)
                                       : ((sreg.type & 0xa) != 0x8)) )
                     _regs.eflags |= X86_EFLAGS_ZF;


because it is relevant to judging whether the subsequent logic is
correct or not.

>
> Also if there was a concern here, it would be orthogonal to the adding of
> the seg_none check: It would then have been similarly an issue for all
> possibilities of taking the memset() path.
>
>> Interestingly, both Intel and AMD state that the NULL selector is
>> neither readable nor writeable.
> Which makes sense, doesn't it? A nul selector is really more like a
> system segment in this regard (for VERR/VERW).

In the 32bit days, yes, the NULL selector was entirely unusable, but
that changed in 64bit.

So IMO it really depends on whether VERR/VERW means "can I use this
selector for $X", or "what does the GDT/LDT say about $X".

AMD say "Verifies whether a code or data segment specified by the
segment selector in the 16-bit register or memory operand is readable
from the current privilege level."

Intel say "Verifies whether the code or data segment specified with the
source operand is readable (VERR) or writable[sic] (VERW) from the
current privilege level (CPL)."

So that's clearly the former meaning rather than the latter meaning.

Nevertheless, AMD clearly decided it wasn't worth changing the behaviour
of the instruction in a 64bit mode, probably for the same reason that
Intel reused VERW for scrubbing; that no-one had really bought into
x86's segmented memory model since the 386 replaced the 286.

I just found it odd, that was all.

>
>> Also, looking at the emulator logic, we're missing the DPL vs
>> CPL/RPL/Conforming checks.
> There's surely nothing "conforming" for a nul selector. Hence perhaps you
> refer to something entirely unrelated?

Sorry, yes.  I think this is a general bug in how we emulate VERW/VERR,
unrelated to this specific OSS-fuzz report.

~Andrew