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 <[email protected]> >> --- >> 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
