Re: [Xen-devel] [PATCH RFC v13 14/20] pvh: Use PV handlers for emulated forced invalid ops, cpuid, and IO

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.09.13 at 18:49, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> v13:
>  - Remove unnecessary privilege check in PIO path, update related comment

While this is the correct thing, ...

>      case EXIT_REASON_IO_INSTRUCTION:
> -        exit_qualification = __vmread(EXIT_QUALIFICATION);
> -        if ( exit_qualification & 0x10 )
> +        if ( is_pvh_vcpu(v) )
>          {
> -            /* INS, OUTS */
> -            if ( !handle_mmio() )
> -                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +            if ( !emulate_privileged_op(regs) )

.. we still shouldn't do this blindly. It is a latent security issue to
do double decoding on an instruction: The hardware decoded it,
and did all guest side checks. If a malicious guest forges the
instruction to be something else by the time
emulate_privileged_op() gets to decode it, we may induce a
guest side security violation. As the decoded information is
available, we ought to use it here.

> +                hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code);
>          }
>          else
>          {
> -            /* IN, OUT */
> -            uint16_t port = (exit_qualification >> 16) & 0xFFFF;
> -            int bytes = (exit_qualification & 0x07) + 1;
> -            int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
> -            if ( handle_pio(port, bytes, dir) )
> -                update_guest_eip(); /* Safe: IN, OUT */
> +            exit_qualification = __vmread(EXIT_QUALIFICATION);
> +            if ( exit_qualification & 0x10 )
> +            {
> +                /* INS, OUTS */
> +                if ( !handle_mmio() )

With this moved into a !PVH code path, is there any path that can
still actively reach handle_mmio() for a PVH guest? If not, the
check an earlier patch put there propbably ought to become an
ASSERT().

Also, to reduce the cde churn due to this patch, if you added
a "break;" to the end of the if() path above, most of the
indentation only changes here would go away.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel