diff mbox series

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

Message ID 1603361762-43062-1-git-send-email-bmeng.cn@gmail.com
State Superseded
Headers show
Series target/i386: seg_helper: Correct segement selector nullification in the RET/IRET helper | expand

Commit Message

Bin Meng Oct. 22, 2020, 10:16 a.m. UTC 
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>
---

 target/i386/seg_helper.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Bin Meng Nov. 2, 2020, 8:20 a.m. UTC | #1 
On Thu, Oct 22, 2020 at 6:16 PM Bin Meng <bmeng.cn@gmail.com> 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>
> ---
>
>  target/i386/seg_helper.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>

Ping?
Bin Meng Nov. 12, 2020, 6:33 a.m. UTC | #2 
On Mon, Nov 2, 2020 at 4:20 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Oct 22, 2020 at 6:16 PM Bin Meng <bmeng.cn@gmail.com> 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>
> > ---
> >
> >  target/i386/seg_helper.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
>
> Ping?

Ping?
Paolo Bonzini Nov. 12, 2020, 11:48 a.m. UTC | #3 
On 22/10/20 12:16, 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.

This is all quite messy in QEMU; for performance reasons, it never even 
checks the flags on memory accesses, only on selector loads.  One way to 
fix it would be to define five extra hflags bits that copy the P bit of 
DS/ES/SS/FS/GS.  gen_lea_v_seg checks if the hflag is clear, and if so 
it generates a #GP or #SS.

Regarding your patch, I think at least the segment should be made 
"unusable".  On Intel processors there is an internal "unusable" flag, 
on AMD and in QEMU, equivalently, the P bit would be cleared in the 
flags.  As far as I know the difference is only visible with VMX.

That is, you'd need something like this:

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

Thanks,

Paolo

> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   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..d8766d8 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);
>           }
>       }
>   }
>
diff mbox series

Patch

diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index be88938..d8766d8 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);
         }
     }
 }