* Re: [Qemu-devel] [PATCH 1/1] x86: enforce DPL checking on task gate switches
2014-05-13 21:12 [Qemu-devel] [PATCH 1/1] x86: enforce DPL checking on task gate switches Alex Zuepke
@ 2014-05-14 17:45 ` Richard Henderson
0 siblings, 0 replies; 2+ messages in thread
From: Richard Henderson @ 2014-05-14 17:45 UTC (permalink / raw)
To: Alex Zuepke, qemu-devel
On 05/13/2014 02:12 PM, Alex Zuepke wrote:
> x86 software emulation (non-KVM mode) does not check privilege levels on task gate switches.
> An "int $8" in user mode panics any OS kernel by a forbidden direct call into the double fault handler.
>
> This testcase crashes a Linux kernel with a double fault panic:
>
> $ cat test.c
> int main(void)
> {
> __asm__ volatile ("int $8");
> }
> $ gcc test.c
> $ ./a.out
> panic ...
>
> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
> ---
> target-i386/seg_helper.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index 8c3f92c..d7f3d49 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -582,12 +582,18 @@ 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;
> @@ -620,8 +626,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);
>
Please refer to the (rather large) pseudo code for the "int" instruction:
# IF ...selected IDT descriptor is not an interrupt-, trap-, or task-gate type
# THEN #GP(error_code(vector_number,1,EXT)); FI;
# (* idt operand to error_code set because vector is used *)
This would be that switch statement that we currently have, with the default
case raising this exception. I'll note that we appear to be ignoring EXT?
I.e. this should be intno * 8 + 2 + !is_int.
# IF software interrupt (* Generated by INT n, INT3, or INTO *)
# THEN
# IF gate DPL < CPL (* PE = 1, DPL < CPL, software interrupt *)
# THEN #GP(error_code(vector_number,1,0)); FI;
# (* idt operand to error_code set because vector is used *)
# (* ext operand to error_code is 0 because INT n ... *)
# FI;
This is the check that you're moving. But...
# IF gate not present
# THEN #NP(error_code(vector_number,1,EXT)); FI;
# (* idt operand to error_code set because vector is used *)
... the #NP check should come after the #GP check. I'll note the missed EXT as
part of the error_code here as well.
So it would seem that moving the entire "check privilege" block above the
switch statement will do the right thing, since the non-task-gate entries in
the switch either do nothing or raise another identical #GP.
r~
^ permalink raw reply [flat|nested] 2+ messages in thread