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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.17 at 16:27, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/04/17 10:13, Jan Beulich wrote:
>>>>> 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.
> 
> 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.

I think I didn't make myself clear then: I'm not at all talking about how
the memory access of these insns get carried out, I solely talk about
the size of their operands: In long mode to load IDTR or GDTR you'd
expect a 64-bit base and a 16-bit limit. Hence if _all_ system segment
register related insns worked consistently in long mode, the four
named insns would have to have 10-byte operands. I'm pretty sure
they don't though, so there is _one_ anomaly already. With that I
don't think we can rule out there being other anomalies, with this
not being talked about explicitly anywhere in the doc.

Jan


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