Re: [PATCH] target/i386: Check privilege level for protected mode 'int N' task gate

[Paolo Bonzini <pbonzini@redhat.com>]

On 21/11/20 23:44, Peter Maydell wrote:
> When the 'int N' instruction is executed in protected mode, the
> pseudocode in the architecture manual specifies that we need to check:
> 
>   * vector number within IDT limits
>   * selected IDT descriptor is a valid type (interrupt, trap or task gate)
>   * if this was a software interrupt then gate DPL < CPL
> 
> The way we had structured the code meant that the privilege check for
> software interrupts ended up not in the code path taken for task gate
> handling, because all of the task gate handling code was in the 'case 5'
> of the switch which was checking "is this descriptor a valid type".
> 
> Move the task gate handling code out of that switch (so that it is now
> purely doing the "valid type?" check) and below the software interrupt
> privilege check.
> 
> The effect of this missing check was that in a guest userspace binary
> executing 'int 8' would cause a guest kernel panic rather than the
> userspace binary being handed a SEGV.
> 
> This is essentially the same bug fixed in VirtualBox in 2012:
> https://www.halfdog.net/Security/2012/VirtualBoxSoftwareInterrupt0x8GuestCrash/
> 
> Note that for QEMU this is not a security issue because it is only
> present when using TCG.
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1813201
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/i386/seg_helper.c | 35 +++++++++++++++++++++--------------
>   1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
> index 09b6554660..5c8b8652f5 100644
> --- a/target/i386/seg_helper.c
> +++ b/target/i386/seg_helper.c
> @@ -633,6 +633,24 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
>       type = (e2 >> DESC_TYPE_SHIFT) & 0x1f;
>       switch (type) {
>       case 5: /* task gate */
> +    case 6: /* 286 interrupt gate */
> +    case 7: /* 286 trap gate */
> +    case 14: /* 386 interrupt gate */
> +    case 15: /* 386 trap gate */
> +        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);
> +    }
> +
> +    if (type == 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);
> @@ -660,21 +678,10 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
>               SET_ESP(esp, mask);
>           }
>           return;
> -    case 6: /* 286 interrupt gate */
> -    case 7: /* 286 trap gate */
> -    case 14: /* 386 interrupt gate */
> -    case 15: /* 386 trap gate */
> -        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);
>       }
> +
> +    /* Otherwise, trap or interrupt gate */
> +
>       /* check valid bit */
>       if (!(e2 & DESC_P_MASK)) {
>           raise_exception_err(env, EXCP0B_NOSEG, intno * 8 + 2);
> 

Queued, thanks.

Paolo