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

PATCH In review Alex ZUEPKE 4 participants 5 messages Aug 17, 2012 – Sep 1, 2012

lore

Share & tools

lore

Alex ZUEPKE

author

2012-08-17 15:30 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

target-i386/seg_helper.c +5 -2

index 5fff8d5..23c5542 100644
@@ -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);

Alex ZUEPKE

author

2012-08-27 16:39 UTC #1 lore raw

Ping, no response so far ...

Thanks,
Alex

Quoted 61 lines · Alex ZUEPKE

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 <[email protected]>

target-i386/seg_helper.c +5 -2

index 5fff8d5..23c5542 100644
@@ -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);

Don Slutz

2012-08-31 16:54 UTC #2 lore raw

In reply to Alex ZUEPKE

Quoted 7 lines · Alex ZUEPKE

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

Quoted 10 lines

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

Quoted 34 lines

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 <[email protected]>

target-i386/seg_helper.c +5 -0

index 5fff8d5..23c5542 100644
@@ -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;

target-i386/seg_helper.c static void do_interrupt_protected(CPUX86State *env, int intno, int is_int, · line 626

Quoted 8 lines

target-i386/seg_helper.c +0 -2

@@ -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) {

Peter Maydell

2012-08-31 17:02 UTC #3 lore raw

Quoted 2 lines · Don Slutz

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

Blue Swirl

2012-09-01 11:42 UTC #4 lore raw

In reply to Alex ZUEPKE

Quoted 19 lines · Alex ZUEPKE

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)

Quoted 3 lines

Best Regards,
Alex

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

Quoted 4 lines

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

Quoted 10 lines

Signed-off-by Alex Zuepke <[email protected]>

target-i386/seg_helper.c +0 -0

index 5fff8d5..23c5542 100644
@@ -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;

target-i386/seg_helper.c static void do_interrupt_protected(CPUX86State *env, int intno, int is_int, · line 590

Quoted 10 lines

target-i386/seg_helper.c +4 -0

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

Quoted 15 lines

target-i386/seg_helper.c +1 -2

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