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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 5 Aug 2024, 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>

Looking at oss-fuzz's report and at this patch I think it is correct

Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>


That said, there are a few other places where read_segment is called
without any checks for seg being x86_seg_none. The hvm implementation of
read_segment (hvmemul_read_segment) seems to return error if
x86_seg_none is passed as an argument, but there is no return error
checks any time we call ops->read_segment in x86_emulate.c.

It seems that there might still be an issue?


> ---
> It is pure guesswork that one of those insns did trigger the assertion.
> The report from oss-fuzz sadly lacks details on the insn under
> emulation. I'm further surprised that AFL never found this.
> 
> --- 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
>