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