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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

~Andrew