Re: [PATCH SpectreV1+L1TF v5 6/9] is_control_domain: block speculation

["Jan Beulich" <JBeulich@suse.com>]

>>> On 06.02.19 at 16:36, <nmanthey@amazon.de> wrote:
> On 2/6/19 16:03, Jan Beulich wrote:
>>>>> On 29.01.19 at 15:43, <nmanthey@amazon.de> wrote:
>>> @@ -908,10 +909,10 @@ void watchdog_domain_destroy(struct domain *d);
>>>   *    (that is, this would not be suitable for a driver domain)
>>>   *  - There is never a reason to deny the hardware domain access to this
>>>   */
>>> -#define is_hardware_domain(_d) ((_d) == hardware_domain)
>>> +#define is_hardware_domain(_d) evaluate_nospec((_d) == hardware_domain)
>>>  
>>>  /* This check is for functionality specific to a control domain */
>>> -#define is_control_domain(_d) ((_d)->is_privileged)
>>> +#define is_control_domain(_d) evaluate_nospec((_d)->is_privileged)
>> I'm afraid there's another fly in the ointment here: While looking at
>> the still questionable grant table change I've started wondering
>> about constructs like
>>
>>     case XENMEM_machphys_mapping:
>>     {
>>         struct xen_machphys_mapping mapping = {
>>             .v_start = MACH2PHYS_VIRT_START,
>>             .v_end   = MACH2PHYS_VIRT_END,
>>             .max_mfn = MACH2PHYS_NR_ENTRIES - 1
>>         };
>>
>>         if ( !mem_hotplug && is_hardware_domain(current->domain) )
>>             mapping.max_mfn = max_page - 1;
>>         if ( copy_to_guest(arg, &mapping, 1) )
>>             return -EFAULT;
>>
>>         return 0;
>>     }
>>
>> Granted the example here could be easily re-arranged, but there
>> are others where this is less easy or not possible at all. What I'm
>> trying to get at are constructs where the such-protected
>> predicates sit on the right side of && or || - afaict (also from
>> looking at some much simplified code examples) the intended
>> protection is gone in these cases.
> 
> I do not follow this. Independently of other conditionals in the if
> statement, there should be an lfence instruction between the
> "is_domain_control(...)" evaluation and accessing the max_page variable
> - in case the code actually protects accessing that variable via that
> function.

Hmm, yes, I may have been confused by looking at the && and ||
variants of the generated code, and mixing up the cases.

> I validated this property for the above code snippet in the generated
> assembly. However, I just noticed another problem: while my initial
> version just placed the lfence instruction right into the code, not the
> arch_barrier_nospec_true method is called via callq. I would like to get
> the instructions to be embedded into the code directly, without the call
> detour. In case I cannot force the compiler to do that, I would go back
> to using a fixed lfence statement on all x86 platforms.

I think we had made pretty clear that incurring the overhead even
onto unaffected platforms is not an option. Did you try whether
adding always_inline helps? (I take it that this is another case of
the size-of-asm issue that's being worked on in Linux as well iirc.)

Jan



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