[v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

[PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

Posted by Bin Meng 3 years, 5 months ago

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);
         }
     }
 }
-- 
2.7.4

Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

Posted by Paolo Bonzini 3 years, 5 months ago

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

Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

Posted by Bin Meng 3 years, 5 months ago

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

Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

Posted by Paolo Bonzini 3 years, 5 months ago

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

Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

Posted by Bin Meng 3 years, 5 months ago

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

Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

Posted by Paolo Bonzini 3 years, 5 months ago

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

Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

Posted by Bin Meng 3 years, 4 months ago

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

Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

Posted by Philippe Mathieu-Daudé 3 years, 5 months ago

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

Re: [PATCH v2] target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper

Posted by Bin Meng 3 years, 4 months ago

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