[Qemu-devel] [PATCH] target-i386: fix segment limit check in ljmp

[Qemu-devel] [PATCH] target-i386: fix segment limit check in ljmp

[andrew]

From: Andrew Oates <aoates@google.com>

The current implementation has three bugs,
 * segment limits are not enforced in protected mode if the L bit is set
   in the target segment descriptor[1]
 * segment limits are not enforced in compatability mode (ljmp to 32-bit
   code segment in long mode)
 * #GP(new_cs) is generated rather than #GP(0)

Now the segment limits are enforced if we're not in long mode OR the
target code segment doesn't have the L bit set.

[1] this is an invalid configuration (in protected mode the L bit is
reserved and should be set to zero), but qemu doesn't enforce that.

Signed-off-by: Andrew Oates <aoates@google.com>
---
The limit check is still incorrect for ljmp-through-call-gate in 64-bit
mode.  That's a larger fix I'm still working on.

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

diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index 00301a0c04..975365fd30 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -1628,8 +1628,8 @@ void helper_ljmp_protected(CPUX86State *env, int new_cs, target_ulong new_eip,
         }
         limit = get_seg_limit(e1, e2);
         if (new_eip > limit &&
-            !(env->hflags & HF_LMA_MASK) && !(e2 & DESC_L_MASK)) {
-            raise_exception_err_ra(env, EXCP0D_GPF, new_cs & 0xfffc, GETPC());
+            (!(env->hflags & HF_LMA_MASK) || !(e2 & DESC_L_MASK))) {
+            raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
         }
         cpu_x86_load_seg_cache(env, R_CS, (new_cs & 0xfffc) | cpl,
                        get_seg_base(e1, e2), limit, e2);
-- 
2.17.0

Re: [Qemu-devel] [PATCH] target-i386: fix segment limit check in ljmp

[Paolo Bonzini]

On 16/08/2018 03:19, andrew@andrewoates.com wrote:
> From: Andrew Oates <aoates@google.com>
> 
> The current implementation has three bugs,
>  * segment limits are not enforced in protected mode if the L bit is set
>    in the target segment descriptor[1]
>  * segment limits are not enforced in compatability mode (ljmp to 32-bit
>    code segment in long mode)
>  * #GP(new_cs) is generated rather than #GP(0)
> 
> Now the segment limits are enforced if we're not in long mode OR the
> target code segment doesn't have the L bit set.
> 
> [1] this is an invalid configuration (in protected mode the L bit is
> reserved and should be set to zero), but qemu doesn't enforce that.

Stupid question, why not fix that instead at least for this case?

> Signed-off-by: Andrew Oates <aoates@google.com>
> ---
> The limit check is still incorrect for ljmp-through-call-gate in 64-bit
> mode.  That's a larger fix I'm still working on.

