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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/08/2024 2:26 pm, Jan Beulich wrote:
> LAR, LSL, VERR, and VERW emulation involve calling protmode_load_seg()
> with x86_seg_none. The fuzzer's read_segment() hook function has an
> assertion which triggers in this case. Calling the hook function,
> however, makes little sense for those insns, as there's no data to
> retrieve. Instead zero-filling the output structure is what properly
> corresponds to those insns being invoked with a NUL selector.
>
> Fixes: 06a3b8cd7ad2 ("x86emul: support LAR/LSL/VERR/VERW")
> Oss-fuzz: 70918
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> It is pure guesswork that one of those insns did trigger the assertion.

Tamas handed me the repro seeing as I don't have access to the bugs.  It
was VERR/VERW.

> The report from oss-fuzz sadly lacks details on the insn under
> emulation.

I've got a still-pending patch to add `--debug` to the harness to dump
that information.

> I'm further surprised that AFL never found this.

I haven't done any AFL testing since 06a3b8cd7ad2 was added.

This crash is specific to VERW/etc with a NULL selector, which will be a
rare combination to encounter.

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

Interestingly, both Intel and AMD state that the NULL selector is
neither readable nor writeable.

Also, looking at the emulator logic, we're missing the DPL vs
CPL/RPL/Conforming checks.

~Andrew