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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan