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

[Jan Beulich <jbeulich@xxxxxxxx>]

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".

Jan