[PATCH] xsm: also panic upon "flask=enforcing" when XSM_FLASK=n

[PATCH] xsm: also panic upon "flask=enforcing" when XSM_FLASK=n

[Jan Beulich]

While the behavior to ignore this option without FLASK support was
properly documented, it is still somewhat surprising to someone using
this option and then still _not_ getting the assumed security. Add a
2nd handler for the command line option for the XSM_FLASK=n case, and
invoke panic() when the option is specified (and not subsequently
overridden by "flask=disabled").

Suggested-by: Ian Jackson <ian.jackson@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -998,8 +998,9 @@ to use the default.
 > Default: `enforcing`
 
 Specify how the FLASK security server should be configured.  This option is only
-available if the hypervisor was compiled with FLASK support.  This can be
-enabled by running either:
+available if the hypervisor was compiled with FLASK support, except that
+`flask=enforcing` will still keep the hypervisor from successfully booting even
+without FLASK support.  FLASK support can be enabled by running either:
 - make -C xen config and enabling XSM and FLASK.
 - make -C xen menuconfig and enabling 'FLux Advanced Security Kernel support' and 'Xen Security Modules support'
 
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -211,7 +211,33 @@ int __init register_xsm(struct xsm_opera
     return 0;
 }
 
-#endif
+#endif /* CONFIG_XSM */
+
+#ifndef CONFIG_XSM_FLASK
+static bool __initdata _flask_enforcing;
+
+static int __init parse_flask_param(const char *s)
+{
+    if ( !strcmp(s, "enforcing") )
+        _flask_enforcing = true;
+    else if ( !strcmp(s, "disabled") )
+        _flask_enforcing = false;
+    else
+        return -EINVAL;
+
+    return 0;
+}
+custom_param("flask", parse_flask_param);
+
+static int __init check_flask_enforcing(void)
+{
+    if ( _flask_enforcing )
+        panic("\"flask=enforcing\" specified without FLASK support\n");
+
+    return 0;
+}
+__initcall(check_flask_enforcing);
+#endif /* !CONFIG_XSM_FLASK */
 
 long do_xsm_op (XEN_GUEST_HANDLE_PARAM(xsm_op_t) op)
 {

Re: [PATCH] xsm: also panic upon "flask=enforcing" when XSM_FLASK=n

[Andrew Cooper]

On 29/05/2020 10:34, Jan Beulich wrote:
> While the behavior to ignore this option without FLASK support was
> properly documented, it is still somewhat surprising to someone using
> this option and then still _not_ getting the assumed security. Add a
> 2nd handler for the command line option for the XSM_FLASK=n case, and
> invoke panic() when the option is specified (and not subsequently
> overridden by "flask=disabled").
>
> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'm very tempted to nack this outright, lest I remind both of you of the
total disaster that was XSA-9, and the subsequent retraction of the code
which did exactly this.

If you want to do something like this, prohibit creating guests so the
administrator can still log in and unbreak, instead of having it
entering a reboot loop with no output.  The console isn't established
this early, so none of this text makes it out onto VGA/serial.

~Andrew

Re: [PATCH] xsm: also panic upon "flask=enforcing" when XSM_FLASK=n

[Jan Beulich]

On 29.05.2020 12:07, Andrew Cooper wrote:
> On 29/05/2020 10:34, Jan Beulich wrote:
>> While the behavior to ignore this option without FLASK support was
>> properly documented, it is still somewhat surprising to someone using
>> this option and then still _not_ getting the assumed security. Add a
>> 2nd handler for the command line option for the XSM_FLASK=n case, and
>> invoke panic() when the option is specified (and not subsequently
>> overridden by "flask=disabled").
>>
>> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm very tempted to nack this outright, lest I remind both of you of the
> total disaster that was XSA-9, and the subsequent retraction of the code
> which did exactly this.
> 
> If you want to do something like this, prohibit creating guests so the
> administrator can still log in and unbreak,

Unbreaking is as easy as removing the command line option, or
adding "flask=disable" at the end of the command line.

Preventing to create guests is another option, but complicated
by the late-hwdom feature we still have - to achieve what you
want we'd have to permit creating that one further domain.
Dom0less perhaps also would need special treatment (and there
I'm not sure we'd know which of the domains we are supposed to
allow being created, and which one(s) not).

> instead of having it
> entering a reboot loop with no output.  The console isn't established
> this early, so none of this text makes it out onto VGA/serial.

You didn't look at the patch then: I'm intentionally _not_
panic()-ing from the command line parsing function, but from
an initcall. Both VGA and serial have been set up by that time.
(I was in fact considering to pull it a little earlier, into
a pre-SMP initcall.)

Jan

Re: [PATCH] xsm: also panic upon "flask=enforcing" when XSM_FLASK=n

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH] xsm: also panic upon "flask=enforcing" when XSM_FLASK=n"):
> On 29/05/2020 10:34, Jan Beulich wrote:
> > While the behavior to ignore this option without FLASK support was
> > properly documented, it is still somewhat surprising to someone using
> > this option and then still _not_ getting the assumed security. Add a
> > 2nd handler for the command line option for the XSM_FLASK=n case, and
> > invoke panic() when the option is specified (and not subsequently
> > overridden by "flask=disabled").
> >
> > Suggested-by: Ian Jackson <ian.jackson@citrix.com>
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm very tempted to nack this outright, lest I remind both of you of the
> total disaster that was XSA-9, and the subsequent retraction of the code
> which did exactly this.

