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