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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

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.

~Andrew