| Message ID | 20240828113044.35541-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] x86/dom0: disable SMAP for PV domain building only | expand |
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
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; }
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(-)