| Message ID | 20240223120616.73503-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/spec: improve command line parsing | expand |
On 23.02.2024 13:06, Roger Pau Monne wrote: > The current logic to handle the BRANCH_HARDEN option will report it as enabled > even when build-time disabled. Fix this by only allowing the option to be set > when support for it is built into Xen. > > Fixes: 2d6f36daa086 ('x86/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v1: > - Use no_config_param(). > --- > xen/arch/x86/spec_ctrl.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c > index 421fe3f640df..4ce8a7a0b8ef 100644 > --- a/xen/arch/x86/spec_ctrl.c > +++ b/xen/arch/x86/spec_ctrl.c > @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; > int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; > int8_t __read_mostly opt_eager_fpu = -1; > int8_t __read_mostly opt_l1d_flush = -1; > -static bool __initdata opt_branch_harden = true; > +static bool __initdata opt_branch_harden = > + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); > > bool __initdata bsp_delay_spec_ctrl; > uint8_t __read_mostly default_xen_spec_ctrl; > @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) > else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > opt_l1d_flush = val; > else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > + { > +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH > opt_branch_harden = val; > +#else > + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); > + rc = -EINVAL; > +#endif > + } If you use #ifdef (rather than IS_ENABLED()) here, the variable probably shouldn't exist at all when the Kconfig option is off (albeit yet, another #ifdef would then be needed higher up in the function). Question is - why don't you use IS_ENABLED() here as well? Jan
On 23.02.2024 13:06, Roger Pau Monne wrote: > --- a/xen/arch/x86/spec_ctrl.c > +++ b/xen/arch/x86/spec_ctrl.c > @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; > int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; > int8_t __read_mostly opt_eager_fpu = -1; > int8_t __read_mostly opt_l1d_flush = -1; > -static bool __initdata opt_branch_harden = true; > +static bool __initdata opt_branch_harden = > + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); > > bool __initdata bsp_delay_spec_ctrl; > uint8_t __read_mostly default_xen_spec_ctrl; > @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) > else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > opt_l1d_flush = val; > else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > + { > +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH > opt_branch_harden = val; > +#else > + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); > + rc = -EINVAL; > +#endif > + } > else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) > opt_srb_lock = val; > else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) While looking at patch 3 I noticed another inconsistency: Wouldn't "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just like for thunks both CONFIG_* state and actual runtime choice are logged? Or alternatively, should logging of thunk runtime choice be suppressed when the Kconfig setting is off? Jan
On Mon, Feb 26, 2024 at 09:42:58AM +0100, Jan Beulich wrote: > On 23.02.2024 13:06, Roger Pau Monne wrote: > > --- a/xen/arch/x86/spec_ctrl.c > > +++ b/xen/arch/x86/spec_ctrl.c > > @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; > > int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; > > int8_t __read_mostly opt_eager_fpu = -1; > > int8_t __read_mostly opt_l1d_flush = -1; > > -static bool __initdata opt_branch_harden = true; > > +static bool __initdata opt_branch_harden = > > + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); > > > > bool __initdata bsp_delay_spec_ctrl; > > uint8_t __read_mostly default_xen_spec_ctrl; > > @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) > > else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > > opt_l1d_flush = val; > > else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > > + { > > +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH > > opt_branch_harden = val; > > +#else > > + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); > > + rc = -EINVAL; > > +#endif > > + } > > else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) > > opt_srb_lock = val; > > else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) > > While looking at patch 3 I noticed another inconsistency: Wouldn't > "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just > like for thunks both CONFIG_* state and actual runtime choice are > logged? Yes, I guess we would also need to expand "Compiled-in support:" to include HARDEN_ARRAY and HARDEN_GUEST_ACCESS. > Or alternatively, should logging of thunk runtime choice be > suppressed when the Kconfig setting is off? Hm, I think printing "BTI-Thunk N/A" is good enough when the thunk has been built time disabled. Thanks, Roger.
On 26.02.2024 10:09, Roger Pau Monné wrote: > On Mon, Feb 26, 2024 at 09:42:58AM +0100, Jan Beulich wrote: >> On 23.02.2024 13:06, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/spec_ctrl.c >>> +++ b/xen/arch/x86/spec_ctrl.c >>> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; >>> int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; >>> int8_t __read_mostly opt_eager_fpu = -1; >>> int8_t __read_mostly opt_l1d_flush = -1; >>> -static bool __initdata opt_branch_harden = true; >>> +static bool __initdata opt_branch_harden = >>> + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); >>> >>> bool __initdata bsp_delay_spec_ctrl; >>> uint8_t __read_mostly default_xen_spec_ctrl; >>> @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) >>> else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) >>> opt_l1d_flush = val; >>> else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) >>> + { >>> +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH >>> opt_branch_harden = val; >>> +#else >>> + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); >>> + rc = -EINVAL; >>> +#endif >>> + } >>> else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) >>> opt_srb_lock = val; >>> else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) >> >> While looking at patch 3 I noticed another inconsistency: Wouldn't >> "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just >> like for thunks both CONFIG_* state and actual runtime choice are >> logged? > > Yes, I guess we would also need to expand "Compiled-in support:" to > include HARDEN_ARRAY and HARDEN_GUEST_ACCESS. > >> Or alternatively, should logging of thunk runtime choice be >> suppressed when the Kconfig setting is off? > > Hm, I think printing "BTI-Thunk N/A" is good enough when the thunk has > been built time disabled. Good enough - yes. But redundant with the other log line. And since all of this output is getting longer and longer anyway, omitting whatever can sensibly be omitted might be at least worth considering. Jan
On Mon, Feb 26, 2024 at 10:33:14AM +0100, Jan Beulich wrote: > On 26.02.2024 10:09, Roger Pau Monné wrote: > > On Mon, Feb 26, 2024 at 09:42:58AM +0100, Jan Beulich wrote: > >> On 23.02.2024 13:06, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/spec_ctrl.c > >>> +++ b/xen/arch/x86/spec_ctrl.c > >>> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; > >>> int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; > >>> int8_t __read_mostly opt_eager_fpu = -1; > >>> int8_t __read_mostly opt_l1d_flush = -1; > >>> -static bool __initdata opt_branch_harden = true; > >>> +static bool __initdata opt_branch_harden = > >>> + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); > >>> > >>> bool __initdata bsp_delay_spec_ctrl; > >>> uint8_t __read_mostly default_xen_spec_ctrl; > >>> @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) > >>> else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > >>> opt_l1d_flush = val; > >>> else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > >>> + { > >>> +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH > >>> opt_branch_harden = val; > >>> +#else > >>> + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); > >>> + rc = -EINVAL; > >>> +#endif > >>> + } > >>> else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) > >>> opt_srb_lock = val; > >>> else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) > >> > >> While looking at patch 3 I noticed another inconsistency: Wouldn't > >> "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just > >> like for thunks both CONFIG_* state and actual runtime choice are > >> logged? > > > > Yes, I guess we would also need to expand "Compiled-in support:" to > > include HARDEN_ARRAY and HARDEN_GUEST_ACCESS. > > > >> Or alternatively, should logging of thunk runtime choice be > >> suppressed when the Kconfig setting is off? > > > > Hm, I think printing "BTI-Thunk N/A" is good enough when the thunk has > > been built time disabled. > > Good enough - yes. But redundant with the other log line. And since all > of this output is getting longer and longer anyway, omitting whatever > can sensibly be omitted might be at least worth considering. I can add yet another patch to remove that when not built-in, this cleanup is turning into a never-ending exercise I'm afraid. Thanks, Roger.
On 26.02.2024 10:57, Roger Pau Monné wrote: > On Mon, Feb 26, 2024 at 10:33:14AM +0100, Jan Beulich wrote: >> On 26.02.2024 10:09, Roger Pau Monné wrote: >>> On Mon, Feb 26, 2024 at 09:42:58AM +0100, Jan Beulich wrote: >>>> On 23.02.2024 13:06, Roger Pau Monne wrote: >>>>> --- a/xen/arch/x86/spec_ctrl.c >>>>> +++ b/xen/arch/x86/spec_ctrl.c >>>>> @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; >>>>> int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; >>>>> int8_t __read_mostly opt_eager_fpu = -1; >>>>> int8_t __read_mostly opt_l1d_flush = -1; >>>>> -static bool __initdata opt_branch_harden = true; >>>>> +static bool __initdata opt_branch_harden = >>>>> + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); >>>>> >>>>> bool __initdata bsp_delay_spec_ctrl; >>>>> uint8_t __read_mostly default_xen_spec_ctrl; >>>>> @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) >>>>> else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) >>>>> opt_l1d_flush = val; >>>>> else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) >>>>> + { >>>>> +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH >>>>> opt_branch_harden = val; >>>>> +#else >>>>> + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); >>>>> + rc = -EINVAL; >>>>> +#endif >>>>> + } >>>>> else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) >>>>> opt_srb_lock = val; >>>>> else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 ) >>>> >>>> While looking at patch 3 I noticed another inconsistency: Wouldn't >>>> "Compiled-in support:" better also enumerate HARDEN_BRANCH then, just >>>> like for thunks both CONFIG_* state and actual runtime choice are >>>> logged? >>> >>> Yes, I guess we would also need to expand "Compiled-in support:" to >>> include HARDEN_ARRAY and HARDEN_GUEST_ACCESS. >>> >>>> Or alternatively, should logging of thunk runtime choice be >>>> suppressed when the Kconfig setting is off? >>> >>> Hm, I think printing "BTI-Thunk N/A" is good enough when the thunk has >>> been built time disabled. >> >> Good enough - yes. But redundant with the other log line. And since all >> of this output is getting longer and longer anyway, omitting whatever >> can sensibly be omitted might be at least worth considering. > > I can add yet another patch to remove that when not built-in, this > cleanup is turning into a never-ending exercise I'm afraid. Just to be clear: I'm not meaning to insist that you also clean up the thunk part. But I still wanted to mention such as I noticed it. Jan
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 421fe3f640df..4ce8a7a0b8ef 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -50,7 +50,8 @@ static int8_t __initdata opt_psfd = -1; int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; int8_t __read_mostly opt_eager_fpu = -1; int8_t __read_mostly opt_l1d_flush = -1; -static bool __initdata opt_branch_harden = true; +static bool __initdata opt_branch_harden = + IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH); bool __initdata bsp_delay_spec_ctrl; uint8_t __read_mostly default_xen_spec_ctrl; @@ -268,7 +269,14 @@ static int __init cf_check parse_spec_ctrl(const char *s) else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) opt_l1d_flush = val; else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) + { +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH opt_branch_harden = val; +#else + no_config_param("SPECULATIVE_HARDEN_BRANCH", "spec-ctrl", s, ss); + rc = -EINVAL; +#endif + } else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) opt_srb_lock = val; else if ( (val = parse_boolean("unpriv-mmio", s, ss)) >= 0 )
The current logic to handle the BRANCH_HARDEN option will report it as enabled even when build-time disabled. Fix this by only allowing the option to be set when support for it is built into Xen. Fixes: 2d6f36daa086 ('x86/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Use no_config_param(). --- xen/arch/x86/spec_ctrl.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)