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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

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).

> 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?

Jan