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

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

On Wed, Aug 28, 2024 at 07:57:39PM +0100, Andrew Cooper wrote:
> On 28/08/2024 2:02 pm, Roger Pau Monné wrote:
> > On Wed, Aug 28, 2024 at 12:51:23PM +0100, Andrew Cooper wrote:
> >> On 28/08/2024 12:50 pm, Jan Beulich wrote:
> >>> On 28.08.2024 13:30, 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>
> >>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>> preferably with ...
> >>>
> >>>> @@ -1051,6 +1051,34 @@ out:
> >>>>      return rc;
> >>>>  }
> >>>>  
> >>>> +int __init dom0_construct_pv(struct domain *d,
> >>>> +                             const module_t *image,
> >>>> +                             unsigned long image_headroom,
> >>>> +                             module_t *initrd,
> >>>> +                             const char *cmdline)
> >>>> +{
> >>>> +    int rc;
> >>>> +
> >>>> +    /*
> >>>> +     * Temporarily clear SMAP in CR4 to allow user-accesses in
> >>>> +     * construct_dom0().  This saves a large number of corner cases
> >>> ... the final 's' dropped here and ...
> >>>
> >>>> +     * interactions with copy_from_user().
> 
> 
> Actually, even with this adjustment the comment is still wonky.
> 
> The point is that we're clearing SMAP so we *don't* need to rewrite
> construct_dom0() in terms of copy_{to,from}_user().
> 
> I've adjusted it.

It did seem weird to me, I've assumed the wording was meaning to imply
that SMAP was disabled so that construct_dom0() didn't need to use
copy_from_user().

The comment you added seems fine to me, however there's a typo I
think:

    /*
     * Clear SMAP in CR4 to allow user-accesses in construct_dom0().  This
     * prevents us needing to write rewrite construct_dom0() in terms of
                              ^ extra?
     * copy_{to,from}_user().
     */

We can fix at some later point when modifying this.

Thanks, Roger.