[PATCH v2 0/3] Kconfig oldconfig string update

[PATCH v2 0/3] Kconfig oldconfig string update

[Mickaël Salaün]

Hi,

This patch series gives the opportunity to users, when running make
oldconfig, to update configuration strings (e.g. CONFIG_LSM) according
to dependency changes.  This helps users keep a consistent up-to-date
kernel configuration.

This second series fixes a build error reported by kernel test robot
when building without any LSM.

This patch series can be applied on v5.11 .  Previous version:
https://lore.kernel.org/r/20210215122513.1773897-1-mic@digikod.net

Regards,

Mickaël Salaün (3):
  kconfig: Remove duplicate call to sym_get_string_value()
  kconfig: Ask user if string needs to be changed when dependency
    changed
  security: Add LSMs dependencies to CONFIG_LSM

 scripts/kconfig/conf.c | 37 ++++++++++++++++++++++++++++++++++---
 security/Kconfig       |  4 ++++
 2 files changed, 38 insertions(+), 3 deletions(-)


base-commit: f40ddce88593482919761f74910f42f4b84c004b
-- 
2.30.0

[PATCH v2 1/3] kconfig: Remove duplicate call to sym_get_string_value()

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Use the saved returned value of sym_get_string_value() instead of
calling it twice.

Cc: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210215181511.2840674-2-mic@digikod.net
---
 scripts/kconfig/conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index db03e2f45de4..18a233d27a8d 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -137,7 +137,7 @@ static int conf_string(struct menu *menu)
 		printf("%*s%s ", indent - 1, "", menu->prompt->text);
 		printf("(%s) ", sym->name);
 		def = sym_get_string_value(sym);
-		if (sym_get_string_value(sym))
+		if (def)
 			printf("[%s] ", def);
 		if (!conf_askvalue(sym, def))
 			return 0;
-- 
2.30.0

[PATCH v2 2/3] kconfig: Ask user if string needs to be changed when dependency changed

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Content of string configuration may depend on related kernel
configurations.  Modify oldconfig and syncconfig to inform users about
possible required configuration update and give them the opportunity to
update it:
* if dependencies of this string has changed (e.g. enabled or disabled),
* and if the current value of this string is different than the (new)
  default one.

This is particularly relevant for CONFIG_LSM which contains a list of
LSMs enabled at boot, but users will not have a chance to update this
list with a make oldconfig.

Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: James Morris <jmorris@namei.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210215181511.2840674-3-mic@digikod.net
---
 scripts/kconfig/conf.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 18a233d27a8d..8633dacd39a9 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -82,6 +82,26 @@ static void xfgets(char *str, int size, FILE *in)
 		printf("%s", str);
 }
 
