On 23.04.2020 12:57, Roger Pau Monné wrote:
> On Thu, Apr 23, 2020 at 12:41:49PM +0200, Jan Beulich wrote:
>> On 23.04.2020 12:30, Wei Liu wrote:
>>> On Wed, Apr 22, 2020 at 06:33:38PM +0200, Roger Pau Monné wrote:
>>>> On Thu, Apr 16, 2020 at 03:59:07PM +0200, Roger Pau Monne wrote:
>>>>> @@ -254,3 +257,14 @@ unsigned int flush_area_local(const void *va,
>>>>> unsigned int flags)
>>>>>
>>>>> return flags;
>>>>> }
>>>>> +
>>>>> +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask)
>>>>> +{
>>>>> + unsigned int flags = (is_pv_domain(d) || paging_mode_shadow(d) ?
>>>>> FLUSH_TLB
>>>>> + : 0) |
>>>>> + (is_hvm_domain(d) && cpu_has_svm ?
>>>>> FLUSH_HVM_ASID_CORE
>>>>> + : 0);
>>>>
>>>> Maybe I'm getting confused, but I think the above is wrong and ASID
>>>> should _always_ be flushed when running a HVM domain in shadow mode
>>>> regardless of whether the underlying hw is Intel or AMD, ie:
>>>>
>>>> bool shadow = paging_mode_shadow(d);
>>>> unsigned int flags = (shadow ? FLUSH_TLB : 0) |
>>>> (is_hvm_domain(d) &&
>>>> (cpu_has_svm || shadow) ? FLUSH_HVM_ASID_CORE : 0);
>>>
>>> This sort of long expression is prone to error. See XSA-316.
>>
>> To be honest I consider it quite fine. XSA-316 was in particular
>> because of successive closing parentheses, of which there are
>> none here. (This isn't to say I would strictly mind splitting,
>> but I fear this would result in (multiple?) single use local
>> variables.)
>
> Right now it's exactly (including the indentation):
>
> bool shadow = paging_mode_shadow(d);
>
> return (shadow ? FLUSH_TLB : 0) |
> (is_hvm_domain(d) && (cpu_has_svm || shadow) ? FLUSH_HVM_ASID_CORE
> : 0);
>
> I could change it to:
>
> bool shadow = paging_mode_shadow(d);
> bool asid = is_hvm_domain(d) && (cpu_has_svm || shadow);
>
> return (shadow ? FLUSH_TLB : 0) | (asid ? FLUSH_HVM_ASID_CORE : 0);
>
> But would result in a single use asid local variable.
>
> I think XSA-316 was slightly different because the issue arose from
> assigning and comparing a variable inside of an if condition, which is
> not the case here. I however don't mind changing it if it's regarded
> as potentially insecure, or hard to read.
I'd be okay to ack either, but would still somewhat prefer the original
variant.
Jan
