Re: [PATCH] x86/pagewalk: Read guest PTEs with ACCESS_ONCE()

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Mon, Aug 10, 2026 at 11:10:29PM +0100, Andrew Cooper wrote:
> This has been a plain C read for as far back as I can trace in history.
> 
> Research into invented-loads has flagged it as a possible vulnerability.
> After careful analysis, it is believed to be a bug only, not a security
> vulnerability.
> 
> The code fits the pattern for invented loads, and it is a risk.
> 
> The analysis suggests that we can read one value out of the guest, operate on
> another, and that this could be an in-guest privliege escalation.  Any entity
> in the guest able to modify the pagetables already has full privilege, so
> while Xen can potentially malfunction, the effects don't cross a privilege
> boundary.
> 
> The analysis also suggests that this is worse for shadow guests because we may
> put the TOCTOU entry in the shadows, but this is inaccurate.  What we put in
> the shadows is still translated under the P2M and refers to guest physical
> address space.
> 
> Either way, harden the accesses.
> 
> Link: 
> https://github.com/xoreaxeaxeax/schrodingers-toctou/blob/main/observer-effect/audits/audit-xen-ptwalk-RELEASE-4.21.1.md#86-per-candidate-finding
> Fixes: 49f7c7364e0a ("Replace shadow pagetable code with shadow2.")
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Acked-by: Roger Pau Monné <roger@xxxxxxxxxxxxxx>

> ---
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
> CC: Teddy Astie <teddy.astie@xxxxxxxxxx>
> 
> I'm not really sure about the fixes tag.  That's the oldest commit which bares
> any reseblence to the current code, and it was a bulk rewrite of the whole
> shadow pagetable code.  Prior to that, it was all mixed up and it's not
> completely obvious what's (definiely) walking the guest pagetables as opposed
> to the shadows.

I'm fine with no Fixes tag if there's no clear introduction point, or
if the introduction is simply that far away (ie: < 4.0) that it's
no really relevant anymore.

> Bloat-o-meter shows this clearly makes a code-gen difference in all cases:
> 
>   add/remove: 0/0 grow/shrink: 1/2 up/down: 16/-19 (-3)
>   Function                                     old     new   delta
>   guest_walk_tables_2_levels                  1688    1704     +16
>   guest_walk_tables_4_levels                  3708    3703      -5
>   guest_walk_tables_3_levels                  2233    2219     -14
> 
> To start with, l?e_read() looked to be the right helper, but they don't exist
> for guest pagetable types, leading to:
> 
> arch/x86/mm/guest_walk.c: In function ‘guest_walk_tables_2_levels’:
> ./arch/x86/include/asm/page.h:135:36: error: incompatible types when 
> assigning to type ‘guest_l2e_t’ from type ‘l2_pgentry_t’
>   135 | #define l2e_from_intpte(intpte)    ((l2_pgentry_t) { 
> (intpte_t)(intpte) })
>       |                                    ^
> ---
>  xen/arch/x86/mm/guest_walk.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> index f48c3ef75f48..df2ccaa67475 100644
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -129,7 +129,7 @@ guest_walk_tables(const struct vcpu *v, struct p2m_domain 
> *p2m,
>              guest_l4_table_offset(va) * sizeof(gw->l4e);
>      if ( !hvmemul_read_cache(v, l4gpa, &gw->l4e, sizeof(gw->l4e)) )
>      {
> -        gw->l4e = l4p[guest_l4_table_offset(va)];
> +        gw->l4e = (guest_l4e_t){ 
> ACCESS_ONCE(l4p[guest_l4_table_offset(va)].l4) };

I wouldn't mind if this was a macro or static inline function, maybe
that would prevent new usages from forgetting to use ACCESS_ONCE().

Thanks, Roger.