Message ID | 1605261378-77971-1-git-send-email-bmeng.cn@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper | expand |
On 13/11/20 10:56, Bin Meng wrote: > From: Bin Meng <bin.meng@windriver.com> > > Per the SDM, when returning to outer privilege level, for segment > registers (ES, FS, GS, and DS) if the check fails, the segment > selector becomes null, but QEMU clears the base/limit/flags as well > as nullifying the segment selector, which should be a spec violation. > > Real hardware seems to be compliant with the spec, at least on one > Coffee Lake board I tested. > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > --- > > Changes in v2: > - clearing the DESC_P bit in the segment descriptor > > target/i386/seg_helper.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c > index be88938..d539573 100644 > --- a/target/i386/seg_helper.c > +++ b/target/i386/seg_helper.c > @@ -2108,7 +2108,10 @@ static inline void validate_seg(CPUX86State *env, int seg_reg, int cpl) > if (!(e2 & DESC_CS_MASK) || !(e2 & DESC_C_MASK)) { > /* data or non conforming code segment */ > if (dpl < cpl) { > - cpu_x86_load_seg_cache(env, seg_reg, 0, 0, 0, 0); > + cpu_x86_load_seg_cache(env, seg_reg, 0, > + env->segs[seg_reg].base, > + env->segs[seg_reg].limit, > + env->segs[seg_reg].flags & ~DESC_P_MASK); > } > } > } > Queued, thanks. It would be nicer if the commit message explained how the guest can notice the difference. Paolo
Hi Paolo, On Fri, Nov 13, 2020 at 6:18 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 13/11/20 10:56, Bin Meng wrote: > > From: Bin Meng <bin.meng@windriver.com> > > > > Per the SDM, when returning to outer privilege level, for segment > > registers (ES, FS, GS, and DS) if the check fails, the segment > > selector becomes null, but QEMU clears the base/limit/flags as well > > as nullifying the segment selector, which should be a spec violation. > > > > Real hardware seems to be compliant with the spec, at least on one > > Coffee Lake board I tested. > > > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > > > --- > > > > Changes in v2: > > - clearing the DESC_P bit in the segment descriptor > > > > target/i386/seg_helper.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c > > index be88938..d539573 100644 > > --- a/target/i386/seg_helper.c > > +++ b/target/i386/seg_helper.c > > @@ -2108,7 +2108,10 @@ static inline void validate_seg(CPUX86State *env, int seg_reg, int cpl) > > if (!(e2 & DESC_CS_MASK) || !(e2 & DESC_C_MASK)) { > > /* data or non conforming code segment */ > > if (dpl < cpl) { > > - cpu_x86_load_seg_cache(env, seg_reg, 0, 0, 0, 0); > > + cpu_x86_load_seg_cache(env, seg_reg, 0, > > + env->segs[seg_reg].base, > > + env->segs[seg_reg].limit, > > + env->segs[seg_reg].flags & ~DESC_P_MASK); > > } > > } > > } > > > > Queued, thanks. Thanks! > It would be nicer if the commit message explained how > the guest can notice the difference. The commit message says "Per the SDM" :) The actual failure case involves a special code sequence that is exposed in VxWorks guest testing. Linux does not expose this however. Regards, Bin
On 13/11/20 11:23, Bin Meng wrote: >> It would be nicer if the commit message explained how >> the guest can notice the difference. > > The commit message says "Per the SDM" :) The actual failure case > involves a special code sequence that is exposed in VxWorks guest > testing. Linux does not expose this however. I see. Is there any chance you could write a testcase for kvm-unit-tests? Or just explain how to write such a test, and then I can write it myself; it's not clear to me how the guest can observe the base and limit of a non-present segment. Paolo
On 11/13/20 11:18 AM, Paolo Bonzini wrote: > On 13/11/20 10:56, Bin Meng wrote: ... > > Queued, thanks. It would be nicer if the commit message explained how > the guest can notice the difference. Typo "segement" -> "segment" in subject. > > Paolo > >
Hi Paolo, On Fri, Nov 13, 2020 at 6:39 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 13/11/20 11:23, Bin Meng wrote: > >> It would be nicer if the commit message explained how > >> the guest can notice the difference. > > > > The commit message says "Per the SDM" :) The actual failure case > > involves a special code sequence that is exposed in VxWorks guest > > testing. Linux does not expose this however. > > I see. Is there any chance you could write a testcase for > kvm-unit-tests? Or just explain how to write such a test, and then I > can write it myself; it's not clear to me how the guest can observe the > base and limit of a non-present segment. I am not familiar with kvm-unit-test. The original issue cannot be reproduced with a KVM enabled QEMU as the codes-in-flaw is in the emulation path. Regards, Bin
On 17/11/20 11:08, Bin Meng wrote: >> I see. Is there any chance you could write a testcase for >> kvm-unit-tests? Or just explain how to write such a test, and then I >> can write it myself; it's not clear to me how the guest can observe the >> base and limit of a non-present segment. > > I am not familiar with kvm-unit-test. The original issue cannot be > reproduced with a KVM enabled QEMU as the codes-in-flaw is in the > emulation path. kvm-unit-tests, despite the name, is a set generic tests for CPU behavior; it works with other accelerators that QEMU supports including the emulation path. You can find it at https://gitlab.com/kvm-unit-tests/kvm-unit-tests. If you explain in enough detail how VxWorks triggers the bug, I can take care of writing the test. Paolo
Hi Paolo, On Tue, Nov 17, 2020 at 7:06 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 17/11/20 11:08, Bin Meng wrote: > >> I see. Is there any chance you could write a testcase for > >> kvm-unit-tests? Or just explain how to write such a test, and then I > >> can write it myself; it's not clear to me how the guest can observe the > >> base and limit of a non-present segment. > > > > I am not familiar with kvm-unit-test. The original issue cannot be > > reproduced with a KVM enabled QEMU as the codes-in-flaw is in the > > emulation path. > > kvm-unit-tests, despite the name, is a set generic tests for CPU > behavior; it works with other accelerators that QEMU supports including > the emulation path. You can find it at > https://gitlab.com/kvm-unit-tests/kvm-unit-tests. I see. Thanks for the info. > If you explain in enough detail how VxWorks triggers the bug, I can take > care of writing the test. I will try to create a test case using the kvm-unit-tests framework. Regards, Bin
On Mon, Nov 16, 2020 at 8:48 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 11/13/20 11:18 AM, Paolo Bonzini wrote: > > On 13/11/20 10:56, Bin Meng wrote: > ... > > > > Queued, thanks. It would be nicer if the commit message explained how > > the guest can notice the difference. > > Typo "segement" -> "segment" in subject. Thanks Philippe. Paolo, please let me know if you need me to respin this patch, or you can fix this when applying. Regards, Bin
diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c index be88938..d539573 100644 --- a/target/i386/seg_helper.c +++ b/target/i386/seg_helper.c @@ -2108,7 +2108,10 @@ static inline void validate_seg(CPUX86State *env, int seg_reg, int cpl) if (!(e2 & DESC_CS_MASK) || !(e2 & DESC_C_MASK)) { /* data or non conforming code segment */ if (dpl < cpl) { - cpu_x86_load_seg_cache(env, seg_reg, 0, 0, 0, 0); + cpu_x86_load_seg_cache(env, seg_reg, 0, + env->segs[seg_reg].base, + env->segs[seg_reg].limit, + env->segs[seg_reg].flags & ~DESC_P_MASK); } } }