Re: [PATCH] x86/cet: Support cet=<bool> on the command line

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Apr 28, 2022 at 12:13:31PM +0200, Jan Beulich wrote:
> On 28.04.2022 10:52, Andrew Cooper wrote:
> > @@ -283,6 +283,8 @@ CET is incompatible with 32bit PV guests.  If any CET 
> > sub-options are active,
> >  they will override the `pv=32` boolean to `false`.  Backwards compatibility
> >  can be maintained with the pv-shim mechanism.
> >  
> > +*   An unqualified boolean is shorthand for setting all suboptions at once.
> 
> You're the native speaker, but I wonder whether there an "a" missing
> before "shorthand".
> 
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -117,7 +117,20 @@ static int __init cf_check parse_cet(const char *s)
> >          if ( !ss )
> >              ss = strchr(s, '\0');
> >  
> > -        if ( (val = parse_boolean("shstk", s, ss)) >= 0 )
> > +        if ( (val = parse_bool(s, ss)) >= 0 )
> > +        {
> > +#ifdef CONFIG_XEN_SHSTK
> > +            opt_xen_shstk = val;
> > +#else
> > +            no_config_param("XEN_SHSTK", "cet", s, ss);
> > +#endif
> > +#ifdef CONFIG_XEN_IBT
> > +            opt_xen_ibt = val;
> > +#else
> > +            no_config_param("XEN_IBT", "cet", s, ss);
> > +#endif
> 
> There shouldn't be two invocations of no_config_param() here; imo if
> either CONFIG_* is defined, use of the option shouldn't produce any
> warning at all.

Hm, I think we would want to warn if someone sets cet=1 but some of
the options have not been built in?  Or else a not very conscious
administrator might believe that all CET options are enabled when some
might not be present in the build.  This would also assume that all
options are positive.

IMO the current approach doesn't seem bad to me, I think it's always
better to error on the side of printing too verbose information rather
than omitting it, specially when it's related to user input on the
command line and security sensitive options.

Thanks, Roger.