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

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

On Tue, Aug 27, 2024 at 02:07:07PM +0100, Andrew Cooper wrote:
> On 27/08/2024 2:04 pm, Jan Beulich wrote:
> > On 27.08.2024 14:59, Andrew Cooper wrote:
> >> On 27/08/2024 1:39 pm, Roger Pau Monne wrote:
> >>> --- 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?
> > Just to mention it: I'm fine with this in principle, as long as this won't
> > mean a pile of new goto-s in dom0_construct_pv(). If a new wrapper was
> > introduced (with the present function becoming static), I'd be okay.
> 
> I'd be happy with that too.
> 
> In fact, static helpers are probably best, seeing as we'll eventually
> need real #ifdefary around the cr4_pv32_mask accesses.

Do you mean a static helper in dom0_build.c for enabling/disabling
SMAP?

Because my plan would be to also add a wrapper for dom0_construct_pv()
so that I don't need to adjust the returns path in dom0_construct_pv()
itself.

Thanks, Roger.