Re: [PATCH v2 9/9] x86/shadow: harden shadow_size()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.01.2023 20:13, Andrew Cooper wrote:
> On 12/01/2023 10:42 am, Jan Beulich wrote:
>> On 12.01.2023 11:31, Andrew Cooper wrote:
>>> On 12/01/2023 9:47 am, Jan Beulich wrote:
>>>> On 12.01.2023 00:15, Andrew Cooper wrote:
>>>>> On 11/01/2023 1:57 pm, Jan Beulich wrote:
>>>>>> Make HVM=y release build behavior prone against array overrun, by
>>>>>> (ab)using array_access_nospec(). This is in particular to guard against
>>>>>> e.g. SH_type_unused making it here unintentionally.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>> ---
>>>>>> v2: New.
>>>>>>
>>>>>> --- a/xen/arch/x86/mm/shadow/private.h
>>>>>> +++ b/xen/arch/x86/mm/shadow/private.h
>>>>>> @@ -27,6 +27,7 @@
>>>>>>  // been included...
>>>>>>  #include <asm/page.h>
>>>>>>  #include <xen/domain_page.h>
>>>>>> +#include <xen/nospec.h>
>>>>>>  #include <asm/x86_emulate.h>
>>>>>>  #include <asm/hvm/support.h>
>>>>>>  #include <asm/atomic.h>
>>>>>> @@ -368,7 +369,7 @@ shadow_size(unsigned int shadow_type)
>>>>>>  {
>>>>>>  #ifdef CONFIG_HVM
>>>>>>      ASSERT(shadow_type < ARRAY_SIZE(sh_type_to_size));
>>>>>> -    return sh_type_to_size[shadow_type];
>>>>>> +    return array_access_nospec(sh_type_to_size, shadow_type);
>>>>> I don't think this is warranted.
>>>>>
>>>>> First, if the commit message were accurate, then it's a problem for all
>>>>> arrays of size SH_type_unused, yet you've only adjusted a single instance.
>>>> Because I think the risk is higher here than for other arrays. In
>>>> other cases we have suitable build-time checks (HASH_CALLBACKS_CHECK()
>>>> in particular) which would trip upon inappropriate use of one of the
>>>> types which are aliased to SH_type_unused when !HVM.
>>>>
>>>>> Secondly, if it were reliably 16 then we could do the basically-free
>>>>> "type &= 15;" modification.  (It appears my change to do this
>>>>> automatically hasn't been taken yet.), but we'll end up with lfence
>>>>> variation here.
>>>> How could anything be "reliably 16"? Such enums can change at any time:
>>>> They did when making HVM types conditional, and they will again when
>>>> adding types needed for 5-level paging.
>>>>
>>>>> But the value isn't attacker controlled.  shadow_type always comes from
>>>>> Xen's metadata about the guest, not the guest itself.  So I don't see
>>>>> how this can conceivably be a speculative issue even in principle.
>>>> I didn't say anything about there being a speculative issue here. It
>>>> is for this very reason that I wrote "(ab)using".
>>> Then it is entirely wrong to be using a nospec accessor.
>>>
>>> Speculation problems are subtle enough, without false uses of the safety
>>> helpers.
>>>
>>> If you want to "harden" against runtime architectural errors, you want
>>> to up the ASSERT() to a BUG(), which will execute faster than sticking
>>> an hiding an lfence in here, and not hide what is obviously a major
>>> malfunction in the shadow pagetable logic.
>> I should have commented on this earlier already: What lfence are you
>> talking about?
> 
> The one I thought was hiding under array_access_nospec(), but I forgot
> we'd done the sbb trick.
> 
>> As to BUG() - the goal here specifically is to avoid a
>> crash in release builds, by forcing the function to return zero (via
>> having it use array slot zero for out of range indexes).
> 
> I'm very uneasy about having something this deep inside a component,
> which ASSERT()s correctness doing something custom like this "just to
> avoid crashing".
> 
> There is either something important which makes this more likely than
> most to go wrong at runtime, or there is not.  And honestly, I can't see
> why it is more risky at runtime.

Well, okay. I did explain why I think there is an increased risk here.

> If we really do need to clamp it, then we need a brand new helper with a
> name that doesn't reference speculation at all.  It's fine for *_nospec
> to reuse this helper, stating the safety of doing so, but at a code
> level there need to be two appropriately named helpers for their two
> logically-unrelated purposes.

I don't think anything can sensibly be made for more general purpose
(not speculation related) use. Here I'm specifically utilizing that
array slot 0 is being picked as the fallback slot _and_ that slot is
actually suitable. This may not be the case for other arrays.

Anyway - taking things together I will then simply consider the patch
rejected, despite it being a seemingly easy step of hardening.

Jan