Re: [PATCH] x86/boot: Further simplify CR4 handling in dom0_construct_pv()

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Sep 24, 2024 at 03:44:07PM +0200, Jan Beulich wrote:
> On 24.09.2024 13:23, Andrew Cooper wrote:
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -1057,29 +1057,31 @@ int __init dom0_construct_pv(struct domain *d,
> >                               module_t *initrd,
> >                               const char *cmdline)
> >  {
> > +    unsigned long cr4 = read_cr4();
> > +    unsigned long mask = X86_CR4_SMAP | X86_CR4_LASS;
> >      int rc;
> >  
> >      /*
> > -     * Clear SMAP in CR4 to allow user-accesses in construct_dom0().  This
> > -     * prevents us needing to write construct_dom0() in terms of
> > +     * Clear SMAP/LASS in CR4 to allow user-accesses in construct_dom0().
> > +     * This prevents us needing to write construct_dom0() in terms of
> >       * copy_{to,from}_user().
> >       */
> > -    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> > +    if ( cr4 & mask )
> >      {
> >          if ( IS_ENABLED(CONFIG_PV32) )
> > -            cr4_pv32_mask &= ~X86_CR4_SMAP;
> > +            cr4_pv32_mask &= ~mask;
> >  
> > -        write_cr4(read_cr4() & ~X86_CR4_SMAP);
> > +        write_cr4(cr4 & ~mask);
> >      }
> >  
> >      rc = dom0_construct(d, image, image_headroom, initrd, cmdline);
> >  
> > -    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> > +    if ( cr4 & mask )
> >      {
> > -        write_cr4(read_cr4() | X86_CR4_SMAP);
> > +        write_cr4(cr4);
> >  
> >          if ( IS_ENABLED(CONFIG_PV32) )
> > -            cr4_pv32_mask |= X86_CR4_SMAP;
> > +            cr4_pv32_mask |= mask;
> 
> You may end up setting a bit here which wasn't previously set, and which
> might then fault when cr4_pv32_restore tries to OR this into %cr4. Aiui
> you must have tested this on LASS-capable hardware, for it to have worked.

Possibly also needs X86_CR4_LASS adding to the XEN_CR4_PV32_BITS
define, as otherwise it won't end up in cr4_pv32_mask in the first
place AFAICT.

Thanks, Roger.