+static bool may_need_string_update(struct symbol *sym, const char *def)
+{
+	const struct symbol *dep_sym;
+	const struct expr *e;
+
+	if (sym->type != S_STRING)
+		return false;
+	if (strcmp(def, sym_get_string_default(sym)) == 0)
+		return false;
+	/*
+	 * The user may want to synchronize the content of a string related to
+	 * changed dependencies (e.g. CONFIG_LSM).
+	 */
+	expr_list_for_each_sym(sym->dir_dep.expr, e, dep_sym) {
+		if (dep_sym->flags & SYMBOL_CHANGED)
+			return true;
+	}
+	return false;
+}
+
 static int conf_askvalue(struct symbol *sym, const char *def)
 {
 	enum symbol_type type = sym_get_type(sym);
@@ -102,7 +122,7 @@ static int conf_askvalue(struct symbol *sym, const char *def)
 	switch (input_mode) {
 	case oldconfig:
 	case syncconfig:
-		if (sym_has_value(sym)) {
+		if (sym_has_value(sym) && !may_need_string_update(sym, def)) {
 			printf("%s\n", def);
 			return 0;
 		}
@@ -137,8 +157,19 @@ static int conf_string(struct menu *menu)
 		printf("%*s%s ", indent - 1, "", menu->prompt->text);
 		printf("(%s) ", sym->name);
 		def = sym_get_string_value(sym);
-		if (def)
-			printf("[%s] ", def);
+		if (def) {
+			if (may_need_string_update(sym, def)) {
+				indent += 2;
+				printf("\n%*sDefault value is [%s]\n",
+						indent - 1, "",
+						sym_get_string_default(sym));
+				printf("%*sCurrent value is [%s] ",
+						indent - 1, "", def);
+				indent -= 2;
+			} else {
+				printf("[%s] ", def);
+			}
+		}
 		if (!conf_askvalue(sym, def))
 			return 0;
 		switch (line[0]) {
-- 
2.30.0

[PATCH v2 3/3] security: Add LSMs dependencies to CONFIG_LSM

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Thanks to the previous commit, this gives the opportunity to users, when
running make oldconfig, to update the list of enabled LSMs at boot time
if an LSM has just been enabled or disabled in the build.  Moreover,
this list only makes sense if at least one LSM is enabled.

Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: James Morris <jmorris@namei.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210215181511.2840674-4-mic@digikod.net
---

Changes since v1:
* Add CONFIG_SECURITY as a dependency of CONFIG_LSM.  This prevent an
  error when building without any LSMs.
---
 security/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/security/Kconfig b/security/Kconfig
index 7561f6f99f1d..addcc1c04701 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -277,6 +277,10 @@ endchoice
 
 config LSM
 	string "Ordered list of enabled LSMs"
+	depends on SECURITY || SECURITY_LOCKDOWN_LSM || SECURITY_YAMA || \
+		SECURITY_LOADPIN || SECURITY_SAFESETID || INTEGRITY || \
+		SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || \
+		SECURITY_APPARMOR || BPF_LSM
 	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
 	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
 	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
-- 
2.30.0

Re: [PATCH v2 3/3] security: Add LSMs dependencies to CONFIG_LSM

[Ondrej Mosnacek]

On Mon, Feb 15, 2021 at 7:17 PM Mickaël Salaün <mic@digikod.net> wrote:
> From: Mickaël Salaün <mic@linux.microsoft.com>
>
> Thanks to the previous commit, this gives the opportunity to users, when
> running make oldconfig, to update the list of enabled LSMs at boot time
> if an LSM has just been enabled or disabled in the build.  Moreover,
> this list only makes sense if at least one LSM is enabled.
>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> Link: https://lore.kernel.org/r/20210215181511.2840674-4-mic@digikod.net
> ---
>
> Changes since v1:
> * Add CONFIG_SECURITY as a dependency of CONFIG_LSM.  This prevent an
>   error when building without any LSMs.
> ---
>  security/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/security/Kconfig b/security/Kconfig
> index 7561f6f99f1d..addcc1c04701 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -277,6 +277,10 @@ endchoice
>
>  config LSM
>         string "Ordered list of enabled LSMs"
> +       depends on SECURITY || SECURITY_LOCKDOWN_LSM || SECURITY_YAMA || \
> +               SECURITY_LOADPIN || SECURITY_SAFESETID || INTEGRITY || \
> +               SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || \
> +               SECURITY_APPARMOR || BPF_LSM

This looks really awkward, since all of these already depend on
SECURITY (if not, it's a bug)... I guarantee you that after some time
someone will come, see that the weird boolean expression is equivalent
to just SECURITY, and simplify it.

I assume the new mechanism wouldn't work as intended if there is just
SECURITY? If not, then maybe you should rather specify this value
dependency via some new  field rather than abusing "depends on" (say,
"value depends on"?). The fact that a seemingly innocent change to the
config definition breaks your mechanism suggests that the design is
flawed.

I do think this would be a useful feature, but IMHO shouldn't be
implemented like this.

>         default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
>         default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
>         default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> --
> 2.30.0
>

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 2/3] kconfig: Ask user if string needs to be changed when dependency changed

[Masahiro Yamada]

On Tue, Feb 16, 2021 at 3:14 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> From: Mickaël Salaün <mic@linux.microsoft.com>
>
> Content of string configuration may depend on related kernel
> configurations.  Modify oldconfig and syncconfig to inform users about
> possible required configuration update and give them the opportunity to
> update it:
> * if dependencies of this string has changed (e.g. enabled or disabled),
> * and if the current value of this string is different than the (new)
>   default one.
>
> This is particularly relevant for CONFIG_LSM which contains a list of
> LSMs enabled at boot, but users will not have a chance to update this
> list with a make oldconfig.

If CONFIG_LSM already exists in the .config,
oldconfig does not show a prompt.
This is the expected behavior.

You are trying to fix your problem in a wrong way.
NACK.



>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> Link: https://lore.kernel.org/r/20210215181511.2840674-3-mic@digikod.net
> ---
>  scripts/kconfig/conf.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 18a233d27a8d..8633dacd39a9 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -82,6 +82,26 @@ static void xfgets(char *str, int size, FILE *in)
>                 printf("%s", str);
>  }
>
> +static bool may_need_string_update(struct symbol *sym, const char *def)
> +{
> +       const struct symbol *dep_sym;
> +       const struct expr *e;
> +
> +       if (sym->type != S_STRING)
> +               return false;
> +       if (strcmp(def, sym_get_string_default(sym)) == 0)
> +               return false;
> +       /*
> +        * The user may want to synchronize the content of a string related to
> +        * changed dependencies (e.g. CONFIG_LSM).
> +        */
> +       expr_list_for_each_sym(sym->dir_dep.expr, e, dep_sym) {
> +               if (dep_sym->flags & SYMBOL_CHANGED)
> +                       return true;
> +       }
> +       return false;
> +}
> +
>  static int conf_askvalue(struct symbol *sym, const char *def)
>  {
>         enum symbol_type type = sym_get_type(sym);
> @@ -102,7 +122,7 @@ static int conf_askvalue(struct symbol *sym, const char *def)
>         switch (input_mode) {
>         case oldconfig:
>         case syncconfig:
> -               if (sym_has_value(sym)) {
> +               if (sym_has_value(sym) && !may_need_string_update(sym, def)) {
>                         printf("%s\n", def);
>                         return 0;
>                 }
> @@ -137,8 +157,19 @@ static int conf_string(struct menu *menu)
>                 printf("%*s%s ", indent - 1, "", menu->prompt->text);
>                 printf("(%s) ", sym->name);
>                 def = sym_get_string_value(sym);
> -               if (def)
> -                       printf("[%s] ", def);
> +               if (def) {
> +                       if (may_need_string_update(sym, def)) {
> +                               indent += 2;
> +                               printf("\n%*sDefault value is [%s]\n",
> +                                               indent - 1, "",
> +                                               sym_get_string_default(sym));
> +                               printf("%*sCurrent value is [%s] ",
> +                                               indent - 1, "", def);
> +                               indent -= 2;
> +                       } else {
> +                               printf("[%s] ", def);
> +                       }
> +               }
>                 if (!conf_askvalue(sym, def))
>                         return 0;
>                 switch (line[0]) {
> --
> 2.30.0
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 3/3] security: Add LSMs dependencies to CONFIG_LSM

[Masahiro Yamada]

On Tue, Feb 16, 2021 at 4:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Mon, Feb 15, 2021 at 7:17 PM Mickaël Salaün <mic@digikod.net> wrote:
> > From: Mickaël Salaün <mic@linux.microsoft.com>
> >
> > Thanks to the previous commit, this gives the opportunity to users, when
> > running make oldconfig, to update the list of enabled LSMs at boot time
> > if an LSM has just been enabled or disabled in the build.  Moreover,
> > this list only makes sense if at least one LSM is enabled.
> >
> > Cc: Casey Schaufler <casey@schaufler-ca.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > Cc: Serge E. Hallyn <serge@hallyn.com>
> > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> > Link: https://lore.kernel.org/r/20210215181511.2840674-4-mic@digikod.net
> > ---
> >
> > Changes since v1:
> > * Add CONFIG_SECURITY as a dependency of CONFIG_LSM.  This prevent an
> >   error when building without any LSMs.
> > ---
> >  security/Kconfig | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/security/Kconfig b/security/Kconfig
> > index 7561f6f99f1d..addcc1c04701 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -277,6 +277,10 @@ endchoice
> >
> >  config LSM
> >         string "Ordered list of enabled LSMs"
> > +       depends on SECURITY || SECURITY_LOCKDOWN_LSM || SECURITY_YAMA || \
> > +               SECURITY_LOADPIN || SECURITY_SAFESETID || INTEGRITY || \
> > +               SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || \
> > +               SECURITY_APPARMOR || BPF_LSM
>
> This looks really awkward, since all of these already depend on
> SECURITY (if not, it's a bug)... I guarantee you that after some time
> someone will come, see that the weird boolean expression is equivalent
> to just SECURITY, and simplify it.


Currently, LSM does not depend on SECURITY.
So you can always define LSM irrespective of SECURITY,
which seems a bug.

So, I agree with adding 'depends on SECURITY'.

What he is trying to achieve in this series
seems wrong, of course.










> I assume the new mechanism wouldn't work as intended if there is just
> SECURITY? If not, then maybe you should rather specify this value
> dependency via some new  field rather than abusing "depends on" (say,
> "value depends on"?). The fact that a seemingly innocent change to the
> config definition breaks your mechanism suggests that the design is
> flawed.
>
> I do think this would be a useful feature, but IMHO shouldn't be
> implemented like this.
>
> >         default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
> >         default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
> >         default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> > --
> > 2.30.0
> >
>
> --
> Ondrej Mosnacek
> Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 1/3] kconfig: Remove duplicate call to sym_get_string_value()

[Masahiro Yamada]

On Tue, Feb 16, 2021 at 3:15 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> From: Mickaël Salaün <mic@linux.microsoft.com>
>
> Use the saved returned value of sym_get_string_value() instead of
> calling it twice.
>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> Link: https://lore.kernel.org/r/20210215181511.2840674-2-mic@digikod.net
> ---


Applied to linux-kbuild. Thanks.



>  scripts/kconfig/conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index db03e2f45de4..18a233d27a8d 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -137,7 +137,7 @@ static int conf_string(struct menu *menu)
>                 printf("%*s%s ", indent - 1, "", menu->prompt->text);
>                 printf("(%s) ", sym->name);
>                 def = sym_get_string_value(sym);
> -               if (sym_get_string_value(sym))
> +               if (def)
>                         printf("[%s] ", def);
>                 if (!conf_askvalue(sym, def))
>                         return 0;
> --
> 2.30.0
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 2/3] kconfig: Ask user if string needs to be changed when dependency changed

[Mickaël Salaün]

On 21/02/2021 09:47, Masahiro Yamada wrote:
> On Tue, Feb 16, 2021 at 3:14 AM Mickaël Salaün <mic@digikod.net> wrote:
>>
>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>
>> Content of string configuration may depend on related kernel
>> configurations.  Modify oldconfig and syncconfig to inform users about
>> possible required configuration update and give them the opportunity to
>> update it:
>> * if dependencies of this string has changed (e.g. enabled or disabled),
>> * and if the current value of this string is different than the (new)
>>   default one.
>>
>> This is particularly relevant for CONFIG_LSM which contains a list of
>> LSMs enabled at boot, but users will not have a chance to update this
>> list with a make oldconfig.
> 
> If CONFIG_LSM already exists in the .config,
> oldconfig does not show a prompt.
> This is the expected behavior.

It is not the behavior wished for LSM stacking.

> 
> You are trying to fix your problem in a wrong way.
> NACK.

What do you suggest to ensure that users will be asked to update the
CONFIG_LSM string if they add or remove an LSM?



> 
> 
> 
>>
>> Cc: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: James Morris <jmorris@namei.org>
>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>> Link: https://lore.kernel.org/r/20210215181511.2840674-3-mic@digikod.net
>> ---
>>  scripts/kconfig/conf.c | 37 ++++++++++++++++++++++++++++++++++---
>>  1 file changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
>> index 18a233d27a8d..8633dacd39a9 100644
>> --- a/scripts/kconfig/conf.c
>> +++ b/scripts/kconfig/conf.c
>> @@ -82,6 +82,26 @@ static void xfgets(char *str, int size, FILE *in)
>>                 printf("%s", str);
>>  }
>>
>> +static bool may_need_string_update(struct symbol *sym, const char *def)
>> +{
>> +       const struct symbol *dep_sym;
>> +       const struct expr *e;
>> +
>> +       if (sym->type != S_STRING)
>> +               return false;
>> +       if (strcmp(def, sym_get_string_default(sym)) == 0)
>> +               return false;
>> +       /*
>> +        * The user may want to synchronize the content of a string related to
>> +        * changed dependencies (e.g. CONFIG_LSM).
>> +        */
>> +       expr_list_for_each_sym(sym->dir_dep.expr, e, dep_sym) {
>> +               if (dep_sym->flags & SYMBOL_CHANGED)
>> +                       return true;
>> +       }
>> +       return false;
>> +}
>> +
>>  static int conf_askvalue(struct symbol *sym, const char *def)
>>  {
>>         enum symbol_type type = sym_get_type(sym);
>> @@ -102,7 +122,7 @@ static int conf_askvalue(struct symbol *sym, const char *def)
>>         switch (input_mode) {
>>         case oldconfig:
>>         case syncconfig:
>> -               if (sym_has_value(sym)) {
>> +               if (sym_has_value(sym) && !may_need_string_update(sym, def)) {
>>                         printf("%s\n", def);
>>                         return 0;
>>                 }
>> @@ -137,8 +157,19 @@ static int conf_string(struct menu *menu)
>>                 printf("%*s%s ", indent - 1, "", menu->prompt->text);
>>                 printf("(%s) ", sym->name);
>>                 def = sym_get_string_value(sym);
>> -               if (def)
>> -                       printf("[%s] ", def);
>> +               if (def) {
>> +                       if (may_need_string_update(sym, def)) {
>> +                               indent += 2;
>> +                               printf("\n%*sDefault value is [%s]\n",
>> +                                               indent - 1, "",
>> +                                               sym_get_string_default(sym));
>> +                               printf("%*sCurrent value is [%s] ",
>> +                                               indent - 1, "", def);
>> +                               indent -= 2;
>> +                       } else {
>> +                               printf("[%s] ", def);
>> +                       }
>> +               }
>>                 if (!conf_askvalue(sym, def))
>>                         return 0;
>>                 switch (line[0]) {
>> --
>> 2.30.0
>>
> 
> 

Re: [PATCH v2 3/3] security: Add LSMs dependencies to CONFIG_LSM

[Mickaël Salaün]

On 21/02/2021 09:50, Masahiro Yamada wrote:
> On Tue, Feb 16, 2021 at 4:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>>
>> On Mon, Feb 15, 2021 at 7:17 PM Mickaël Salaün <mic@digikod.net> wrote:
>>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>>
>>> Thanks to the previous commit, this gives the opportunity to users, when
>>> running make oldconfig, to update the list of enabled LSMs at boot time
>>> if an LSM has just been enabled or disabled in the build.  Moreover,
>>> this list only makes sense if at least one LSM is enabled.
>>>
>>> Cc: Casey Schaufler <casey@schaufler-ca.com>
>>> Cc: James Morris <jmorris@namei.org>
>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>> Cc: Serge E. Hallyn <serge@hallyn.com>
>>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>>> Link: https://lore.kernel.org/r/20210215181511.2840674-4-mic@digikod.net
>>> ---
>>>
>>> Changes since v1:
>>> * Add CONFIG_SECURITY as a dependency of CONFIG_LSM.  This prevent an
>>>   error when building without any LSMs.
>>> ---
>>>  security/Kconfig | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/security/Kconfig b/security/Kconfig
>>> index 7561f6f99f1d..addcc1c04701 100644
>>> --- a/security/Kconfig
>>> +++ b/security/Kconfig
>>> @@ -277,6 +277,10 @@ endchoice
>>>
>>>  config LSM
>>>         string "Ordered list of enabled LSMs"
>>> +       depends on SECURITY || SECURITY_LOCKDOWN_LSM || SECURITY_YAMA || \
>>> +               SECURITY_LOADPIN || SECURITY_SAFESETID || INTEGRITY || \
>>> +               SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || \
>>> +               SECURITY_APPARMOR || BPF_LSM
>>
>> This looks really awkward, since all of these already depend on
>> SECURITY (if not, it's a bug)... I guarantee you that after some time
>> someone will come, see that the weird boolean expression is equivalent
>> to just SECURITY, and simplify it.
> 
> 
> Currently, LSM does not depend on SECURITY.
> So you can always define LSM irrespective of SECURITY,
> which seems a bug.
> 
> So, I agree with adding 'depends on SECURITY'.
> 
> What he is trying to achieve in this series
> seems wrong, of course.

This may be wrong in the general case, but not for CONFIG_LSM.

> 
> 
>> I assume the new mechanism wouldn't work as intended if there is just
>> SECURITY? If not, then maybe you should rather specify this value
>> dependency via some new  field rather than abusing "depends on" (say,
>> "value depends on"?). The fact that a seemingly innocent change to the
>> config definition breaks your mechanism suggests that the design is
>> flawed.

Masahiro, what do you think about this suggested "value depends on"?


>>
>> I do think this would be a useful feature, but IMHO shouldn't be
>> implemented like this.
>>
>>>         default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
>>>         default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
>>>         default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
>>> --
>>> 2.30.0
>>>
>>
>> --
>> Ondrej Mosnacek
>> Software Engineer, Linux Security - SELinux kernel
>> Red Hat, Inc.
>>
> 
> 

Re: [PATCH v2 2/3] kconfig: Ask user if string needs to be changed when dependency changed

[Masahiro Yamada]

On Sun, Feb 21, 2021 at 8:09 PM Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 21/02/2021 09:47, Masahiro Yamada wrote:
> > On Tue, Feb 16, 2021 at 3:14 AM Mickaël Salaün <mic@digikod.net> wrote:
> >>
> >> From: Mickaël Salaün <mic@linux.microsoft.com>
> >>
> >> Content of string configuration may depend on related kernel
> >> configurations.  Modify oldconfig and syncconfig to inform users about
> >> possible required configuration update and give them the opportunity to
> >> update it:
> >> * if dependencies of this string has changed (e.g. enabled or disabled),
> >> * and if the current value of this string is different than the (new)
> >>   default one.
> >>
> >> This is particularly relevant for CONFIG_LSM which contains a list of
> >> LSMs enabled at boot, but users will not have a chance to update this
> >> list with a make oldconfig.
> >
> > If CONFIG_LSM already exists in the .config,
> > oldconfig does not show a prompt.
> > This is the expected behavior.
>
> It is not the behavior wished for LSM stacking.

Because LSM is doing wrong.


> >
> > You are trying to fix your problem in a wrong way.
> > NACK.
>
> What do you suggest to ensure that users will be asked to update the
> CONFIG_LSM string if they add or remove an LSM?


Fix it in the security subsystem.


Hint:
See 050e9baa9dc9fbd9ce2b27f0056990fc9e0a08a0

-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 3/3] security: Add LSMs dependencies to CONFIG_LSM

[Masahiro Yamada]

On Sun, Feb 21, 2021 at 8:11 PM Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 21/02/2021 09:50, Masahiro Yamada wrote:
> > On Tue, Feb 16, 2021 at 4:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >>
> >> On Mon, Feb 15, 2021 at 7:17 PM Mickaël Salaün <mic@digikod.net> wrote:
> >>> From: Mickaël Salaün <mic@linux.microsoft.com>
> >>>
> >>> Thanks to the previous commit, this gives the opportunity to users, when
> >>> running make oldconfig, to update the list of enabled LSMs at boot time
> >>> if an LSM has just been enabled or disabled in the build.  Moreover,
> >>> this list only makes sense if at least one LSM is enabled.
> >>>
> >>> Cc: Casey Schaufler <casey@schaufler-ca.com>
> >>> Cc: James Morris <jmorris@namei.org>
> >>> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >>> Cc: Serge E. Hallyn <serge@hallyn.com>
> >>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> >>> Link: https://lore.kernel.org/r/20210215181511.2840674-4-mic@digikod.net
> >>> ---
> >>>
> >>> Changes since v1:
> >>> * Add CONFIG_SECURITY as a dependency of CONFIG_LSM.  This prevent an
> >>>   error when building without any LSMs.
> >>> ---
> >>>  security/Kconfig | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/security/Kconfig b/security/Kconfig
> >>> index 7561f6f99f1d..addcc1c04701 100644
> >>> --- a/security/Kconfig
> >>> +++ b/security/Kconfig
> >>> @@ -277,6 +277,10 @@ endchoice
> >>>
> >>>  config LSM
> >>>         string "Ordered list of enabled LSMs"
> >>> +       depends on SECURITY || SECURITY_LOCKDOWN_LSM || SECURITY_YAMA || \
> >>> +               SECURITY_LOADPIN || SECURITY_SAFESETID || INTEGRITY || \
> >>> +               SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || \
> >>> +               SECURITY_APPARMOR || BPF_LSM
> >>
> >> This looks really awkward, since all of these already depend on
> >> SECURITY (if not, it's a bug)... I guarantee you that after some time
> >> someone will come, see that the weird boolean expression is equivalent
> >> to just SECURITY, and simplify it.
> >
> >
> > Currently, LSM does not depend on SECURITY.
> > So you can always define LSM irrespective of SECURITY,
> > which seems a bug.
> >
> > So, I agree with adding 'depends on SECURITY'.
> >
> > What he is trying to achieve in this series
> > seems wrong, of course.
>
> This may be wrong in the general case, but not for CONFIG_LSM.
>
> >
> >
> >> I assume the new mechanism wouldn't work as intended if there is just
> >> SECURITY? If not, then maybe you should rather specify this value
> >> dependency via some new  field rather than abusing "depends on" (say,
> >> "value depends on"?). The fact that a seemingly innocent change to the
> >> config definition breaks your mechanism suggests that the design is
> >> flawed.
>
> Masahiro, what do you think about this suggested "value depends on"?


Of course, no.


See the help text in init/Kconfig:

          This choice is there only for converting CONFIG_DEFAULT_SECURITY
          in old kernel configs to CONFIG_LSM in new kernel configs. Don't
          change this choice unless you are creating a fresh kernel config,
          for this choice will be ignored after CONFIG_LSM has been set.


When CONFIG_LSM is already set in the .config,
this choice is just ignored.
So, oldconfig is working as the help message says.

If you think 2623c4fbe2ad1341ff2d1e12410d0afdae2490ca
is a pointless commit, you should ask Kees about it.






> >>
> >> I do think this would be a useful feature, but IMHO shouldn't be
> >> implemented like this.
> >>
> >>>         default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
> >>>         default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
> >>>         default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> >>> --
> >>> 2.30.0
> >>>
> >>
> >> --
> >> Ondrej Mosnacek
> >> Software Engineer, Linux Security - SELinux kernel
> >> Red Hat, Inc.
> >>
> >
> >
--
Best Regards
Masahiro Yamada

Re: [PATCH v2 3/3] security: Add LSMs dependencies to CONFIG_LSM

[Mickaël Salaün]

On 21/02/2021 15:45, Masahiro Yamada wrote:
> On Sun, Feb 21, 2021 at 8:11 PM Mickaël Salaün <mic@digikod.net> wrote:
>>
>>
>> On 21/02/2021 09:50, Masahiro Yamada wrote:
>>> On Tue, Feb 16, 2021 at 4:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>>>>
>>>> On Mon, Feb 15, 2021 at 7:17 PM Mickaël Salaün <mic@digikod.net> wrote:
>>>>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>>>>
>>>>> Thanks to the previous commit, this gives the opportunity to users, when
>>>>> running make oldconfig, to update the list of enabled LSMs at boot time
>>>>> if an LSM has just been enabled or disabled in the build.  Moreover,
>>>>> this list only makes sense if at least one LSM is enabled.
>>>>>
>>>>> Cc: Casey Schaufler <casey@schaufler-ca.com>
>>>>> Cc: James Morris <jmorris@namei.org>
>>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>>>> Cc: Serge E. Hallyn <serge@hallyn.com>
>>>>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>>>>> Link: https://lore.kernel.org/r/20210215181511.2840674-4-mic@digikod.net
>>>>> ---
>>>>>
>>>>> Changes since v1:
>>>>> * Add CONFIG_SECURITY as a dependency of CONFIG_LSM.  This prevent an
>>>>>   error when building without any LSMs.
>>>>> ---
>>>>>  security/Kconfig | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/security/Kconfig b/security/Kconfig
>>>>> index 7561f6f99f1d..addcc1c04701 100644
>>>>> --- a/security/Kconfig
>>>>> +++ b/security/Kconfig
>>>>> @@ -277,6 +277,10 @@ endchoice
>>>>>
>>>>>  config LSM
>>>>>         string "Ordered list of enabled LSMs"
>>>>> +       depends on SECURITY || SECURITY_LOCKDOWN_LSM || SECURITY_YAMA || \
>>>>> +               SECURITY_LOADPIN || SECURITY_SAFESETID || INTEGRITY || \
>>>>> +               SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || \
>>>>> +               SECURITY_APPARMOR || BPF_LSM
>>>>
>>>> This looks really awkward, since all of these already depend on
>>>> SECURITY (if not, it's a bug)... I guarantee you that after some time
>>>> someone will come, see that the weird boolean expression is equivalent
>>>> to just SECURITY, and simplify it.
>>>
>>>
>>> Currently, LSM does not depend on SECURITY.
>>> So you can always define LSM irrespective of SECURITY,
>>> which seems a bug.
>>>
>>> So, I agree with adding 'depends on SECURITY'.
>>>
>>> What he is trying to achieve in this series
>>> seems wrong, of course.
>>
>> This may be wrong in the general case, but not for CONFIG_LSM.
>>
>>>
>>>
>>>> I assume the new mechanism wouldn't work as intended if there is just
>>>> SECURITY? If not, then maybe you should rather specify this value
>>>> dependency via some new  field rather than abusing "depends on" (say,
>>>> "value depends on"?). The fact that a seemingly innocent change to the
>>>> config definition breaks your mechanism suggests that the design is
>>>> flawed.
>>
>> Masahiro, what do you think about this suggested "value depends on"?
> 
> 
> Of course, no.
> 
> 
> See the help text in init/Kconfig:
> 
>           This choice is there only for converting CONFIG_DEFAULT_SECURITY
>           in old kernel configs to CONFIG_LSM in new kernel configs. Don't
>           change this choice unless you are creating a fresh kernel config,
>           for this choice will be ignored after CONFIG_LSM has been set.
> 
> 
> When CONFIG_LSM is already set in the .config,
> this choice is just ignored.
> So, oldconfig is working as the help message says.
> 
> If you think 2623c4fbe2ad1341ff2d1e12410d0afdae2490ca
> is a pointless commit, you should ask Kees about it.

This commit was for backward compatibility to not change the configured
system behavior because of a new default configuration.
Here I want to address a forward compatibility issue: when users want to
enable an LSM, give them the opportunity to enable it at boot time too
instead of silently ignoring this new configuration at boot time.
Indeed, there is two kind of configurations: built time configuration
with Kconfig, and boot time configuration with the content of
CONFIG_LSM. However, there is no direct dependency between LSM toggles
and CONFIG_LSM once it is set.

I think a better solution would be to add a new CONFIG_LSM_AUTO boolean
to automatically generate the content of CONFIG_LSM according to the
(build/kconfig) enabled LSMs, while letting users the ability to
manually configure CONFIG_LSM otherwise. What do you think?

> 
>>>>
>>>> I do think this would be a useful feature, but IMHO shouldn't be
>>>> implemented like this.
>>>>
>>>>>         default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
>>>>>         default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
>>>>>         default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
>>>>> --
>>>>> 2.30.0
>>>>>
>>>>
>>>> --
>>>> Ondrej Mosnacek
>>>> Software Engineer, Linux Security - SELinux kernel
>>>> Red Hat, Inc.
>>>>
>>>
>>>
> --
> Best Regards
> Masahiro Yamada
> 

Re: [PATCH v2 3/3] security: Add LSMs dependencies to CONFIG_LSM

[Mickaël Salaün]

On 22/02/2021 11:47, Mickaël Salaün wrote:
> 
> On 21/02/2021 15:45, Masahiro Yamada wrote:
>> On Sun, Feb 21, 2021 at 8:11 PM Mickaël Salaün <mic@digikod.net> wrote:
>>>
>>>
>>> On 21/02/2021 09:50, Masahiro Yamada wrote:
>>>> On Tue, Feb 16, 2021 at 4:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>>>>>
>>>>> On Mon, Feb 15, 2021 at 7:17 PM Mickaël Salaün <mic@digikod.net> wrote:
>>>>>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>>>>>
>>>>>> Thanks to the previous commit, this gives the opportunity to users, when
>>>>>> running make oldconfig, to update the list of enabled LSMs at boot time
>>>>>> if an LSM has just been enabled or disabled in the build.  Moreover,
>>>>>> this list only makes sense if at least one LSM is enabled.
>>>>>>
>>>>>> Cc: Casey Schaufler <casey@schaufler-ca.com>
>>>>>> Cc: James Morris <jmorris@namei.org>
>>>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>>>>> Cc: Serge E. Hallyn <serge@hallyn.com>
>>>>>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>>>>>> Link: https://lore.kernel.org/r/20210215181511.2840674-4-mic@digikod.net
>>>>>> ---
>>>>>>
>>>>>> Changes since v1:
>>>>>> * Add CONFIG_SECURITY as a dependency of CONFIG_LSM.  This prevent an
>>>>>>   error when building without any LSMs.
>>>>>> ---
>>>>>>  security/Kconfig | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/security/Kconfig b/security/Kconfig
>>>>>> index 7561f6f99f1d..addcc1c04701 100644
>>>>>> --- a/security/Kconfig
>>>>>> +++ b/security/Kconfig
>>>>>> @@ -277,6 +277,10 @@ endchoice
>>>>>>
>>>>>>  config LSM
>>>>>>         string "Ordered list of enabled LSMs"
>>>>>> +       depends on SECURITY || SECURITY_LOCKDOWN_LSM || SECURITY_YAMA || \
>>>>>> +               SECURITY_LOADPIN || SECURITY_SAFESETID || INTEGRITY || \
>>>>>> +               SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || \
>>>>>> +               SECURITY_APPARMOR || BPF_LSM
>>>>>
>>>>> This looks really awkward, since all of these already depend on
>>>>> SECURITY (if not, it's a bug)... I guarantee you that after some time
>>>>> someone will come, see that the weird boolean expression is equivalent
>>>>> to just SECURITY, and simplify it.
>>>>
>>>>
>>>> Currently, LSM does not depend on SECURITY.
>>>> So you can always define LSM irrespective of SECURITY,
>>>> which seems a bug.
>>>>
>>>> So, I agree with adding 'depends on SECURITY'.
>>>>
>>>> What he is trying to achieve in this series
>>>> seems wrong, of course.
>>>
>>> This may be wrong in the general case, but not for CONFIG_LSM.
>>>
>>>>
>>>>
>>>>> I assume the new mechanism wouldn't work as intended if there is just
>>>>> SECURITY? If not, then maybe you should rather specify this value
>>>>> dependency via some new  field rather than abusing "depends on" (say,
>>>>> "value depends on"?). The fact that a seemingly innocent change to the
>>>>> config definition breaks your mechanism suggests that the design is
>>>>> flawed.
>>>
>>> Masahiro, what do you think about this suggested "value depends on"?
>>
>>
>> Of course, no.
>>
>>
>> See the help text in init/Kconfig:
>>
>>           This choice is there only for converting CONFIG_DEFAULT_SECURITY
>>           in old kernel configs to CONFIG_LSM in new kernel configs. Don't
>>           change this choice unless you are creating a fresh kernel config,
>>           for this choice will be ignored after CONFIG_LSM has been set.
>>
>>
>> When CONFIG_LSM is already set in the .config,
>> this choice is just ignored.
>> So, oldconfig is working as the help message says.
>>
>> If you think 2623c4fbe2ad1341ff2d1e12410d0afdae2490ca
>> is a pointless commit, you should ask Kees about it.
> 
> This commit was for backward compatibility to not change the configured
> system behavior because of a new default configuration.
> Here I want to address a forward compatibility issue: when users want to
> enable an LSM, give them the opportunity to enable it at boot time too
> instead of silently ignoring this new configuration at boot time.
> Indeed, there is two kind of configurations: built time configuration
> with Kconfig, and boot time configuration with the content of
> CONFIG_LSM. However, there is no direct dependency between LSM toggles
> and CONFIG_LSM once it is set.
> 
> I think a better solution would be to add a new CONFIG_LSM_AUTO boolean
> to automatically generate the content of CONFIG_LSM according to the
> (build/kconfig) enabled LSMs, while letting users the ability to
> manually configure CONFIG_LSM otherwise. What do you think?

I sent a new patch series dedicated to the LSM issue:
https://lore.kernel.org/linux-security-module/20210222150608.808146-1-mic@digikod.net/

> 
>>
>>>>>
>>>>> I do think this would be a useful feature, but IMHO shouldn't be
>>>>> implemented like this.
>>>>>
>>>>>>         default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
>>>>>>         default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
>>>>>>         default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
>>>>>> --
>>>>>> 2.30.0
>>>>>>
>>>>>
>>>>> --
>>>>> Ondrej Mosnacek
>>>>> Software Engineer, Linux Security - SELinux kernel
>>>>> Red Hat, Inc.
>>>>>
>>>>
>>>>
>> --
>> Best Regards
>> Masahiro Yamada
>>