diff mbox series

[for-4.11,1/2] XSM: adjust Kconfig names

Message ID 5D08EF5A02000078002394E6@prv1-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show
Series XSA-295 backport adjustments | expand

Commit Message

Jan Beulich June 18, 2019, 2:04 p.m. UTC 
Since the Kconfig option renaming was not backported, the new uses of
involved CONFIG_* settings should have been adopted to the existing
names in the XSA-295 series. Do this now, also changing XSM_SILO to just
SILO to better match its FLASK counterpart.

To avoid breaking the Kconfig menu structure also adjust XSM_POLICY's
dependency (as was also silently done on master during the renaming).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Julien Grall June 18, 2019, 2:11 p.m. UTC | #1 
Hi Jan,

On 18/06/2019 15:04, Jan Beulich wrote:
> Since the Kconfig option renaming was not backported, the new uses of
> involved CONFIG_* settings should have been adopted to the existing
> names in the XSA-295 series. Do this now, also changing XSM_SILO to just
> SILO to better match its FLASK counterpart.
> 
> To avoid breaking the Kconfig menu structure also adjust XSM_POLICY's
> dependency (as was also silently done on master during the renaming).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Sorry for the breakage. To avoid such blunder during XSAs, would it be possible 
to test them on osstest before they are published?

Also, do we need to update the advisory?

Acked-by: Julien Grall <julien.grall@arm.com>

