* [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT
@ 2012-08-17 15:30 Alex ZUEPKE
2012-08-27 16:39 ` Alex ZUEPKE
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Alex ZUEPKE @ 2012-08-17 15:30 UTC (permalink / raw)
To: qemu-devel
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT
2012-08-17 15:30 [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT Alex ZUEPKE
@ 2012-08-27 16:39 ` Alex ZUEPKE
2012-08-31 16:54 ` Don Slutz
2012-09-01 11:42 ` Blue Swirl
2 siblings, 0 replies; 6+ messages in thread
From: Alex ZUEPKE @ 2012-08-27 16:39 UTC (permalink / raw)
To: qemu-devel
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);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT
2012-08-17 15:30 [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT Alex ZUEPKE
2012-08-27 16:39 ` Alex ZUEPKE
@ 2012-08-31 16:54 ` Don Slutz
2012-08-31 17:01 ` Peter Maydell
2012-09-01 11:42 ` Blue Swirl
2 siblings, 1 reply; 6+ messages in thread
From: Don Slutz @ 2012-08-31 16:54 UTC (permalink / raw)
To: Alex ZUEPKE; +Cc: qemu-devel
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) {
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT
2012-08-31 16:54 ` Don Slutz
@ 2012-08-31 17:01 ` Peter Maydell
0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2012-08-31 17:01 UTC (permalink / raw)
To: Don Slutz; +Cc: Alex ZUEPKE, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT
2012-08-17 15:30 [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT Alex ZUEPKE
2012-08-27 16:39 ` Alex ZUEPKE
2012-08-31 16:54 ` Don Slutz
@ 2012-09-01 11:42 ` Blue Swirl
2 siblings, 0 replies; 6+ messages in thread
From: Blue Swirl @ 2012-09-01 11:42 UTC (permalink / raw)
To: Alex ZUEPKE; +Cc: qemu-devel
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);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT
@ 2012-08-17 9:14 Alex ZUEPKE
0 siblings, 0 replies; 6+ messages in thread
From: Alex ZUEPKE @ 2012-08-17 9:14 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 518 bytes --]
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)
Forgive me for sending this patch as attachment, I'm not used to git.
Best Regards,
Alex
[-- Attachment #2: x86_task_gate_priv_check.patch --]
[-- Type: text/x-diff, Size: 1558 bytes --]
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.de>
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-01 11:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-17 15:30 [Qemu-devel] [PATCH] x86: enforce DPL checking on task gate switches invoked through IDT Alex ZUEPKE
2012-08-27 16:39 ` Alex ZUEPKE
2012-08-31 16:54 ` Don Slutz
2012-08-31 17:01 ` Peter Maydell
2012-09-01 11:42 ` Blue Swirl
-- strict thread matches above, loose matches on Subject: below --
2012-08-17 9:14 Alex ZUEPKE
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).