[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> 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?

Jan



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.