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