You are right to remind me of, well, whatever it is you are trying to
remind me of, since I'm afraid I don't know what you mean ...  Do you
really mean XSA-9 ?  I went at looked it up and the connection eludes
me.

> If you want to do something like this, prohibit creating guests so the
> administrator can still log in and unbreak, instead of having it
> entering a reboot loop with no output.  The console isn't established
> this early, so none of this text makes it out onto VGA/serial.

Certainly a silent reboot loop would be very unhelpful.  I think if
Xen were to actually print something to the serial console that would
be tolerable (since the administrator can then adjust the boot command
line), but your suggestion to prohibit guest creation would be fine
too.

Thanks,
Ian.

Re: [PATCH] xsm: also panic upon "flask=enforcing" when XSM_FLASK=n

[George Dunlap]

> On May 29, 2020, at 11:39 AM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> Andrew Cooper writes ("Re: [PATCH] xsm: also panic upon "flask=enforcing" when XSM_FLASK=n"):
>> On 29/05/2020 10:34, Jan Beulich wrote:
>>> While the behavior to ignore this option without FLASK support was
>>> properly documented, it is still somewhat surprising to someone using
>>> this option and then still _not_ getting the assumed security. Add a
>>> 2nd handler for the command line option for the XSM_FLASK=n case, and
>>> invoke panic() when the option is specified (and not subsequently
>>> overridden by "flask=disabled").
>>> 
>>> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> I'm very tempted to nack this outright, lest I remind both of you of the
>> total disaster that was XSA-9, and the subsequent retraction of the code
>> which did exactly this.
> 
> You are right to remind me of, well, whatever it is you are trying to
> remind me of, since I'm afraid I don't know what you mean ...  Do you
> really mean XSA-9 ?  I went at looked it up and the connection eludes
> me.

http://xenbits.xen.org/hg/xen-unstable.hg/rev/bc2f3a848f9a

Which apparently would cause Xen to (purposely) panic when booted on systems with the specified AMD erratum.

>> If you want to do something like this, prohibit creating guests so the
>> administrator can still log in and unbreak, instead of having it
>> entering a reboot loop with no output.  The console isn't established
>> this early, so none of this text makes it out onto VGA/serial.
> 
> Certainly a silent reboot loop would be very unhelpful.  I think if
> Xen were to actually print something to the serial console that would
> be tolerable (since the administrator can then adjust the boot command
> line), but your suggestion to prohibit guest creation would be fine
> too.

It seems like having the infrastructure to put Xen into a “unsafe mode”, where we refused to create guests (or some other similar response), would be a good investment to handle this sort of issue in the future.

 -George

Re: [PATCH] xsm: also panic upon "flask=enforcing" when XSM_FLASK=n

[Jan Beulich]

On 29.05.2020 12:30, Jan Beulich wrote:
> On 29.05.2020 12:07, Andrew Cooper wrote:
>> On 29/05/2020 10:34, Jan Beulich wrote:
>>> While the behavior to ignore this option without FLASK support was
>>> properly documented, it is still somewhat surprising to someone using
>>> this option and then still _not_ getting the assumed security. Add a
>>> 2nd handler for the command line option for the XSM_FLASK=n case, and
>>> invoke panic() when the option is specified (and not subsequently
>>> overridden by "flask=disabled").
>>>
>>> Suggested-by: Ian Jackson <ian.jackson@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> I'm very tempted to nack this outright, lest I remind both of you of the
>> total disaster that was XSA-9, and the subsequent retraction of the code
>> which did exactly this.
>>
>> If you want to do something like this, prohibit creating guests so the
>> administrator can still log in and unbreak,
> 
> Unbreaking is as easy as removing the command line option, or
> adding "flask=disable" at the end of the command line.
> 
> Preventing to create guests is another option, but complicated
> by the late-hwdom feature we still have - to achieve what you
> want we'd have to permit creating that one further domain.
> Dom0less perhaps also would need special treatment (and there
> I'm not sure we'd know which of the domains we are supposed to
> allow being created, and which one(s) not).

Furthermore the policy that would normally be loaded might
constrain Dom0 itself as well. Allowing Dom0 to boot is therefore
not necessarily the right thing to do. Therefore, while overall I
agree that generalizing x86's "allow_unsafe" may be a helpful
thing to do, it's not what I would want to use here. Instead,
together with ...

>> instead of having it
>> entering a reboot loop with no output.  The console isn't established
>> this early, so none of this text makes it out onto VGA/serial.
> 
> You didn't look at the patch then: I'm intentionally _not_
> panic()-ing from the command line parsing function, but from
> an initcall. Both VGA and serial have been set up by that time.
> (I was in fact considering to pull it a little earlier, into
> a pre-SMP initcall.)

... this, I think the patch wants to be re-considered as is.

Jan