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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 03/04/17 10:13, Jan Beulich wrote:
>>>> On 31.03.17 at 21:50, <andrew.cooper3@citrix.com> 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.

I purposefully wanted to avoid people opencoding the logic and getting
it wrong (looks like even I got it wrong).

I'm not convinced that passing the parameters individually is better.

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

Very good point.

>
>> +    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.

You are mistaken.

{L,S}{I,G}DT are {read,write}_segment_register() operations, using a
plain memory operand.

When we come to the segmentation check, it will be by default
%ds-relative, with size as calculated by op_bytes in the instruction
emulator.

There are no instructions which cause a direct system segment-relative
memory access.  All of them are implicit, such as `int $imm`, `mov $reg,
%sreg`.

Therefore, I think your expectations of the described behaviour are
correct, and that my code is correct and behaves in the way you describe.

~Andrew

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