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

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

[Andrew Cooper]

... as a shorthand for setting both suboptions at once.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

I think this wants backporting.  cet=0 is "so obviously" the way to turn off
both that I tried using it to debug a problem.  It's absence was an oversight
of the original CET logic.
---
 docs/misc/xen-command-line.pandoc |  4 +++-
 xen/arch/x86/setup.c              | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 1dc7e1ca0706..1720cb216824 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -271,7 +271,7 @@ enough. Setting this to a high value may cause boot failure, particularly if
 the NMI watchdog is also enabled.
 
 ### cet
-    = List of [ shstk=<bool>, ibt=<bool> ]
+    = List of [ <bool>, shstk=<bool>, ibt=<bool> ]
 
     Applicability: x86
 
@@ -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.
+
 *   The `shstk=` boolean controls whether Xen uses Shadow Stacks for its own
     protection.
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 53a73010e029..090abfd71754 100644
--- 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
+        }
+        else if ( (val = parse_boolean("shstk", s, ss)) >= 0 )
         {
 #ifdef CONFIG_XEN_SHSTK
             opt_xen_shstk = val;
-- 
2.11.0

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

[Roger Pau Monné]

On Thu, Apr 28, 2022 at 09:52:09AM +0100, Andrew Cooper wrote:
> ... as a shorthand for setting both suboptions at once.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

From the implementation below we would support settings like:

cet=true,shstk=false

Which I think it's indented?  Have a global default for all options,
set some to a different value.

Thanks, Roger.

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

[Andrew Cooper]

On 28/04/2022 10:19, Roger Pau Monné wrote:
> On Thu, Apr 28, 2022 at 09:52:09AM +0100, Andrew Cooper wrote:
>> ... as a shorthand for setting both suboptions at once.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>
> From the implementation below we would support settings like:
>
> cet=true,shstk=false
>
> Which I think it's indented?  Have a global default for all options,
> set some to a different value.

That's how all list options work, and it's also equivalent to

cet=true cet=shstk=false

Options can be given multiple time, and are parsed left to right with
the latest setting taking precedence.

~Andrew

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

[Jan Beulich]

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.

> +        }
> +        else if ( (val = parse_boolean("shstk", s, ss)) >= 0 )
>          {
>  #ifdef CONFIG_XEN_SHSTK
>              opt_xen_shstk = val;

Having seen Roger's reply, I'd like to make explicit that I don't
mind us allowing strange option combinations to be used, so long as
what we do matches the sequence in which they were provided.

Jan

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Andrew Cooper]

On 28/04/2022 11:13, 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".

I was going to say it was correct as is, but it turn out both are
acceptable.  "shorthand" is both a countable and uncountable quantity,
and both "sound" right.

~Andrew

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

[Andrew Cooper]

On 28/04/2022 11:13, Jan Beulich wrote:
>> --- 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.

It's this, or:

        if ( (val = parse_bool(s, ss)) >= 0 )
        {
#if !defined(CONFIG_XEN_SHSTK) && !defined(CONFIG_XEN_IBT)
            no_config_param("XEN_{SHSTK,IBT}", "cet", s, ss);
#endif
#ifdef CONFIG_XEN_SHSTK
            opt_xen_shstk = val;
#endif
#ifdef CONFIG_XEN_IBT
            opt_xen_ibt = val;
#endif
        }

I'm not terribly fussed.

~Andrew

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

[Jan Beulich]

On 29.04.2022 12:13, Andrew Cooper wrote:
> On 28/04/2022 11:13, Jan Beulich wrote:
>>> --- 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.
> 
> It's this, or:
> 
>         if ( (val = parse_bool(s, ss)) >= 0 )
>         {
> #if !defined(CONFIG_XEN_SHSTK) && !defined(CONFIG_XEN_IBT)
>             no_config_param("XEN_{SHSTK,IBT}", "cet", s, ss);
> #endif
> #ifdef CONFIG_XEN_SHSTK
>             opt_xen_shstk = val;
> #endif
> #ifdef CONFIG_XEN_IBT
>             opt_xen_ibt = val;
> #endif
>         }
> 
> I'm not terribly fussed.

I'd prefer the alternative variant; hopefully Roger doesn't strongly
prefer the other one. And then
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan