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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 31, 2024 at 08:44:46AM +0200, Jan Beulich wrote:
> 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.

On approach I didn't test would be to add ASSERTs in stac/clac
functions to ensure that the state is as intended.  IOW: for stac we
would assert that the AC flag is not set, while for clac we would do
the opposite and assert that it's set before clearing it.

That should detect nesting.

> 
> > 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?

No, I think the current code is correct, it was my change that was
incorrect.

> > --- 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).

Hm, yes, would be better to reword that.

Thanks, Roger.