Re: [Xen-devel] [PATCH SpectreV1+L1TF v5 9/9] common/memory: block speculative out-of-bound accesses

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 2/6/19 17:08, Jan Beulich wrote:
>>>> On 06.02.19 at 16:39, <nmanthey@xxxxxxxxx> wrote:
>> On 2/6/19 16:25, Jan Beulich wrote:
>>>>>> On 29.01.19 at 15:43, <nmanthey@xxxxxxxxx> wrote:
>>>> @@ -33,10 +34,10 @@ unsigned long __read_mostly 
>>>> pdx_group_valid[BITS_TO_LONGS(
>>>>  
>>>>  bool __mfn_valid(unsigned long mfn)
>>>>  {
>>>> -    return likely(mfn < max_page) &&
>>>> -           likely(!(mfn & pfn_hole_mask)) &&
>>>> -           likely(test_bit(pfn_to_pdx(mfn) / PDX_GROUP_COUNT,
>>>> -                           pdx_group_valid));
>>>> +    return evaluate_nospec(likely(mfn < max_page) &&
>>>> +                           likely(!(mfn & pfn_hole_mask)) &&
>>>> +                           likely(test_bit(pfn_to_pdx(mfn) / 
>>>> PDX_GROUP_COUNT,
>>>> +                                           pdx_group_valid)));
>>> Other than in the questionable grant table case, here I agree that
>>> you want to wrap the entire construct. This has an unwanted effect
>>> though: The test_bit() may still be speculated into with an out-of-
>>> bounds mfn. (As mentioned elsewhere, operations on bit arrays are
>>> an open issue altogether.) I therefore think you want to split this into
>>> two:
>>>
>>> bool __mfn_valid(unsigned long mfn)
>>> {
>>>     return likely(evaluate_nospec(mfn < max_page)) &&
>>>            evaluate_nospec(likely(!(mfn & pfn_hole_mask)) &&
>>>                            likely(test_bit(pfn_to_pdx(mfn) / 
>>> PDX_GROUP_COUNT,
>>>                                            pdx_group_valid)));
>>> }
>> I can split the code. However, I wonder whether the test_bit accesses
>> should be protected separately, or actually as part of the test_bit
>> method themselves. Do you have any plans to do that already, because in
>> that case I would not have to modify the code.
> I don't think we want to do that in test_bit() and friends
> themselves, as that would likely produce more unnecessary
> changes than necessary ones. Even the change here
> already looks to have much bigger impact than would be
> wanted, as in the common case MFNs aren't guest controlled.
> ISTR that originally you had modified just a single call site,
> but I can't seem to find that in my inbox anymore. If that
> was the case, what exactly were the criteria upon which
> you had chosen this sole caller?

I understand that these fixes should not go into test_bit itself. I
could add a local array_index_nospec fix for this call, to not introduce
another lfence to be passed.

I picked the specific caller in the first versions, because there was a
direct path from a hypercall where the guest had full control over mfn.
Iirc, that call was not spotted by tooling, but by manual analysis.

Best,
Norbert




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

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