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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2022 10:10, Roger Pau Monné wrote:
> 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.

But the positive options aren't really the interesting ones here, as
things are enabled by default anyway. I would expect "cet=0" to be
silent unless neither CONFIG_* is enabled in the build - it simply
means "disable whatever CET support there is".

I can agree that "cet=1" may indeed be useful to warn, though, but I
wonder whether the logic here then wouldn't become unduly complicated.

> 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.

While fundamentally I agree, too verbose output can also raise
unnecessary questions or induce unnecessary investigations.

Jan