| Message ID | 20240223094215.71889-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/spec: fix BRANCH_HARDEN option to only be set when build-enabled | expand |
On 23/02/2024 9:42 am, 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> > --- > xen/arch/x86/spec_ctrl.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c > index 421fe3f640df..e634c6b559b4 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; > @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char *s) > opt_eager_fpu = val; > else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > opt_l1d_flush = val; > - else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > + else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) && > + (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > opt_branch_harden = val; Yeah, we should definitely fix this, but could we use no_config_param() here for the compiled-out case ? See cet= for an example. If we're going to ignore what the user asks, we should tell them why. And given this as an example, shouldn't we do the same with CONFIG_INDIRECT_THUNK and bti=thunk= too ? ~Andrew
On Fri, Feb 23, 2024 at 09:46:27AM +0000, Andrew Cooper wrote: > On 23/02/2024 9:42 am, 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> > > --- > > xen/arch/x86/spec_ctrl.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c > > index 421fe3f640df..e634c6b559b4 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; > > @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char *s) > > opt_eager_fpu = val; > > else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > > opt_l1d_flush = val; > > - else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > > + else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) && > > + (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > > opt_branch_harden = val; > > Yeah, we should definitely fix this, but could we use no_config_param() > here for the compiled-out case ? Oh, didn't know that helper existed. > See cet= for an example. If we're going to ignore what the user asks, > we should tell them why. > > And given this as an example, shouldn't we do the same with > CONFIG_INDIRECT_THUNK and bti=thunk= too ? Checked for SPECULATIVE_HARDEN_ARRAY and SPECULATIVE_HARDEN_GUEST_ACCESS, but not the thunk, will add it as a followup patch. Thanks, Roger.
On Fri, Feb 23, 2024 at 09:46:27AM +0000, Andrew Cooper wrote: > On 23/02/2024 9:42 am, 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> > > --- > > xen/arch/x86/spec_ctrl.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c > > index 421fe3f640df..e634c6b559b4 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; > > @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char *s) > > opt_eager_fpu = val; > > else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > > opt_l1d_flush = val; > > - else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > > + else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) && > > + (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > > opt_branch_harden = val; > > Yeah, we should definitely fix this, but could we use no_config_param() > here for the compiled-out case ? > > See cet= for an example. If we're going to ignore what the user asks, > we should tell them why. Maybe I'm missing something: I've looked into using no_config_param(), but there's no difference really, because cmdline_parse() is called before the console is initialized, so those messages seem to be lost. Should this go into some kind of buffer which is then printed by __start_xen() once the console has been initialized? (just after printing cmdline itself). Thanks, Roger.
On 23/02/2024 10:17 am, Roger Pau Monné wrote: > On Fri, Feb 23, 2024 at 09:46:27AM +0000, Andrew Cooper wrote: >> On 23/02/2024 9:42 am, 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> >>> --- >>> xen/arch/x86/spec_ctrl.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c >>> index 421fe3f640df..e634c6b559b4 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; >>> @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char *s) >>> opt_eager_fpu = val; >>> else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) >>> opt_l1d_flush = val; >>> - else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) >>> + else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) && >>> + (val = parse_boolean("branch-harden", s, ss)) >= 0 ) >>> opt_branch_harden = val; >> Yeah, we should definitely fix this, but could we use no_config_param() >> here for the compiled-out case ? >> >> See cet= for an example. If we're going to ignore what the user asks, >> we should tell them why. > Maybe I'm missing something: I've looked into using no_config_param(), > but there's no difference really, because cmdline_parse() is called > before the console is initialized, so those messages seem to be > lost. Look at `xl dmesg` rather than the console. They also do appear on vga in some configurations. There's a separate todo to get these out in a slightly nicer way, but they at least exist in logs. ~Andrew
On Fri, Feb 23, 2024 at 10:26:15AM +0000, Andrew Cooper wrote: > On 23/02/2024 10:17 am, Roger Pau Monné wrote: > > On Fri, Feb 23, 2024 at 09:46:27AM +0000, Andrew Cooper wrote: > >> On 23/02/2024 9:42 am, 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> > >>> --- > >>> xen/arch/x86/spec_ctrl.c | 6 ++++-- > >>> 1 file changed, 4 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c > >>> index 421fe3f640df..e634c6b559b4 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; > >>> @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char *s) > >>> opt_eager_fpu = val; > >>> else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) > >>> opt_l1d_flush = val; > >>> - else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > >>> + else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) && > >>> + (val = parse_boolean("branch-harden", s, ss)) >= 0 ) > >>> opt_branch_harden = val; > >> Yeah, we should definitely fix this, but could we use no_config_param() > >> here for the compiled-out case ? > >> > >> See cet= for an example. If we're going to ignore what the user asks, > >> we should tell them why. > > Maybe I'm missing something: I've looked into using no_config_param(), > > but there's no difference really, because cmdline_parse() is called > > before the console is initialized, so those messages seem to be > > lost. > > Look at `xl dmesg` rather than the console. They also do appear on vga > in some configurations. Oh, my internal buffer was too small on those also got truncated, had to bump it. > There's a separate todo to get these out in a slightly nicer way, but > they at least exist in logs. I've created: https://gitlab.com/xen-project/xen/-/issues/184 Thanks, Roger.
On 23/02/2024 11:11 am, Roger Pau Monné wrote: > On Fri, Feb 23, 2024 at 10:26:15AM +0000, Andrew Cooper wrote: >> On 23/02/2024 10:17 am, Roger Pau Monné wrote: >>> On Fri, Feb 23, 2024 at 09:46:27AM +0000, Andrew Cooper wrote: >>>> On 23/02/2024 9:42 am, 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> >>>>> --- >>>>> xen/arch/x86/spec_ctrl.c | 6 ++++-- >>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c >>>>> index 421fe3f640df..e634c6b559b4 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; >>>>> @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char *s) >>>>> opt_eager_fpu = val; >>>>> else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) >>>>> opt_l1d_flush = val; >>>>> - else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) >>>>> + else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) && >>>>> + (val = parse_boolean("branch-harden", s, ss)) >= 0 ) >>>>> opt_branch_harden = val; >>>> Yeah, we should definitely fix this, but could we use no_config_param() >>>> here for the compiled-out case ? >>>> >>>> See cet= for an example. If we're going to ignore what the user asks, >>>> we should tell them why. >>> Maybe I'm missing something: I've looked into using no_config_param(), >>> but there's no difference really, because cmdline_parse() is called >>> before the console is initialized, so those messages seem to be >>> lost. >> Look at `xl dmesg` rather than the console. They also do appear on vga >> in some configurations. > Oh, my internal buffer was too small on those also got truncated, had > to bump it. Yeah - the default buffer size in Xen is too small. It has been for more than a decade, which is how long the adjustment in XenServer has been around. > >> There's a separate todo to get these out in a slightly nicer way, but >> they at least exist in logs. > I've created: > > https://gitlab.com/xen-project/xen/-/issues/184 Thanks. ~Andrew
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 421fe3f640df..e634c6b559b4 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; @@ -267,7 +268,8 @@ static int __init cf_check parse_spec_ctrl(const char *s) opt_eager_fpu = val; else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 ) opt_l1d_flush = val; - else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 ) + else if ( IS_ENABLED(CONFIG_SPECULATIVE_HARDEN_BRANCH) && + (val = parse_boolean("branch-harden", s, ss)) >= 0 ) opt_branch_harden = val; else if ( (val = parse_boolean("srb-lock", s, ss)) >= 0 ) opt_srb_lock = val;
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> --- xen/arch/x86/spec_ctrl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)