Re: [Xen-devel] [PATCH for 4.9 3/6] x86/hvm: Fix segmentation logic for system segments

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.03.17 at 21:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2374,13 +2374,27 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
>      return X86EMUL_OKAY;
>  }
>  
> +enum hvm_segmentation_mode hvm_seg_mode(
> +    const struct vcpu *v, enum x86_segment seg,
> +    const struct segment_register *cs)

The inputs here are at least somewhat counterintuitive (for example,
from an abstract pov it is unexpected that the result depends on seg
and cs). At the very least I think the naming should make clear that
cs is not just some code segment, but the CS v has currently in use
(e.g. active_cs). Going further the question is whether having this
helper is really useful (and not perhaps inviting mis-use), and hence
whether the inputs passed here wouldn't better be passed directly
to hvm_virtual_to_linear_addr(), the more that the "seg" argument
is required to match up between the two calls.

> +{
> +    if ( !(v->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE) )
> +        return hvm_seg_mode_real;

What about VM86 mode?

> +    if ( hvm_long_mode_active(v) &&
> +         (is_x86_system_segment(seg) || cs->attr.fields.l) )
> +        return hvm_seg_mode_long;
> +
> +    return hvm_seg_mode_prot;

Did you verify this actually matching real hardware behavior? There
being obvious anomalies when compat ring-0 code executes
LGDT/LIDT/SGDT/SIDT (in long mode these ought to have 10-byte
operands, yet 32-bit/compat code would expect 6-byte ones, so
one of the two views is necessarily wrong, and whichever it is, it
introduces an inconsistency), I wouldn't take it for given that _all_
descriptor table accessing insns behave like they would from a
64-bit code segment (I nevertheless assume they do, but as I
can't see it written down anywhere, we shouldn't assume so,
considering how many other oddities there are in x86).

This question is also being supported by the SDM using the same
standard "Same exceptions as in protected mode" in the
respective insns' "Compatibility Mode Exceptions" sections, yet
the behavior above implies that #GP(0) might also result for
compat mode descriptor table accesses if the descriptor address
ends up being non-canonical. Interestingly enough the PM
doesn't separate exception specifications for 32-bit protected,
compat, and 64-bit modes.

Jan


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