Can you resend the call-gate for both ljmp and lcall when you're done
(there's plenty of time until 3.1)?

Thanks,

Paolo

>  target/i386/seg_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
> index 00301a0c04..975365fd30 100644
> --- a/target/i386/seg_helper.c
> +++ b/target/i386/seg_helper.c
> @@ -1628,8 +1628,8 @@ void helper_ljmp_protected(CPUX86State *env, int new_cs, target_ulong new_eip,
>          }
>          limit = get_seg_limit(e1, e2);
>          if (new_eip > limit &&
> -            !(env->hflags & HF_LMA_MASK) && !(e2 & DESC_L_MASK)) {
> -            raise_exception_err_ra(env, EXCP0D_GPF, new_cs & 0xfffc, GETPC());
> +            (!(env->hflags & HF_LMA_MASK) || !(e2 & DESC_L_MASK))) {
> +            raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
>          }
>          cpu_x86_load_seg_cache(env, R_CS, (new_cs & 0xfffc) | cpl,
>                         get_seg_base(e1, e2), limit, e2);
> 

Re: [Qemu-devel] [PATCH] target-i386: fix segment limit check in ljmp

[Andrew Oates]

On Fri, Aug 17, 2018 at 1:38 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 16/08/2018 03:19, andrew@andrewoates.com wrote:
> > From: Andrew Oates <aoates@google.com>
> >
> > The current implementation has three bugs,
> >  * segment limits are not enforced in protected mode if the L bit is set
> >    in the target segment descriptor[1]
> >  * segment limits are not enforced in compatability mode (ljmp to 32-bit
> >    code segment in long mode)
> >  * #GP(new_cs) is generated rather than #GP(0)
> >
> > Now the segment limits are enforced if we're not in long mode OR the
> > target code segment doesn't have the L bit set.
> >
> > [1] this is an invalid configuration (in protected mode the L bit is
> > reserved and should be set to zero), but qemu doesn't enforce that.
>
> Stupid question, why not fix that instead at least for this case?
>

I suppose we could check for that here, sure.  I was thinking we'd want to
check for that when the segment descriptor was actually loaded, but now
that I think about it there isn't a central place to do that other than in
the lgdt and lldt instructions.

Is there a central place to do that sort of validation?  We could do it in
load_segment_ra, but that doesn't feel quite right.  Otherwise it's
whack-a-mole to check validity at every place a code segment is
referenced---but maybe that's ok, there probably aren't too many of them.

WDYT?


>
> > Signed-off-by: Andrew Oates <aoates@google.com>
> > ---
> > The limit check is still incorrect for ljmp-through-call-gate in 64-bit
> > mode.  That's a larger fix I'm still working on.
>
> Can you resend the call-gate for both ljmp and lcall when you're done
> (there's plenty of time until 3.1)?
>

Yeah, absolutely, just want to do some more testing on it to make sure it
covers all the corner cases correctly.


>
> Thanks,
>
> Paolo
>
> >  target/i386/seg_helper.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
> > index 00301a0c04..975365fd30 100644
> > --- a/target/i386/seg_helper.c
> > +++ b/target/i386/seg_helper.c
> > @@ -1628,8 +1628,8 @@ void helper_ljmp_protected(CPUX86State *env, int
> new_cs, target_ulong new_eip,
> >          }
> >          limit = get_seg_limit(e1, e2);
> >          if (new_eip > limit &&
> > -            !(env->hflags & HF_LMA_MASK) && !(e2 & DESC_L_MASK)) {
> > -            raise_exception_err_ra(env, EXCP0D_GPF, new_cs & 0xfffc,
> GETPC());
> > +            (!(env->hflags & HF_LMA_MASK) || !(e2 & DESC_L_MASK))) {
> > +            raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
> >          }
> >          cpu_x86_load_seg_cache(env, R_CS, (new_cs & 0xfffc) | cpl,
> >                         get_seg_base(e1, e2), limit, e2);
> >
>
>

Re: [Qemu-devel] [PATCH] target-i386: fix segment limit check in ljmp

[Paolo Bonzini]

On 17/08/2018 20:04, Andrew Oates wrote:
>>> [1] this is an invalid configuration (in protected mode the L bit is
>>> reserved and should be set to zero), but qemu doesn't enforce that.
>
> Is there a central place to do that sort of validation?  We could do it
> in load_segment_ra, but that doesn't feel quite right.  Otherwise it's
> whack-a-mole to check validity at every place a code segment is
> referenced---but maybe that's ok, there probably aren't too many of them.
> 
> WDYT?

To find them you have to look for the calls to cpu_x86_load_seg_cache.

However, the L bit is simply ignored in the descriptor cache outside
64-bit mode (at least that's what you can guess from the Intel manual's
26.3 CHECKING AND LOADING GUEST STATE, which is as close as you can get
to an official explanation of the descriptor cache), so you'd have to
take that into account too and do the check in the callers of
cpu_x86_load_seg_cache.

And in fact, the wording is a bit wishy-washy in both the Intel and AMD
manuals, for example AMD says of bit 21 that

	Generally, software should clear all reserved bits to 0, so they
	can be defined in future revisions to the AMD64 architecture.

so I am not 100% sure that the processor will actually raise an
exception if L is 1.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] target-i386: fix segment limit check in ljmp

[Andrew Oates]

On Mon, Aug 20, 2018 at 6:06 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 17/08/2018 20:04, Andrew Oates wrote:
> >>> [1] this is an invalid configuration (in protected mode the L bit is
> >>> reserved and should be set to zero), but qemu doesn't enforce that.
> >
> > Is there a central place to do that sort of validation?  We could do it
> > in load_segment_ra, but that doesn't feel quite right.  Otherwise it's
> > whack-a-mole to check validity at every place a code segment is
> > referenced---but maybe that's ok, there probably aren't too many of them.
> >
> > WDYT?
>
> To find them you have to look for the calls to cpu_x86_load_seg_cache.
>
> However, the L bit is simply ignored in the descriptor cache outside
> 64-bit mode (at least that's what you can guess from the Intel manual's
> 26.3 CHECKING AND LOADING GUEST STATE, which is as close as you can get
> to an official explanation of the descriptor cache), so you'd have to
> take that into account too and do the check in the callers of
> cpu_x86_load_seg_cache.
>
> And in fact, the wording is a bit wishy-washy in both the Intel and AMD
> manuals, for example AMD says of bit 21 that
>
>         Generally, software should clear all reserved bits to 0, so they
>         can be defined in future revisions to the AMD64 architecture.
>
> so I am not 100% sure that the processor will actually raise an
> exception if L is 1.

That's good enough for me :)  My vote would be to continue to ignore
the L bit in 32-bit mode, then.  We should just remove the part that
implies qemu should enforce it from my patch commit message.

>
> Thanks,
>
> Paolo

Re: [Qemu-devel] [PATCH] target-i386: fix segment limit check in ljmp

[Paolo Bonzini]

On 20/08/2018 13:32, Andrew Oates wrote:
> We should just remove the part that
> implies qemu should enforce it from my patch commit message.

Ok, will do.

Paolo