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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 7 Aug 2024, Jan Beulich wrote:
> On 06.08.2024 20:24, Stefano Stabellini wrote:
> > 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>
> 
> S-o-b?

Sorry I meant reviewed-by


> > 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.
> 
> There is a pretty limited number of cases where x86_seg_none is used.
> For example, state->ea.mem.seg cannot hold this value.
> realmode_load_seg() also cannot be passed this value. We could add
> assertions to that effect, yet that can get unwieldy as further
> x86_seg_* enumerators are added (see "x86: introduce x86_seg_sys"; I
> expect we'll need at least one more when adding VMX/SVM insn emulation,
> where physical addresses are used as insn operands).
> 
> > It seems that there might still be an issue?
> 
> In my auditing I didn't spot any.

Following your explanation I come to the same conclusion as you, so I
think it is OK. But knowing that state->ea.mem.seg cannot hold this
value and realmode_load_seg() also cannot be passed this value is not
something for the casual observer, so in my opinion there is room here
for making the code more resilient and more obvious by always checking
the return value of read_segment.