xen/arch/x86/include/asm/setup.h | 2 ++ xen/arch/x86/pv/dom0_build.c | 38 +++++++++++++++++++++++++++----- xen/arch/x86/setup.c | 20 +---------------- 3 files changed, 36 insertions(+), 24 deletions(-)
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 v5:
- Drop comment and asmlinkage attribute.
- Introduce a wrapper for dom0_construct_pv() so the usage of cr4_pv32_mask
can be limited to a PV-specific file.
Changes since v4:
- New approach, move the current logic so it's only applied when creating a PV
dom0.
---
xen/arch/x86/include/asm/setup.h | 2 ++
xen/arch/x86/pv/dom0_build.c | 38 +++++++++++++++++++++++++++-----
xen/arch/x86/setup.c | 20 +----------------
3 files changed, 36 insertions(+), 24 deletions(-)
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/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 57e58a02e707..22988f2660b0 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -354,11 +354,11 @@ static struct page_info * __init alloc_chunk(struct domain *d,
return page;
}
-int __init dom0_construct_pv(struct domain *d,
- const module_t *image,
- unsigned long image_headroom,
- module_t *initrd,
- const char *cmdline)
+static int __init dom0_construct(struct domain *d,
+ const module_t *image,
+ unsigned long image_headroom,
+ module_t *initrd,
+ const char *cmdline)
{
int i, rc, order, machine;
bool compatible, compat;
@@ -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
+ * 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(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;
+ }
+
+ return rc;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index eee20bb1753c..f1076c72032d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -79,8 +79,7 @@ bool __read_mostly use_invpcid;
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 __ro_after_init cr4_pv32_mask;
/* **** Linux config option: propagated to domain0. */
/* "acpi=off": Sisables both ACPI table parsing and interpreter. */
@@ -955,26 +954,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
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@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
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().
> + */
> + if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> + {
> + cr4_pv32_mask &= ~X86_CR4_SMAP;
> + write_cr4(read_cr4() & ~X86_CR4_SMAP);
> + }
> + rc = dom0_construct(d, image, image_headroom, initrd, cmdline);
> + if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
... blank lines added around the function call. Happy to adjust while
committing, so long as you agree.
Jan
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@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 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().
>> + */
>> + if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
>> + {
>> + cr4_pv32_mask &= ~X86_CR4_SMAP;
>> + write_cr4(read_cr4() & ~X86_CR4_SMAP);
>> + }
>> + rc = dom0_construct(d, image, image_headroom, initrd, cmdline);
>> + if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> ... blank lines added around the function call. Happy to adjust while
> committing, so long as you agree.
+1 to both suggestions.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
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@citrix.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > 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().
> >> + */
> >> + if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> >> + {
> >> + cr4_pv32_mask &= ~X86_CR4_SMAP;
> >> + write_cr4(read_cr4() & ~X86_CR4_SMAP);
> >> + }
> >> + rc = dom0_construct(d, image, image_headroom, initrd, cmdline);
> >> + if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> > ... blank lines added around the function call. Happy to adjust while
> > committing, so long as you agree.
>
> +1 to both suggestions.
Sure, please adjust at commit.
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks (to both).
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@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> 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.
~Andrew
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@citrix.com>
> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>> 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.
On 29/08/2024 8:31 am, Roger Pau Monné wrote:
> 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@citrix.com>
>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>> 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.
Urgh, sorry.
Luckily, I've got just the patch to do this in...
~Andrew
© 2016 - 2024 Red Hat, Inc.