Re: [PATCH v5] x86/dom0: disable SMAP for PV domain building only

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/08/2024 1:39 pm, Roger Pau Monne wrote:
> Move the logic that disables SMAP so it's only performed when building a PV
> dom0, PVH dom0 builder doesn't require disabling SMAP.
>
> The fixes tag is to account for the wrong usage of cpu_has_smap in
> create_dom0(), it should instead have used
> boot_cpu_has(X86_FEATURE_XEN_SMAP).  Fix while moving the logic to apply to PV
> only.
>
> While there also make cr4_pv32_mask __ro_after_init.
>
> Fixes: 493ab190e5b1 ('xen/sm{e, a}p: allow disabling sm{e, a}p for Xen 
> itself')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v4:
>  - New approach, move the current logic so it's only applied when creating a 
> PV
>    dom0.
> ---
>  xen/arch/x86/dom0_build.c        | 17 +++++++++++++++++
>  xen/arch/x86/include/asm/setup.h |  2 ++
>  xen/arch/x86/setup.c             | 19 +------------------
>  3 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 8d56705a0861..31c94b14bb06 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -612,7 +612,24 @@ int __init construct_dom0(struct domain *d, const 
> module_t *image,
>      if ( is_hvm_domain(d) )
>          rc = dom0_construct_pvh(d, image, image_headroom, initrd, cmdline);
>      else if ( is_pv_domain(d) )
> +    {
> +        /*
> +         * Temporarily clear SMAP in CR4 to allow user-accesses in
> +         * construct_dom0().  This saves a large number of corner cases
> +         * interactions with copy_from_user().
> +         */
> +        if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> +        {
> +            cr4_pv32_mask &= ~X86_CR4_SMAP;
> +            write_cr4(read_cr4() & ~X86_CR4_SMAP);
> +        }
>          rc = dom0_construct_pv(d, image, image_headroom, initrd, cmdline);
> +        if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> +        {
> +            write_cr4(read_cr4() | X86_CR4_SMAP);
> +            cr4_pv32_mask |= X86_CR4_SMAP;
> +        }
> +    }

I hate to drag this on further still, but can this logic be move it into
dom0_construct_pv() itself, rather than here?

That way, it won't need moving again to make cr4_pv32_mask exist only in
PV32 builds.  (This step is somewhat tricky, so I'm not suggesting doing
it in this patch.)

~Andrew