x86: enforce DPL checking on task gate switches invoked through IDT

Hi,

x86 software emulation (non-KVM mode) does not check privilege levels on
task gate switches ... so one can invoke a kernel's double fault handler
from user space -- very bad.

Expected behaviour (testcase works with any linux distribution + gcc):
  $ cat test.c
    int main(void)
    {
      __asm__ volatile ("int $8");
    }
  $ gcc test.c
  $ ./a.out
  Segmentation fault
  $
... and not a kernel panic (double fault)


Best Regards,
Alex

---
 x86 software emulation (non-KVM mode) does not check privilege
 levels on task gate switches ... so one can invoke a kernel's
 double fault handler from user space.
 
 Signed-off-by: Alex Zuepke <azu@sysgo.com>

Ping, no response so far ...


Thanks,
Alex

Alex ZUEPKE wrote:
> Hi,
> 
> x86 software emulation (non-KVM mode) does not check privilege levels on
> task gate switches ... so one can invoke a kernel's double fault handler
> from user space -- very bad.
> 
> Expected behaviour (testcase works with any linux distribution + gcc):
>   $ cat test.c
>     int main(void)
>     {
>       __asm__ volatile ("int $8");
>     }
>   $ gcc test.c
>   $ ./a.out
>   Segmentation fault
>   $
> ... and not a kernel panic (double fault)
> 
> 
> Best Regards,
> Alex
> 
> ---
>  x86 software emulation (non-KVM mode) does not check privilege
>  levels on task gate switches ... so one can invoke a kernel's
>  double fault handler from user space.
>  
>  Signed-off-by: Alex Zuepke <azu@sysgo.com>
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index 5fff8d5..23c5542 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -578,12 +578,17 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
>      e2 = cpu_ldl_kernel(env, ptr + 4);
>      /* check gate type */
>      type = (e2 >> DESC_TYPE_SHIFT) & 0x1f;
> +    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> +    cpl = env->hflags & HF_CPL_MASK;
>      switch (type) {
>      case 5: /* task gate */
>          /* must do that check here to return the correct error code */
>          if (!(e2 & DESC_P_MASK)) {
>              raise_exception_err(env, EXCP0B_NOSEG, intno * 8 + 2);
>          }
> +        /* check privilege if software int */
> +        if (is_int && dpl < cpl)
> +            raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
>          switch_tss(env, intno * 8, e1, e2, SWITCH_TSS_CALL, old_eip);
>          if (has_error_code) {
>              int type;
> @@ -616,8 +621,6 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
>          raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
>          break;
>      }
> -    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> -    cpl = env->hflags & HF_CPL_MASK;
>      /* check privilege if software int */
>      if (is_int && dpl < cpl) {
>          raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
> 
>

On 08/17/12 11:30, Alex ZUEPKE wrote:
> Hi,
>
> x86 software emulation (non-KVM mode) does not check privilege levels on
> task gate switches ... so one can invoke a kernel's double fault handler
> from user space -- very bad.
>
> Expected behaviour (testcase works with any linux distribution + gcc):
>    $ cat test.c
>      int main(void)
>      {
>        __asm__ volatile ("int $8");
>      }
>    $ gcc test.c
>    $ ./a.out
>    Segmentation fault
>    $
> ... and not a kernel panic (double fault)
Some where you should say that this is a 32 bit only issue.
>
> Best Regards,
> Alex
>
> ---
>   x86 software emulation (non-KVM mode) does not check privilege
>   levels on task gate switches ... so one can invoke a kernel's
>   double fault handler from user space.
>   
>   Signed-off-by: Alex Zuepke <azu@sysgo.com>
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index 5fff8d5..23c5542 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -578,12 +578,17 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
>       e2 = cpu_ldl_kernel(env, ptr + 4);
>       /* check gate type */
>       type = (e2 >> DESC_TYPE_SHIFT) & 0x1f;
> +    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> +    cpl = env->hflags & HF_CPL_MASK;
>       switch (type) {
>       case 5: /* task gate */
>           /* must do that check here to return the correct error code */
>           if (!(e2 & DESC_P_MASK)) {
>               raise_exception_err(env, EXCP0B_NOSEG, intno * 8 + 2);
>           }
> +        /* check privilege if software int */
> +        if (is_int && dpl < cpl)
> +            raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
>           switch_tss(env, intno * 8, e1, e2, SWITCH_TSS_CALL, old_eip);
>           if (has_error_code) {
>               int type;
> @@ -616,8 +621,6 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
>           raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
>           break;
>       }
> -    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> -    cpl = env->hflags & HF_CPL_MASK;
>       /* check privilege if software int */
>       if (is_int && dpl < cpl) {
>           raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
>
I think it makes sense to move the next 2 checks into the switch (no 
real code flow change).

Doing this I get:

@@ -611,21 +617,19 @@ static void do_interrupt_protected(CPUX86State 
*env, int intno, int is_int,
      case 7: /* 286 trap gate */
      case 14: /* 386 interrupt gate */
      case 15: /* 386 trap gate */
+        /* check privilege if software int */
+        if (is_int && dpl < cpl) {
+            raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
+        }
+        /* check valid bit */
+        if (!(e2 & DESC_P_MASK)) {
+            raise_exception_err(env, EXCP0B_NOSEG, intno * 8 + 2);
+        }
          break;
      default:
          raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
          break;
      }
-    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
-    cpl = env->hflags & HF_CPL_MASK;
-    /* check privilege if software int */
-    if (is_int && dpl < cpl) {
-        raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
-    }
-    /* check valid bit */
-    if (!(e2 & DESC_P_MASK)) {
-        raise_exception_err(env, EXCP0B_NOSEG, intno * 8 + 2);
-    }
      selector = e1 >> 16;
      offset = (e2 & 0xffff0000) | (e1 & 0x0000ffff);
      if ((selector & 0xfffc) == 0) {

On 31 August 2012 17:54, Don Slutz <Don@cloudswitch.com> wrote:
> I think it makes sense to move the next 2 checks into the switch (no real
> code flow change).

I agree (for symmetry). If you do that then I think the
combination of those two patches means that in the task
gate case we do the !(e2 & DESC_P_MASK) check first and
then the dpl<cpl check; whereas in the other cases we
do them the other way around. Is that actually correct
behaviour? If so I think it probably deserves a comment
(perhaps just a clarification/expansion of the one currently
in the 'case 5' code) to the effect that the error
handling on the task gate case is genuinely different.

-- PMM

On Fri, Aug 17, 2012 at 3:30 PM, Alex ZUEPKE <azuepke@sysgo.com> wrote:
> Hi,
>
> x86 software emulation (non-KVM mode) does not check privilege levels on
> task gate switches ... so one can invoke a kernel's double fault handler
> from user space -- very bad.
>
> Expected behaviour (testcase works with any linux distribution + gcc):
>   $ cat test.c
>     int main(void)
>     {
>       __asm__ volatile ("int $8");
>     }
>   $ gcc test.c
>   $ ./a.out
>   Segmentation fault
>   $
> ... and not a kernel panic (double fault)
>
>
> Best Regards,
> Alex
>

This would become the commit message and the text below would be cut by git am.

> ---
>  x86 software emulation (non-KVM mode) does not check privilege
>  levels on task gate switches ... so one can invoke a kernel's
>  double fault handler from user space.

"Fix by adding the privilege checks, raise GPF when fails"?

>
>  Signed-off-by: Alex Zuepke <azu@sysgo.com>
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index 5fff8d5..23c5542 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -578,12 +578,17 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
>      e2 = cpu_ldl_kernel(env, ptr + 4);
>      /* check gate type */
>      type = (e2 >> DESC_TYPE_SHIFT) & 0x1f;
> +    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> +    cpl = env->hflags & HF_CPL_MASK;
>      switch (type) {
>      case 5: /* task gate */
>          /* must do that check here to return the correct error code */
>          if (!(e2 & DESC_P_MASK)) {
>              raise_exception_err(env, EXCP0B_NOSEG, intno * 8 + 2);
>          }
> +        /* check privilege if software int */
> +        if (is_int && dpl < cpl)

Missing braces, please read CODING_STYLE.

> +            raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
>          switch_tss(env, intno * 8, e1, e2, SWITCH_TSS_CALL, old_eip);
>          if (has_error_code) {
>              int type;
> @@ -616,8 +621,6 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
>          raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
>          break;
>      }
> -    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> -    cpl = env->hflags & HF_CPL_MASK;
>      /* check privilege if software int */
>      if (is_int && dpl < cpl) {
>          raise_exception_err(env, EXCP0D_GPF, intno * 8 + 2);
>
>