| Message ID | 81ae365f-98b4-4bb6-bbb6-c5423dfda038@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86emul: don't call ->read_segment() with x86_seg_none | expand |
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@suse.com> Looking at oss-fuzz's report and at this patch I think it is correct Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> 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 >
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@suse.com> > > Looking at oss-fuzz's report and at this patch I think it is correct > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> S-o-b? > 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. Jan
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@suse.com> > > > > Looking at oss-fuzz's report and at this patch I think it is correct > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > 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.
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@suse.com> > --- > 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
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
On 12/08/2024 3:05 pm, Jan Beulich wrote: > 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). Oh, so it is. Any chance I could talk you into folding this hunk in too? diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index 902538267051..d4d02c3e2eb3 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2852,7 +2852,7 @@ x86_emulate( &sreg, ctxt, ops) ) { case X86EMUL_OKAY: - if ( sreg.s && + if ( sreg.s /* Excludes NULL selector too */ && ((modrm_reg & 1) ? ((sreg.type & 0xa) == 0x2) : ((sreg.type & 0xa) != 0x8)) ) _regs.eflags |= X86_EFLAGS_ZF; because it is relevant to judging whether the subsequent logic is correct or not. > > 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). In the 32bit days, yes, the NULL selector was entirely unusable, but that changed in 64bit. So IMO it really depends on whether VERR/VERW means "can I use this selector for $X", or "what does the GDT/LDT say about $X". AMD say "Verifies whether a code or data segment specified by the segment selector in the 16-bit register or memory operand is readable from the current privilege level." Intel say "Verifies whether the code or data segment specified with the source operand is readable (VERR) or writable[sic] (VERW) from the current privilege level (CPL)." So that's clearly the former meaning rather than the latter meaning. Nevertheless, AMD clearly decided it wasn't worth changing the behaviour of the instruction in a 64bit mode, probably for the same reason that Intel reused VERW for scrubbing; that no-one had really bought into x86's segmented memory model since the 386 replaced the 286. I just found it odd, that was all. > >> 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? Sorry, yes. I think this is a general bug in how we emulate VERW/VERR, unrelated to this specific OSS-fuzz report. ~Andrew
On 14.08.2024 14:49, Andrew Cooper wrote: > On 12/08/2024 3:05 pm, Jan Beulich wrote: >> 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). > > Oh, so it is. > > Any chance I could talk you into folding this hunk in too? > > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c > b/xen/arch/x86/x86_emulate/x86_emulate.c > index 902538267051..d4d02c3e2eb3 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -2852,7 +2852,7 @@ x86_emulate( > &sreg, ctxt, ops) ) > { > case X86EMUL_OKAY: > - if ( sreg.s && > + if ( sreg.s /* Excludes NULL selector too */ && > ((modrm_reg & 1) ? ((sreg.type & 0xa) == 0x2) > : ((sreg.type & 0xa) != 0x8)) ) > _regs.eflags |= X86_EFLAGS_ZF; > > > because it is relevant to judging whether the subsequent logic is > correct or not. No problem at all. Am I okay to commit then, with Stefano's R-b? >> 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). > > In the 32bit days, yes, the NULL selector was entirely unusable, but > that changed in 64bit. > > So IMO it really depends on whether VERR/VERW means "can I use this > selector for $X", or "what does the GDT/LDT say about $X". > > AMD say "Verifies whether a code or data segment specified by the > segment selector in the 16-bit register or memory operand is readable > from the current privilege level." > > Intel say "Verifies whether the code or data segment specified with the > source operand is readable (VERR) or writable[sic] (VERW) from the > current privilege level (CPL)." > > So that's clearly the former meaning rather than the latter meaning. Not really. There's no "code or data segment" specified with a nul selector. The use of GDT/LDT is implicit in this wording, but imo it is there. >>> 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? > > Sorry, yes. I think this is a general bug in how we emulate VERW/VERR, > unrelated to this specific OSS-fuzz report. I'll check that later and reply separately (here or with a patch). Jan
On 14/08/2024 2:10 pm, Jan Beulich wrote: > On 14.08.2024 14:49, Andrew Cooper wrote: >> On 12/08/2024 3:05 pm, Jan Beulich wrote: >>> 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). >> Oh, so it is. >> >> Any chance I could talk you into folding this hunk in too? >> >> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c >> b/xen/arch/x86/x86_emulate/x86_emulate.c >> index 902538267051..d4d02c3e2eb3 100644 >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -2852,7 +2852,7 @@ x86_emulate( >> &sreg, ctxt, ops) ) >> { >> case X86EMUL_OKAY: >> - if ( sreg.s && >> + if ( sreg.s /* Excludes NULL selector too */ && >> ((modrm_reg & 1) ? ((sreg.type & 0xa) == 0x2) >> : ((sreg.type & 0xa) != 0x8)) ) >> _regs.eflags |= X86_EFLAGS_ZF; >> >> >> because it is relevant to judging whether the subsequent logic is >> correct or not. > No problem at all. Am I okay to commit then, with Stefano's R-b? Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> and don't forget the conversion to Link: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=70918 ~Andrew
On 14.08.2024 14:49, Andrew Cooper wrote: > On 12/08/2024 3:05 pm, Jan Beulich wrote: >> On 12.08.2024 15:04, Andrew Cooper wrote: >>> 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? > > Sorry, yes. I think this is a general bug in how we emulate VERW/VERR, > unrelated to this specific OSS-fuzz report. In protmode_load_seg() we have case x86_seg_none: /* Non-conforming segment: check DPL against RPL and CPL. */ if ( ((desc.b & (0x1c << 8)) != (0x1c << 8)) && ((dpl < cpl) || (dpl < rpl)) ) return X86EMUL_EXCEPTION; a_flag = 0; break; Is there anything else you think is needed? Jan
--- 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
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@suse.com> --- 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.