Re: [PATCH v2 2/2] x86/dom0: only disable SMAP for the PV dom0 build

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2024 17:28, Roger Pau Monne wrote:
> The PVH dom0 builder doesn't switch page tables and has no need to run with
> SMAP disabled.
> 
> Use stac() and clac(), as that's safe to use even if events would hit in the
> middle of the region with SMAP disabled.  Nesting stac() and clac() calls is
> not safe, so the stac() call is done strictly after elf_load_binary() because
> that uses raw_{copy_to,clear}_guest() accessors which toggle SMAP.

And that was the main concern causing the CR4.SMAP override to be used instead,
iirc. While I'm sure you've properly audited all code paths, how can we be sure
there's not going to be another stac() or clac() added somewhere? Or setting of
EFLAGS as a whole, clearing EFLAGS.AC without that being explicit? I think we'd
be better off sticking to the fiddling with CR4.

> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Considering the bug Andrew pointed out on the code you remove from setup.c,
don't we want a Fixes: tag as well?

> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -830,6 +830,15 @@ int __init dom0_construct_pv(struct domain *d,
>          printk("Failed to load the kernel binary\n");
>          goto out;
>      }
> +
> +    /*
> +     * Disable SMAP to allow user-accesses when running on dom0 page-tables.
> +     * Note this must be done after elf_load_binary(), as such helper uses
> +     * raw_{copy_to,clear}_guest() helpers which internally call 
> stac()/clac()
> +     * and those calls would otherwise nest with the ones here.

Just in case you and Andrew would outvote me on which approach to take:
I'm okay with "helpers" here, but the earlier "such helper" reads a little
odd to me. Imo using "that" or "it" instead would be better. Not the least
because personally a function like elf_load_binary() goes beyond what I'd
call a mere "helper" (in that case dom0_construct_pv() toos could be deemed
a helper, etc).

Jan