Re: [PATCH v3] x86/dom0: delay setting SMAP after dom0 build is done

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.08.2024 11:52, Roger Pau Monne wrote:
> @@ -1907,16 +1890,25 @@ void asmlinkage __init noreturn __start_xen(unsigned 
> long mbi_p)
>      if ( cpu_has_smep && opt_smep != SMEP_HVM_ONLY )
>          setup_force_cpu_cap(X86_FEATURE_XEN_SMEP);
>      if ( boot_cpu_has(X86_FEATURE_XEN_SMEP) )
> +    {
>          set_in_cr4(X86_CR4_SMEP);
> +        cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS;

Could be just "cr4_pv32_mask = X86_CR4_SMEP" now?

> +    }
>  
>      if ( !opt_smap )
>          setup_clear_cpu_cap(X86_FEATURE_SMAP);
>      if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY )
>          setup_force_cpu_cap(X86_FEATURE_XEN_SMAP);
>      if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> -        set_in_cr4(X86_CR4_SMAP);
> -
> -    cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS;
> +        /*
> +         * Set SMAP on the %cr4 mask so that it's set for APs on bringup, but
> +         * don't set for the BSP until domain building is done.
> +         *
> +         * Don't set it in cr4_pv32_mask either, until it's also set on the
> +         * BSP.  Otherwise the BUG in cr4_pv32_restore would trigger for 
> events
> +         * received on the BSP.
> +         */
> +        mmu_cr4_features |= X86_CR4_SMAP;

Don't you put APs at risk this way of triggering the BUG in cr4_pv32_restore()?
They'll have the bit set in %cr4, but the bit remains clear in cr4_pv32_mask
until much later.

> @@ -2048,6 +2040,18 @@ void asmlinkage __init noreturn __start_xen(unsigned 
> long mbi_p)
>      if ( !dom0 )
>          panic("Could not set up DOM0 guest OS\n");
>  
> +    /*
> +     * Enable SMAP only after being done with the domain building phase, as 
> the
> +     * PV builder switches to the domain page-tables and must be run with 
> SMAP
> +     * disabled.
> +     */
> +    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> +    {
> +        ASSERT(mmu_cr4_features & X86_CR4_SMAP);
> +        write_cr4(read_cr4() | X86_CR4_SMAP);
> +        cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS;
> +    }

Similarly for the BSP here: If we take an NMI between setting CR4.SMAP and
setting the bit in cr4_pv32_mask, cr4_pv32_restore() would hit the BUG
there if I'm not mistaken. I further fear that switching things around won't
help either. The code you remove from create_dom0() looks to have the same
issue. The only NMI-safe sequence looks to be: Clear both bits from %cr4,
update cr4_pv32_mask as wanted, and then write %cr4 with the bits from
cr4_pv32_mask ORed in.

Jan