> 
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -130,7 +130,7 @@ config FLASK_AVC_STATS
>   config XSM_POLICY
>   	bool "Compile Xen with a built-in security policy"
>   	default y if HAS_CHECKPOLICY = "y"
> -	depends on XSM
> +	depends on FLASK
>   	---help---
>   	  This includes a default XSM policy in the hypervisor so that the
>   	  bootloader does not need to load a policy to get sane behavior from an
> @@ -143,7 +143,7 @@ config XSM_POLICY
>   
>   	  If unsure, say Y.
>   
> -config XSM_SILO
> +config SILO
>   	def_bool y
>   	prompt "SILO support"
>   	depends on XSM
> @@ -158,16 +158,16 @@ config XSM_SILO
>   choice
>   	prompt "Default XSM implementation"
>   	depends on XSM
> -	default XSM_SILO_DEFAULT if XSM_SILO && ARM
> -	default XSM_FLASK_DEFAULT if XSM_FLASK
> -	default XSM_SILO_DEFAULT if XSM_SILO
> +	default XSM_SILO_DEFAULT if SILO && ARM
> +	default XSM_FLASK_DEFAULT if FLASK
> +	default XSM_SILO_DEFAULT if SILO
>   	default XSM_DUMMY_DEFAULT
>   	config XSM_DUMMY_DEFAULT
>   		bool "Match non-XSM behavior"
>   	config XSM_FLASK_DEFAULT
> -		bool "FLux Advanced Security Kernel" if XSM_FLASK
> +		bool "FLux Advanced Security Kernel" if FLASK
>   	config XSM_SILO_DEFAULT
> -		bool "SILO" if XSM_SILO
> +		bool "SILO" if SILO
>   endchoice
>   
>   config LATE_HWDOM
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -738,7 +738,7 @@ extern const unsigned char xsm_init_poli
>   extern const unsigned int xsm_init_policy_size;
>   #endif
>   
> -#ifdef CONFIG_XSM_SILO
> +#ifdef CONFIG_SILO
>   extern void silo_init(void);
>   #else
>   static inline void silo_init(void) {}
> --- a/xen/xsm/Makefile
> +++ b/xen/xsm/Makefile
> @@ -1,6 +1,6 @@
>   obj-y += xsm_core.o
>   obj-$(CONFIG_XSM) += xsm_policy.o
>   obj-$(CONFIG_XSM) += dummy.o
> -obj-$(CONFIG_XSM_SILO) += silo.o
> +obj-$(CONFIG_SILO) += silo.o
>   
>   subdir-$(CONFIG_FLASK) += flask
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -38,9 +38,9 @@ enum xsm_bootparam {
>   };
>   
>   static enum xsm_bootparam __initdata xsm_bootparam =
> -#ifdef CONFIG_XSM_FLASK_DEFAULT
> +#if defined(CONFIG_XSM_FLASK_DEFAULT)
>       XSM_BOOTPARAM_FLASK;
> -#elif CONFIG_XSM_SILO_DEFAULT
> +#elif defined(CONFIG_XSM_SILO_DEFAULT)
>       XSM_BOOTPARAM_SILO;
>   #else
>       XSM_BOOTPARAM_DUMMY;
> @@ -52,11 +52,11 @@ static int __init parse_xsm_param(const
>   
>       if ( !strcmp(s, "dummy") )
>           xsm_bootparam = XSM_BOOTPARAM_DUMMY;
> -#ifdef CONFIG_XSM_FLASK
> +#ifdef CONFIG_FLASK
>       else if ( !strcmp(s, "flask") )
>           xsm_bootparam = XSM_BOOTPARAM_FLASK;
>   #endif
> -#ifdef CONFIG_XSM_SILO
> +#ifdef CONFIG_SILO
>       else if ( !strcmp(s, "silo") )
>           xsm_bootparam = XSM_BOOTPARAM_SILO;
>   #endif
> 
> 
> 
>
Jan Beulich June 18, 2019, 2:26 p.m. UTC | #2 
>>> On 18.06.19 at 16:11, <julien.grall@arm.com> wrote:
> On 18/06/2019 15:04, Jan Beulich wrote:
>> Since the Kconfig option renaming was not backported, the new uses of
>> involved CONFIG_* settings should have been adopted to the existing
>> names in the XSA-295 series. Do this now, also changing XSM_SILO to just
>> SILO to better match its FLASK counterpart.
>> 
>> To avoid breaking the Kconfig menu structure also adjust XSM_POLICY's
>> dependency (as was also silently done on master during the renaming).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Sorry for the breakage. To avoid such blunder during XSAs, would it be possible 
> to test them on osstest before they are published?

That's an option, but would cause further delays. How exactly to
arrange for this I'm the wrong one to ask, though.

But let's face it: The patch changing Kconfig not having applied
without fuzz should have told whoever did the backport to look
more closely.

What I'd like to ask for in the future in any case though is that after
pushing stuff to stable trees you would please check the osstest
reports, and in case of regressions invest at least some time into
figuring out what broke. Right now, even with the XSM tests
(hopefully) taken care of there's still a flood of armhf failures, which
may or may not be due to environmental issues.

> Also, do we need to update the advisory?

Dunno. I didn't do full analysis of what may go wrong, I've just worked
my way far enough to understand what needs fixing. Whether an
update is needed imo largely depends on whether the purpose of the
patches wasn't fulfilled. People actually using XSM will notice very
quickly that things don't work anymore, as can be seen from the
osstest cases.

> Acked-by: Julien Grall <julien.grall@arm.com>

Thanks, Jan
Julien Grall June 18, 2019, 2:44 p.m. UTC | #3 
(+ Ian)

On 18/06/2019 15:26, Jan Beulich wrote:
>>>> On 18.06.19 at 16:11, <julien.grall@arm.com> wrote:
>> On 18/06/2019 15:04, Jan Beulich wrote:
>>> Since the Kconfig option renaming was not backported, the new uses of
>>> involved CONFIG_* settings should have been adopted to the existing
>>> names in the XSA-295 series. Do this now, also changing XSM_SILO to just
>>> SILO to better match its FLASK counterpart.
>>>
>>> To avoid breaking the Kconfig menu structure also adjust XSM_POLICY's
>>> dependency (as was also silently done on master during the renaming).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Sorry for the breakage. To avoid such blunder during XSAs, would it be possible
>> to test them on osstest before they are published?
> 
> That's an option, but would cause further delays. How exactly to
> arrange for this I'm the wrong one to ask, though.

Indeed, however testings need to be done manually at the moment. With 6 trees to 
take care, this is more likely going to delay more than automatic testing.

Anyway, that's only a suggestion to improve XSA testings (at least on Arm). :)

> 
> But let's face it: The patch changing Kconfig not having applied
> without fuzz should have told whoever did the backport to look
> more closely.
> 
> What I'd like to ask for in the future in any case though is that after
> pushing stuff to stable trees you would please check the osstest
> reports, and in case of regressions invest at least some time into
> figuring out what broke. Right now, even with the XSM tests
> (hopefully) taken care of there's still a flood of armhf failures, which
> may or may not be due to environmental issues.

I usually look over osstest but fail to detect this was an issue because of the 
XSAs. Regarding the other armhf failure, Ian already pointed out on IRC.

However, I will not have time to look at it before Xen Summit. Maybe Stefano can?

>> Also, do we need to update the advisory?
> 
> Dunno. I didn't do full analysis of what may go wrong, I've just worked
> my way far enough to understand what needs fixing. Whether an
> update is needed imo largely depends on whether the purpose of the
> patches wasn't fulfilled. People actually using XSM will notice very
> quickly that things don't work anymore, as can be seen from the
> osstest cases.

AFAICT, Arm does not seem to be affected by the problem (at least osstest does 
not complain). I would not expect x86 users to merge those patch, so maybe it 
should be ok.

Cheers,
Jan Beulich June 18, 2019, 3:04 p.m. UTC | #4 
>>> On 18.06.19 at 16:44, <julien.grall@arm.com> wrote:
> On 18/06/2019 15:26, Jan Beulich wrote:
>> What I'd like to ask for in the future in any case though is that after
>> pushing stuff to stable trees you would please check the osstest
>> reports, and in case of regressions invest at least some time into
>> figuring out what broke. Right now, even with the XSM tests
>> (hopefully) taken care of there's still a flood of armhf failures, which
>> may or may not be due to environmental issues.
> 
> I usually look over osstest but fail to detect this was an issue because of the 
> XSAs. Regarding the other armhf failure, Ian already pointed out on IRC.
> 
> However, I will not have time to look at it before Xen Summit. Maybe Stefano 
> can?

Not before the summit? That's still almost a month out. We really want
to get 4.11.2 and also 4.10.4 out the door before that.

>>> Also, do we need to update the advisory?
>> 
>> Dunno. I didn't do full analysis of what may go wrong, I've just worked
>> my way far enough to understand what needs fixing. Whether an
>> update is needed imo largely depends on whether the purpose of the
>> patches wasn't fulfilled. People actually using XSM will notice very
>> quickly that things don't work anymore, as can be seen from the
>> osstest cases.
> 
> AFAICT, Arm does not seem to be affected by the problem (at least osstest does 
> not complain). I would not expect x86 users to merge those patch, so maybe it 
> should be ok.

Well, the breakage was in the one case where SILO mode actually
disallows what the test is specifically about - qemu running in a
stubdom, and hence needing to communicate with the actual guest.
I don't think there's any similar test to this for Arm in osstest.

Jan
diff mbox series

Patch

--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -130,7 +130,7 @@  config FLASK_AVC_STATS
 config XSM_POLICY
 	bool "Compile Xen with a built-in security policy"
 	default y if HAS_CHECKPOLICY = "y"
-	depends on XSM
+	depends on FLASK
 	---help---
 	  This includes a default XSM policy in the hypervisor so that the
 	  bootloader does not need to load a policy to get sane behavior from an
@@ -143,7 +143,7 @@  config XSM_POLICY
 
 	  If unsure, say Y.
 
-config XSM_SILO
+config SILO
 	def_bool y
 	prompt "SILO support"
 	depends on XSM
@@ -158,16 +158,16 @@  config XSM_SILO
 choice
 	prompt "Default XSM implementation"
 	depends on XSM
-	default XSM_SILO_DEFAULT if XSM_SILO && ARM
-	default XSM_FLASK_DEFAULT if XSM_FLASK
-	default XSM_SILO_DEFAULT if XSM_SILO
+	default XSM_SILO_DEFAULT if SILO && ARM
+	default XSM_FLASK_DEFAULT if FLASK
+	default XSM_SILO_DEFAULT if SILO
 	default XSM_DUMMY_DEFAULT
 	config XSM_DUMMY_DEFAULT
 		bool "Match non-XSM behavior"
 	config XSM_FLASK_DEFAULT
-		bool "FLux Advanced Security Kernel" if XSM_FLASK
+		bool "FLux Advanced Security Kernel" if FLASK
 	config XSM_SILO_DEFAULT
-		bool "SILO" if XSM_SILO
+		bool "SILO" if SILO
 endchoice
 
 config LATE_HWDOM
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -738,7 +738,7 @@  extern const unsigned char xsm_init_poli
 extern const unsigned int xsm_init_policy_size;
 #endif
 
-#ifdef CONFIG_XSM_SILO
+#ifdef CONFIG_SILO
 extern void silo_init(void);
 #else
 static inline void silo_init(void) {}
--- a/xen/xsm/Makefile
+++ b/xen/xsm/Makefile
@@ -1,6 +1,6 @@ 
 obj-y += xsm_core.o
 obj-$(CONFIG_XSM) += xsm_policy.o
 obj-$(CONFIG_XSM) += dummy.o
-obj-$(CONFIG_XSM_SILO) += silo.o
+obj-$(CONFIG_SILO) += silo.o
 
 subdir-$(CONFIG_FLASK) += flask
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -38,9 +38,9 @@  enum xsm_bootparam {
 };
 
 static enum xsm_bootparam __initdata xsm_bootparam =
-#ifdef CONFIG_XSM_FLASK_DEFAULT
+#if defined(CONFIG_XSM_FLASK_DEFAULT)
     XSM_BOOTPARAM_FLASK;
-#elif CONFIG_XSM_SILO_DEFAULT
+#elif defined(CONFIG_XSM_SILO_DEFAULT)
     XSM_BOOTPARAM_SILO;
 #else
     XSM_BOOTPARAM_DUMMY;
@@ -52,11 +52,11 @@  static int __init parse_xsm_param(const
 
     if ( !strcmp(s, "dummy") )
         xsm_bootparam = XSM_BOOTPARAM_DUMMY;
-#ifdef CONFIG_XSM_FLASK
+#ifdef CONFIG_FLASK
     else if ( !strcmp(s, "flask") )
         xsm_bootparam = XSM_BOOTPARAM_FLASK;
 #endif
-#ifdef CONFIG_XSM_SILO
+#ifdef CONFIG_SILO
     else if ( !strcmp(s, "silo") )
         xsm_bootparam = XSM_BOOTPARAM_SILO;
 #endif