1. Patchew
  2. Xen
  3. View series

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

Roger Pau Monne posted 1 patch 2 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240827123949.24400-1-roger.pau@citrix.com
There is a newer version of this series
xen/arch/x86/dom0_build.c        | 17 +++++++++++++++++
xen/arch/x86/include/asm/setup.h |  2 ++
xen/arch/x86/setup.c             | 19 +------------------
3 files changed, 20 insertions(+), 18 deletions(-)
[PATCH v5] x86/dom0: disable SMAP for PV domain building only
Posted by Roger Pau Monne 2 months, 3 weeks ago 
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@citrix.com>
---
Changes since v4:
 - New approach, move the current logic so it's only applied when creating a PV
   dom0.
---
 xen/arch/x86/dom0_build.c        | 17 +++++++++++++++++
 xen/arch/x86/include/asm/setup.h |  2 ++
 xen/arch/x86/setup.c             | 19 +------------------
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 8d56705a0861..31c94b14bb06 100644
--- 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;
+        }
+    }
     else
         panic("Cannot construct Dom0. No guest interface available\n");
 
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index d75589178b91..8f7dfefb4dcf 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -64,6 +64,8 @@ extern bool opt_dom0_verbose;
 extern bool opt_dom0_cpuid_faulting;
 extern bool opt_dom0_msr_relaxed;
 
+extern unsigned long cr4_pv32_mask;
+
 #define max_init_domid (0)
 
 #endif
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index eee20bb1753c..eb0fcb6c8767 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -80,7 +80,7 @@ int8_t __initdata opt_probe_port_aliases = -1;
 boolean_param("probe-port-aliases", opt_probe_port_aliases);
 
 /* Only used in asm code and within this source file */
-unsigned long asmlinkage __read_mostly cr4_pv32_mask;
+unsigned long asmlinkage __ro_after_init cr4_pv32_mask;
 
 /* **** Linux config option: propagated to domain0. */
 /* "acpi=off":    Sisables both ACPI table parsing and interpreter. */
@@ -955,26 +955,9 @@ static struct domain *__init create_dom0(const module_t *image,
         }
     }
 
-    /*
-     * 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 ( cpu_has_smap )
-    {
-        cr4_pv32_mask &= ~X86_CR4_SMAP;
-        write_cr4(read_cr4() & ~X86_CR4_SMAP);
-    }
-
     if ( construct_dom0(d, image, headroom, initrd, cmdline) != 0 )
         panic("Could not construct domain 0\n");
 
-    if ( cpu_has_smap )
-    {
-        write_cr4(read_cr4() | X86_CR4_SMAP);
-        cr4_pv32_mask |= X86_CR4_SMAP;
-    }
-
     return d;
 }
 
-- 
2.46.0
Re: [PATCH v5] x86/dom0: disable SMAP for PV domain building only
Posted by Jan Beulich 2 months, 3 weeks ago 
On 27.08.2024 14:39, 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;
> +        }
> +    }

Andrew, looking at this code I really wonder what benefit you request to
move this further is going to have. Afaict no matter where we put it, it'll
be an #ifdef around each of the two accesses to cr4_pv32_mask when we make
that variable CONFIG_PV32-only.

> --- a/xen/arch/x86/include/asm/setup.h
> +++ b/xen/arch/x86/include/asm/setup.h
> @@ -64,6 +64,8 @@ extern bool opt_dom0_verbose;
>  extern bool opt_dom0_cpuid_faulting;
>  extern bool opt_dom0_msr_relaxed;
>  
> +extern unsigned long cr4_pv32_mask;

With this ...

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -80,7 +80,7 @@ int8_t __initdata opt_probe_port_aliases = -1;
>  boolean_param("probe-port-aliases", opt_probe_port_aliases);
>  
>  /* Only used in asm code and within this source file */
> -unsigned long asmlinkage __read_mostly cr4_pv32_mask;
> +unsigned long asmlinkage __ro_after_init cr4_pv32_mask;

... both the comment and the asmlinkage become stale now.

Jan
Re: [PATCH v5] x86/dom0: disable SMAP for PV domain building only
Posted by Andrew Cooper 2 months, 3 weeks ago 
On 28/08/2024 10:49 am, Jan Beulich wrote:
> On 27.08.2024 14:39, 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;
>> +        }
>> +    }
> Andrew, looking at this code I really wonder what benefit you request to
> move this further is going to have. Afaict no matter where we put it, it'll
> be an #ifdef around each of the two accesses to cr4_pv32_mask when we make
> that variable CONFIG_PV32-only.

This TU is common to PV and HVM, meaning that this logic inspecting
XEN_SMAP and playing with cr4 is included even in !PV builds.

Having it all in x86/pv/dom0_build.c means it will be dropped in a !PV
build.

~Andrew
Re: [PATCH v5] x86/dom0: disable SMAP for PV domain building only
Posted by Andrew Cooper 2 months, 3 weeks ago 
On 27/08/2024 1:39 pm, 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@citrix.com>
> ---
> Changes since v4:
>  - New approach, move the current logic so it's only applied when creating a PV
>    dom0.
> ---
>  xen/arch/x86/dom0_build.c        | 17 +++++++++++++++++
>  xen/arch/x86/include/asm/setup.h |  2 ++
>  xen/arch/x86/setup.c             | 19 +------------------
>  3 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 8d56705a0861..31c94b14bb06 100644
> --- 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?

That way, it won't need moving again to make cr4_pv32_mask exist only in
PV32 builds.  (This step is somewhat tricky, so I'm not suggesting doing
it in this patch.)

~Andrew
Re: [PATCH v5] x86/dom0: disable SMAP for PV domain building only
Posted by Jan Beulich 2 months, 3 weeks ago 
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.

Jan

> That way, it won't need moving again to make cr4_pv32_mask exist only in
> PV32 builds.  (This step is somewhat tricky, so I'm not suggesting doing
> it in this patch.)
> 
> ~Andrew
Re: [PATCH v5] x86/dom0: disable SMAP for PV domain building only
Posted by Roger Pau Monné 2 months, 3 weeks ago 
On Tue, Aug 27, 2024 at 03:04:54PM +0200, 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've considered adding this inside of dom0_construct_pv(), but then I
would need to adjust the return paths to re-enable SMAP.

I can add a wrapper, I didn't do it that way because it seemed
cumbersome IMO.

I will prepare v6 then with that approach.

Thanks, Roger.
Re: [PATCH v5] x86/dom0: disable SMAP for PV domain building only
Posted by Andrew Cooper 2 months, 3 weeks ago 
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.

~Andrew
Re: [PATCH v5] x86/dom0: disable SMAP for PV domain building only
Posted by Roger Pau Monné 2 months, 3 weeks ago 
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.
Re: [PATCH v5] x86/dom0: disable SMAP for PV domain building only
Posted by Jan Beulich 2 months, 3 weeks ago 
On 27.08.2024 15:51, Roger Pau Monné wrote:
> 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?

While that's how I understood Andrew's response, I'm not sure that'll buy
us very much. They'll both be used just once, and hence the amount of
#ifdef-ary is going to be the same as when the logic is kept in line.

Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.