LSM: Allow syzbot to ignore security= parameter.
diff mbox series

Message ID 52531a69-10ed-d263-be66-e707705597d6@i-love.sakura.ne.jp
State New
Headers show
Series
  • LSM: Allow syzbot to ignore security= parameter.
Related show

Commit Message

Tetsuo Handa Feb. 1, 2019, 1:09 p.m. UTC
On 2019/02/01 19:50, Dmitry Vyukov wrote:
> On Fri, Feb 1, 2019 at 11:44 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> On 2019/02/01 19:09, Dmitry Vyukov wrote:
>>> Thanks for the explanations.
>>>
>>> Here is the change that I've come up with:
>>> https://github.com/google/syzkaller/commit/aa53be276dc84aa8b3825b3416542447ff82b41a
>>
>> You are not going to apply this updated config to upstream kernels now, are you?
>> Removing CONFIG_DEFAULT_SECURITY="apparmor" from configs used by upstream kernels
>> will cause failing to enable AppArmor (unless security=apparmor is specified).
> 
> 
> We do use  security=apparmor, see:
> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-apparmor.cmdline
> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-selinux.cmdline
> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-smack.cmdline
> 

Oh, security= parameter is explicitly specified on all targets?
Then, we can abuse CONFIG_DEBUG_AID_FOR_SYZBOT option. ;-)

LSM folks, may we use this patch for linux-next.git ?
CONFIG_DEBUG_AID_FOR_SYZBOT is a linux-next.git-only kernel config option used by syzbot.



From c7d21f9c1c0b610ddea4233b89edf7d3140b8baf Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri, 1 Feb 2019 22:03:55 +0900
Subject: [PATCH linux-next] LSM: Allow syzbot to ignore security= parameter.

LSM is going to get infrastructure managed security blob support in Linux
5.1, and it becomes possible to run TOMOYO with SELinux/Smack/AppArmor.
But for compatibility reason, since security= parameter makes it
impossible to run TOMOYO with SELinux/Smack/AppArmor, syzbot can't
test that combination. Therefore, this patch allows syzbot to temporarily
ignore security= parameter. This patch is meant for linux-next.git only,
and will be removed after infrastructure managed security blob support
went to linux.git.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/security.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Dmitry Vyukov Feb. 4, 2019, 8:07 a.m. UTC | #1
On Fri, Feb 1, 2019 at 2:09 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2019/02/01 19:50, Dmitry Vyukov wrote:
> > On Fri, Feb 1, 2019 at 11:44 AM Tetsuo Handa
> > <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >>
> >> On 2019/02/01 19:09, Dmitry Vyukov wrote:
> >>> Thanks for the explanations.
> >>>
> >>> Here is the change that I've come up with:
> >>> https://github.com/google/syzkaller/commit/aa53be276dc84aa8b3825b3416542447ff82b41a
> >>
> >> You are not going to apply this updated config to upstream kernels now, are you?
> >> Removing CONFIG_DEFAULT_SECURITY="apparmor" from configs used by upstream kernels
> >> will cause failing to enable AppArmor (unless security=apparmor is specified).
> >
> >
> > We do use  security=apparmor, see:
> > https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-apparmor.cmdline
> > https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-selinux.cmdline
> > https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-smack.cmdline
> >
>
> Oh, security= parameter is explicitly specified on all targets?
> Then, we can abuse CONFIG_DEBUG_AID_FOR_SYZBOT option. ;-)
>
> LSM folks, may we use this patch for linux-next.git ?
> CONFIG_DEBUG_AID_FOR_SYZBOT is a linux-next.git-only kernel config option used by syzbot.


Then we also need this on syzbot side, right? Otherwise it seems that
all instances will default to a single security module.
https://github.com/google/syzkaller/commit/ffec3d1894ffd05966b50efa49ca19af76c9ea81


> From c7d21f9c1c0b610ddea4233b89edf7d3140b8baf Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Fri, 1 Feb 2019 22:03:55 +0900
> Subject: [PATCH linux-next] LSM: Allow syzbot to ignore security= parameter.
>
> LSM is going to get infrastructure managed security blob support in Linux
> 5.1, and it becomes possible to run TOMOYO with SELinux/Smack/AppArmor.
> But for compatibility reason, since security= parameter makes it
> impossible to run TOMOYO with SELinux/Smack/AppArmor, syzbot can't
> test that combination. Therefore, this patch allows syzbot to temporarily
> ignore security= parameter. This patch is meant for linux-next.git only,
> and will be removed after infrastructure managed security blob support
> went to linux.git.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  security/security.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/security/security.c b/security/security.c
> index ef03643..0632feb 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -346,12 +346,14 @@ int __init security_init(void)
>  }
>
>  /* Save user chosen LSM */
> +#ifndef CONFIG_DEBUG_AID_FOR_SYZBOT
>  static int __init choose_major_lsm(char *str)
>  {
>         chosen_major_lsm = str;
>         return 1;
>  }
>  __setup("security=", choose_major_lsm);
> +#endif
>
>  /* Explicitly choose LSM initialization order. */
>  static int __init choose_lsm_order(char *str)
> --
> 1.8.3.1
>
Tetsuo Handa Feb. 6, 2019, 10:23 a.m. UTC | #2
On 2019/02/04 17:07, Dmitry Vyukov wrote:
> On Fri, Feb 1, 2019 at 2:09 PM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> On 2019/02/01 19:50, Dmitry Vyukov wrote:
>>> On Fri, Feb 1, 2019 at 11:44 AM Tetsuo Handa
>>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>>>
>>>> On 2019/02/01 19:09, Dmitry Vyukov wrote:
>>>>> Thanks for the explanations.
>>>>>
>>>>> Here is the change that I've come up with:
>>>>> https://github.com/google/syzkaller/commit/aa53be276dc84aa8b3825b3416542447ff82b41a
>>>>
>>>> You are not going to apply this updated config to upstream kernels now, are you?
>>>> Removing CONFIG_DEFAULT_SECURITY="apparmor" from configs used by upstream kernels
>>>> will cause failing to enable AppArmor (unless security=apparmor is specified).
>>>
>>>
>>> We do use  security=apparmor, see:
>>> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-apparmor.cmdline
>>> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-selinux.cmdline
>>> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-smack.cmdline
>>>
>>
>> Oh, security= parameter is explicitly specified on all targets?
>> Then, we can abuse CONFIG_DEBUG_AID_FOR_SYZBOT option. ;-)
>>
>> LSM folks, may we use this patch for linux-next.git ?
>> CONFIG_DEBUG_AID_FOR_SYZBOT is a linux-next.git-only kernel config option used by syzbot.
> 
> 
> Then we also need this on syzbot side, right? Otherwise it seems that
> all instances will default to a single security module.
> https://github.com/google/syzkaller/commit/ffec3d1894ffd05966b50efa49ca19af76c9ea81
> 

Right.

But as I update the documentation ( https://tomoyo.osdn.jp/2.6/chapter-3.html.en#3.6 ),
I came to think that we should ignore security= parameter when lsm= parameter is specified.

Currently, it is possible to enable TOMOYO and only one of SELinux/Smack/AppArmor. Therefore,
it is possible to disable only TOMOYO by specifying security=selinux when we want to enable
only SELinux, by specifying security=smack when we want to enable only Smack, by specifying
security=apparmor when we want to enable only AppArmor. That is, we can use security= parameter
in order to specify the other LSM module which should not be disabled.

But when it becomes possible to enable TOMOYO and more than one of SELinux/Smack/AppArmor,
we will no longer be able to selectively disable one LSM module using security= parameter, for
security= parameter is intended for specifying only one LSM module which should be enabled.
That is, we will need to use lsm= parameter in order to selectively disable LSM modules.

Then, I think that it is straightforward (and easier to manage) to ignore security= parameter
when lsm= parameter is specified. Furthermore, we could even avoid introducing lsm= parameter
by allowing security= parameter to specify multiple LSM modules. For example, security= parameter
is interpreted as a list of all LSM modules which should be enabled when it contains a comma,
and it is interpreted as one of LSM_FLAG_LEGACY_MAJOR modules which should be enabled otherwise.
Then, specifying security=selinux or security=smack or security=tomoyo or security=apparmor or
security=none will respectively enable SELinux, Smack, TOMOYO, AppArmor, none of
SELinux/Smack/TOMOYO/AppArmor. And specifying e.g. security=, will disable all LSM modules.
Casey Schaufler Feb. 6, 2019, 5:03 p.m. UTC | #3
On 2/6/2019 2:23 AM, Tetsuo Handa wrote:
> On 2019/02/04 17:07, Dmitry Vyukov wrote:
>> On Fri, Feb 1, 2019 at 2:09 PM Tetsuo Handa
>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>> On 2019/02/01 19:50, Dmitry Vyukov wrote:
>>>> On Fri, Feb 1, 2019 at 11:44 AM Tetsuo Handa
>>>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>>>> On 2019/02/01 19:09, Dmitry Vyukov wrote:
>>>>>> Thanks for the explanations.
>>>>>>
>>>>>> Here is the change that I've come up with:
>>>>>> https://github.com/google/syzkaller/commit/aa53be276dc84aa8b3825b3416542447ff82b41a
>>>>> You are not going to apply this updated config to upstream kernels now, are you?
>>>>> Removing CONFIG_DEFAULT_SECURITY="apparmor" from configs used by upstream kernels
>>>>> will cause failing to enable AppArmor (unless security=apparmor is specified).
>>>>
>>>> We do use  security=apparmor, see:
>>>> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-apparmor.cmdline
>>>> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-selinux.cmdline
>>>> https://github.com/google/syzkaller/blob/master/dashboard/config/upstream-smack.cmdline
>>>>
>>> Oh, security= parameter is explicitly specified on all targets?
>>> Then, we can abuse CONFIG_DEBUG_AID_FOR_SYZBOT option. ;-)
>>>
>>> LSM folks, may we use this patch for linux-next.git ?
>>> CONFIG_DEBUG_AID_FOR_SYZBOT is a linux-next.git-only kernel config option used by syzbot.
>>
>> Then we also need this on syzbot side, right? Otherwise it seems that
>> all instances will default to a single security module.
>> https://github.com/google/syzkaller/commit/ffec3d1894ffd05966b50efa49ca19af76c9ea81
>>
> Right.
>
> But as I update the documentation ( https://tomoyo.osdn.jp/2.6/chapter-3.html.en#3.6 ),
> I came to think that we should ignore security= parameter when lsm= parameter is specified.
>
> Currently, it is possible to enable TOMOYO and only one of SELinux/Smack/AppArmor. Therefore,
> it is possible to disable only TOMOYO by specifying security=selinux when we want to enable
> only SELinux, by specifying security=smack when we want to enable only Smack, by specifying
> security=apparmor when we want to enable only AppArmor. That is, we can use security= parameter
> in order to specify the other LSM module which should not be disabled.
>
> But when it becomes possible to enable TOMOYO and more than one of SELinux/Smack/AppArmor,
> we will no longer be able to selectively disable one LSM module using security= parameter, for
> security= parameter is intended for specifying only one LSM module which should be enabled.
> That is, we will need to use lsm= parameter in order to selectively disable LSM modules.

Yes. That is correct. The existing behavior of security= is maintained.
The new behavior of lsm= is provided to allow general handling of a list
of security modules. It uses the same form of data as CONFIG_LSM.

> Then, I think that it is straightforward (and easier to manage) to ignore security= parameter
> when lsm= parameter is specified.

That reduces flexibility somewhat. If I am debugging security modules
I may want to use lsm= to specify the order while using security= to
identify a specific exclusive module. I could do that using lsm= by
itself, but habits die hard.

> Furthermore, we could even avoid introducing lsm= parameter
> by allowing security= parameter to specify multiple LSM modules.

security=yama would work differently than it does today.
There would be no way to specify an exclusive module but
no minor modules.

If I have Yama and SELinux built in I could never disable Yama.

	security=selinux - would not disable Yama

whereas

	lsm=selinux      - would disable Yama

> For example, security= parameter
> is interpreted as a list of all LSM modules which should be enabled when it contains a comma,
> and it is interpreted as one of LSM_FLAG_LEGACY_MAJOR modules which should be enabled otherwise.
> Then, specifying security=selinux or security=smack or security=tomoyo or security=apparmor or
> security=none will respectively enable SELinux, Smack, TOMOYO, AppArmor, none of
> SELinux/Smack/TOMOYO/AppArmor. And specifying e.g. security=, will disable all LSM modules.

We debated the possibility of making the comma an indication
that we had an explicit list. It comes down to the "trailing
comma" syntax, where "security=selinux" and "security=selinux,"
mean different things. Consensus was that this is too clever,
and everyone would hate it.
Tetsuo Handa Feb. 7, 2019, 2:30 a.m. UTC | #4
Casey Schaufler wrote:
> On 2/6/2019 2:23 AM, Tetsuo Handa wrote:
> > But as I update the documentation ( https://tomoyo.osdn.jp/2.6/chapter-3.html.en#3.6 ),
> > I came to think that we should ignore security= parameter when lsm= parameter is specified.
> >
> > Currently, it is possible to enable TOMOYO and only one of SELinux/Smack/AppArmor. Therefore,
> > it is possible to disable only TOMOYO by specifying security=selinux when we want to enable
> > only SELinux, by specifying security=smack when we want to enable only Smack, by specifying
> > security=apparmor when we want to enable only AppArmor. That is, we can use security= parameter
> > in order to specify the other LSM module which should not be disabled.
> >
> > But when it becomes possible to enable TOMOYO and more than one of SELinux/Smack/AppArmor,
> > we will no longer be able to selectively disable one LSM module using security= parameter, for
> > security= parameter is intended for specifying only one LSM module which should be enabled.
> > That is, we will need to use lsm= parameter in order to selectively disable LSM modules.
> 
> Yes. That is correct. The existing behavior of security= is maintained.

But the existing behavior of CONFIG_DEFAULT_SECURITY is not maintained.
This might cause a problem like

  commit e5a3b95f581da62e2054ef79d3be2d383e9ed664
  Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
  Date:   Sat Feb 14 11:46:56 2009 +0900

      TOMOYO: Don't create securityfs entries unless registered.

      TOMOYO should not create /sys/kernel/security/tomoyo/ interface unless
      TOMOYO is registered.

for Ubuntu users because Ubuntu kernels are built with

  CONFIG_SECURITY_SELINUX=y
  CONFIG_SECURITY_SMACK=y
  CONFIG_SECURITY_TOMOYO=y
  CONFIG_SECURITY_APPARMOR=y
  CONFIG_SECURITY_YAMA=y
  CONFIG_DEFAULT_SECURITY="apparmor"

. Due to CONFIG_DEFAULT_SECURITY="apparmor", majority of Ubuntu users are enabling
only AppArmor without explicitly specifying "security=apparmor".

Currently default CONFIG_LSM setting is

  "yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"

but Ubuntu kernels would have to be built with non-default CONFIG_LSM setting like

  "yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo"

in order to make sure that AppArmor is by default chosen for the LSM_FLAG_EXCLUSIVE module.

Now that TOMOYO becomes a !LSM_FLAG_EXCLUSIVE module, not specifying "security=apparmor" will
automatically enable TOMOYO. And majority of Ubuntu users will unexpectedly encounter TOMOYO
messages. But removing "tomoyo" from CONFIG_LSM setting in order to save majority of Ubuntu
users from unexpectedly encountering TOMOYO messages also has a problem; Ubuntu users who want
to enable only TOMOYO from LSM_FLAG_LEGACY_MAJOR modules can specify "security=tomoyo", but
Ubuntu users who want to enable TOMOYO and one of SELinux,Smack,AppArmor (including syzbot)
will have to explicitly specify "lsm=" because "security=" can't allow enabling multiple
LSM_FLAG_LEGACY_MAJOR modules.

> The new behavior of lsm= is provided to allow general handling of a list
> of security modules. It uses the same form of data as CONFIG_LSM.
> 
> > Then, I think that it is straightforward (and easier to manage) to ignore security= parameter
> > when lsm= parameter is specified.
> 
> That reduces flexibility somewhat. If I am debugging security modules
> I may want to use lsm= to specify the order while using security= to
> identify a specific exclusive module. I could do that using lsm= by
> itself, but habits die hard.

"lsm=" can be used for identifying a specific exclusive module, and Ubuntu kernels would
have to use CONFIG_LSM (or "lsm=") for identifying the default exclusive module (in order
to allow enabling both TOMOYO and one of SELinux,Smack,AppArmor at the same time).

Since "security=" can't be used for selectively enable/disable more than one of
SELinux,Smack,TOMOYO,AppArmor, I think that recommending users to migrate to "lsm=" is the
better direction. And ignoring "security=" when "lsm=" is specified is easier to understand.
Casey Schaufler Feb. 7, 2019, 4:24 p.m. UTC | #5
On 2/6/2019 6:30 PM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> On 2/6/2019 2:23 AM, Tetsuo Handa wrote:
>>> But as I update the documentation ( https://tomoyo.osdn.jp/2.6/chapter-3.html.en#3.6 ),
>>> I came to think that we should ignore security= parameter when lsm= parameter is specified.
>>>
>>> Currently, it is possible to enable TOMOYO and only one of SELinux/Smack/AppArmor. Therefore,
>>> it is possible to disable only TOMOYO by specifying security=selinux when we want to enable
>>> only SELinux, by specifying security=smack when we want to enable only Smack, by specifying
>>> security=apparmor when we want to enable only AppArmor. That is, we can use security= parameter
>>> in order to specify the other LSM module which should not be disabled.
>>>
>>> But when it becomes possible to enable TOMOYO and more than one of SELinux/Smack/AppArmor,
>>> we will no longer be able to selectively disable one LSM module using security= parameter, for
>>> security= parameter is intended for specifying only one LSM module which should be enabled.
>>> That is, we will need to use lsm= parameter in order to selectively disable LSM modules.
>> Yes. That is correct. The existing behavior of security= is maintained.
> But the existing behavior of CONFIG_DEFAULT_SECURITY is not maintained.

That's a developer interface, not a user interface. I realize
that may be splitting hairs, but it had to change.

> This might cause a problem like
>
>   commit e5a3b95f581da62e2054ef79d3be2d383e9ed664
>   Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>   Date:   Sat Feb 14 11:46:56 2009 +0900
>
>       TOMOYO: Don't create securityfs entries unless registered.
>
>       TOMOYO should not create /sys/kernel/security/tomoyo/ interface unless
>       TOMOYO is registered.
>
> for Ubuntu users because Ubuntu kernels are built with
>
>   CONFIG_SECURITY_SELINUX=y
>   CONFIG_SECURITY_SMACK=y
>   CONFIG_SECURITY_TOMOYO=y
>   CONFIG_SECURITY_APPARMOR=y
>   CONFIG_SECURITY_YAMA=y
>   CONFIG_DEFAULT_SECURITY="apparmor"
>
> . Due to CONFIG_DEFAULT_SECURITY="apparmor", majority of Ubuntu users are enabling
> only AppArmor without explicitly specifying "security=apparmor".
>
> Currently default CONFIG_LSM setting is
>
>   "yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
>
> but Ubuntu kernels would have to be built with non-default CONFIG_LSM setting like
>
>   "yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo"
>
> in order to make sure that AppArmor is by default chosen for the LSM_FLAG_EXCLUSIVE module.

Yes, and Yocto Project is likely to want Smack specified first.

> Now that TOMOYO becomes a !LSM_FLAG_EXCLUSIVE module, not specifying "security=apparmor" will
> automatically enable TOMOYO. And majority of Ubuntu users will unexpectedly encounter TOMOYO
> messages. But removing "tomoyo" from CONFIG_LSM setting in order to save majority of Ubuntu
> users from unexpectedly encountering TOMOYO messages also has a problem; Ubuntu users who want
> to enable only TOMOYO from LSM_FLAG_LEGACY_MAJOR modules can specify "security=tomoyo", but
> Ubuntu users who want to enable TOMOYO and one of SELinux,Smack,AppArmor (including syzbot)
> will have to explicitly specify "lsm=" because "security=" can't allow enabling multiple
> LSM_FLAG_LEGACY_MAJOR modules.

I believe we got general buy in from Ubuntu, and I understand
that the LSM list is awkward, but I don't see a rational alternate.
I know that I played with a half dozen, and nothing was closer to
maintaining the status quo.

>> The new behavior of lsm= is provided to allow general handling of a list
>> of security modules. It uses the same form of data as CONFIG_LSM.
>>
>>> Then, I think that it is straightforward (and easier to manage) to ignore security= parameter
>>> when lsm= parameter is specified.
>> That reduces flexibility somewhat. If I am debugging security modules
>> I may want to use lsm= to specify the order while using security= to
>> identify a specific exclusive module. I could do that using lsm= by
>> itself, but habits die hard.
> "lsm=" can be used for identifying a specific exclusive module, and Ubuntu kernels would
> have to use CONFIG_LSM (or "lsm=") for identifying the default exclusive module (in order
> to allow enabling both TOMOYO and one of SELinux,Smack,AppArmor at the same time).
>
> Since "security=" can't be used for selectively enable/disable more than one of
> SELinux,Smack,TOMOYO,AppArmor, I think that recommending users to migrate to "lsm=" is the
> better direction. And ignoring "security=" when "lsm=" is specified is easier to understand.

I added Kees to the CC list. Kees, what to you think about
ignoring security= if lsm= is specified? I'm ambivalent.
Tetsuo Handa Feb. 8, 2019, 10:52 a.m. UTC | #6
On 2019/02/08 1:24, Casey Schaufler wrote:
>>>> Then, I think that it is straightforward (and easier to manage) to ignore security= parameter
>>>> when lsm= parameter is specified.
>>> That reduces flexibility somewhat. If I am debugging security modules
>>> I may want to use lsm= to specify the order while using security= to
>>> identify a specific exclusive module. I could do that using lsm= by
>>> itself, but habits die hard.
>> "lsm=" can be used for identifying a specific exclusive module, and Ubuntu kernels would
>> have to use CONFIG_LSM (or "lsm=") for identifying the default exclusive module (in order
>> to allow enabling both TOMOYO and one of SELinux,Smack,AppArmor at the same time).
>>
>> Since "security=" can't be used for selectively enable/disable more than one of
>> SELinux,Smack,TOMOYO,AppArmor, I think that recommending users to migrate to "lsm=" is the
>> better direction. And ignoring "security=" when "lsm=" is specified is easier to understand.
> 
> I added Kees to the CC list. Kees, what to you think about
> ignoring security= if lsm= is specified? I'm ambivalent.
> 
> 

To help administrators easily understand what LSM modules are possibly enabled by default (which
have to be fetched from e.g. /boot/config-`uname -r`) and specify lsm= parameter when they need,
I propose changes shown below.

diff --git a/security/security.c b/security/security.c
index 3147785e..051d708 100644
--- a/security/security.c
+++ b/security/security.c
@@ -51,8 +51,6 @@
 static __initdata const char *chosen_lsm_order;
 static __initdata const char *chosen_major_lsm;
 
-static __initconst const char * const builtin_lsm_order = CONFIG_LSM;
-
 /* Ordered list of LSMs to initialize. */
 static __initdata struct lsm_info **ordered_lsms;
 static __initdata struct lsm_info *exclusive;
@@ -284,14 +282,22 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
 static void __init ordered_lsm_init(void)
 {
 	struct lsm_info **lsm;
+	const char *order = CONFIG_LSM;
+	const char *origin = "builtin";
 
 	ordered_lsms = kcalloc(LSM_COUNT + 1, sizeof(*ordered_lsms),
 				GFP_KERNEL);
 
-	if (chosen_lsm_order)
-		ordered_lsm_parse(chosen_lsm_order, "cmdline");
-	else
-		ordered_lsm_parse(builtin_lsm_order, "builtin");
+	if (chosen_lsm_order) {
+		if (chosen_major_lsm) {
+			pr_info("security= is ignored because of lsm=\n");
+			chosen_major_lsm = NULL;
+		}
+		order = chosen_lsm_order;
+		origin = "cmdline";
+	}
+	pr_info("Security Framework initializing: %s\n", order);
+	ordered_lsm_parse(order, origin);
 
 	for (lsm = ordered_lsms; *lsm; lsm++)
 		prepare_lsm(*lsm);
@@ -333,8 +339,6 @@ int __init security_init(void)
 	int i;
 	struct hlist_head *list = (struct hlist_head *) &security_hook_heads;
 
-	pr_info("Security Framework initializing\n");
-
 	for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct hlist_head);
 	     i++)
 		INIT_HLIST_HEAD(&list[i]);
Casey Schaufler Feb. 8, 2019, 4:23 p.m. UTC | #7
On 2/8/2019 2:52 AM, Tetsuo Handa wrote:
> On 2019/02/08 1:24, Casey Schaufler wrote:
>>>>> Then, I think that it is straightforward (and easier to manage) to ignore security= parameter
>>>>> when lsm= parameter is specified.
>>>> That reduces flexibility somewhat. If I am debugging security modules
>>>> I may want to use lsm= to specify the order while using security= to
>>>> identify a specific exclusive module. I could do that using lsm= by
>>>> itself, but habits die hard.
>>> "lsm=" can be used for identifying a specific exclusive module, and Ubuntu kernels would
>>> have to use CONFIG_LSM (or "lsm=") for identifying the default exclusive module (in order
>>> to allow enabling both TOMOYO and one of SELinux,Smack,AppArmor at the same time).
>>>
>>> Since "security=" can't be used for selectively enable/disable more than one of
>>> SELinux,Smack,TOMOYO,AppArmor, I think that recommending users to migrate to "lsm=" is the
>>> better direction. And ignoring "security=" when "lsm=" is specified is easier to understand.
>> I added Kees to the CC list. Kees, what to you think about
>> ignoring security= if lsm= is specified? I'm ambivalent.
>>
>>
> To help administrators easily understand what LSM modules are possibly enabled by default (which
> have to be fetched from e.g. /boot/config-`uname -r`)

$ cat /sys/kernel/security/lsm

>  and specify lsm= parameter when they need,
> I propose changes shown below.
>
> diff --git a/security/security.c b/security/security.c
> index 3147785e..051d708 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -51,8 +51,6 @@
>  static __initdata const char *chosen_lsm_order;
>  static __initdata const char *chosen_major_lsm;
>  
> -static __initconst const char * const builtin_lsm_order = CONFIG_LSM;
> -
>  /* Ordered list of LSMs to initialize. */
>  static __initdata struct lsm_info **ordered_lsms;
>  static __initdata struct lsm_info *exclusive;
> @@ -284,14 +282,22 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>  static void __init ordered_lsm_init(void)
>  {
>  	struct lsm_info **lsm;
> +	const char *order = CONFIG_LSM;
> +	const char *origin = "builtin";
>  
>  	ordered_lsms = kcalloc(LSM_COUNT + 1, sizeof(*ordered_lsms),
>  				GFP_KERNEL);
>  
> -	if (chosen_lsm_order)
> -		ordered_lsm_parse(chosen_lsm_order, "cmdline");
> -	else
> -		ordered_lsm_parse(builtin_lsm_order, "builtin");
> +	if (chosen_lsm_order) {
> +		if (chosen_major_lsm) {
> +			pr_info("security= is ignored because of lsm=\n");
> +			chosen_major_lsm = NULL;
> +		}
> +		order = chosen_lsm_order;
> +		origin = "cmdline";
> +	}
> +	pr_info("Security Framework initializing: %s\n", order);
> +	ordered_lsm_parse(order, origin);
>  
>  	for (lsm = ordered_lsms; *lsm; lsm++)
>  		prepare_lsm(*lsm);
> @@ -333,8 +339,6 @@ int __init security_init(void)
>  	int i;
>  	struct hlist_head *list = (struct hlist_head *) &security_hook_heads;
>  
> -	pr_info("Security Framework initializing\n");
> -
>  	for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct hlist_head);
>  	     i++)
>  		INIT_HLIST_HEAD(&list[i]);

I'm not going to object to this, but I don't see it as important.
Kees Cook Feb. 8, 2019, 9:33 p.m. UTC | #8
On Thu, Feb 7, 2019 at 8:24 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> I added Kees to the CC list. Kees, what to you think about
> ignoring security= if lsm= is specified? I'm ambivalent.

This was one of many earlier suggestions, and the consensus seemed to
be "don't mix security= and lsm=". Why would anyone use both?
Kees Cook Feb. 8, 2019, 9:49 p.m. UTC | #9
On Fri, Feb 8, 2019 at 2:52 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2019/02/08 1:24, Casey Schaufler wrote:
> >>>> Then, I think that it is straightforward (and easier to manage) to ignore security= parameter
> >>>> when lsm= parameter is specified.
> >>> That reduces flexibility somewhat. If I am debugging security modules
> >>> I may want to use lsm= to specify the order while using security= to
> >>> identify a specific exclusive module. I could do that using lsm= by
> >>> itself, but habits die hard.
> >> "lsm=" can be used for identifying a specific exclusive module, and Ubuntu kernels would
> >> have to use CONFIG_LSM (or "lsm=") for identifying the default exclusive module (in order
> >> to allow enabling both TOMOYO and one of SELinux,Smack,AppArmor at the same time).
> >>
> >> Since "security=" can't be used for selectively enable/disable more than one of
> >> SELinux,Smack,TOMOYO,AppArmor, I think that recommending users to migrate to "lsm=" is the
> >> better direction. And ignoring "security=" when "lsm=" is specified is easier to understand.
> >
> > I added Kees to the CC list. Kees, what to you think about
> > ignoring security= if lsm= is specified? I'm ambivalent.
> >
> >
>
> To help administrators easily understand what LSM modules are possibly enabled by default (which
> have to be fetched from e.g. /boot/config-`uname -r`) and specify lsm= parameter when they need,
> I propose changes shown below.
>
> diff --git a/security/security.c b/security/security.c
> index 3147785e..051d708 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -51,8 +51,6 @@
>  static __initdata const char *chosen_lsm_order;
>  static __initdata const char *chosen_major_lsm;
>
> -static __initconst const char * const builtin_lsm_order = CONFIG_LSM;
> -
>  /* Ordered list of LSMs to initialize. */
>  static __initdata struct lsm_info **ordered_lsms;
>  static __initdata struct lsm_info *exclusive;
> @@ -284,14 +282,22 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>  static void __init ordered_lsm_init(void)
>  {
>         struct lsm_info **lsm;
> +       const char *order = CONFIG_LSM;
> +       const char *origin = "builtin";
>
>         ordered_lsms = kcalloc(LSM_COUNT + 1, sizeof(*ordered_lsms),
>                                 GFP_KERNEL);
>
> -       if (chosen_lsm_order)
> -               ordered_lsm_parse(chosen_lsm_order, "cmdline");
> -       else
> -               ordered_lsm_parse(builtin_lsm_order, "builtin");
> +       if (chosen_lsm_order) {
> +               if (chosen_major_lsm) {
> +                       pr_info("security= is ignored because of lsm=\n");

This is intended to be the new default way to change the LSM
("lsm=..."), so I'd rather not have this appear every time. Also, it
must continue to interact with the builtin ordering, so if you wanted
this, I think better would be to do:

diff --git a/security/security.c b/security/security.c
index 3147785e20d7..e6153ed54361 100644
--- a/security/security.c
+++ b/security/security.c
@@ -288,9 +288,13 @@ static void __init ordered_lsm_init(void)
        ordered_lsms = kcalloc(LSM_COUNT + 1, sizeof(*ordered_lsms),
                                GFP_KERNEL);

-       if (chosen_lsm_order)
+       if (chosen_lsm_order) {
+               if (chosen_major_lsm) {
+                       pr_info("security= is ignored because of lsm=\n");
+                       chosen_major_lsm = NULL;
+               }
                ordered_lsm_parse(chosen_lsm_order, "cmdline");
-       else
+       } else
                ordered_lsm_parse(builtin_lsm_order, "builtin");

        for (lsm = ordered_lsms; *lsm; lsm++)

> +       pr_info("Security Framework initializing: %s\n", order);
> +       ordered_lsm_parse(order, origin);
>
>         for (lsm = ordered_lsms; *lsm; lsm++)
>                 prepare_lsm(*lsm);
> @@ -333,8 +339,6 @@ int __init security_init(void)
>         int i;
>         struct hlist_head *list = (struct hlist_head *) &security_hook_heads;
>
> -       pr_info("Security Framework initializing\n");
> -
>         for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct hlist_head);
>              i++)
>                 INIT_HLIST_HEAD(&list[i]);
Tetsuo Handa Feb. 9, 2019, 12:28 a.m. UTC | #10
On 2019/02/09 1:23, Casey Schaufler wrote:
> On 2/8/2019 2:52 AM, Tetsuo Handa wrote:
>> To help administrators easily understand what LSM modules are possibly enabled by default (which
>> have to be fetched from e.g. /boot/config-`uname -r`)
> 
> $ cat /sys/kernel/security/lsm
> 

/sys/kernel/security/lsm is list of "actually" enabled modules, isn't it?
What I want is "possibly" enabled modules. Ubuntu would chose from either

  (a) explicitly add security=apparmor to kernel command line

or

  (b) explicitly remove tomoyo from CONFIG_LSM at kernel config

in order not to enable TOMOYO for those who want to enable only one of 
SELinux/Smack/AppArmor. And for those who want to enable TOMOYO, I think
that (b) (in other words, add

  lsm="modules listed in CONFIG_LSM" + ",tomoyo"

) will retain compatibility when it becomes possible to enable more than
one of SELinux/Smack/AppArmor at the same time.

If we can know "possibly" enabled modules from dmesg, users don't need to
look at e.g. /boot/config-`uname -r`. It is not essential, but it's handy.
Tetsuo Handa Feb. 9, 2019, 1:40 a.m. UTC | #11
On 2019/02/09 9:28, Tetsuo Handa wrote:
> On 2019/02/09 1:23, Casey Schaufler wrote:
>> On 2/8/2019 2:52 AM, Tetsuo Handa wrote:
>>> To help administrators easily understand what LSM modules are possibly enabled by default (which
>>> have to be fetched from e.g. /boot/config-`uname -r`)
>>
>> $ cat /sys/kernel/security/lsm
>>
> 
> /sys/kernel/security/lsm is list of "actually" enabled modules, isn't it?
> What I want is "possibly" enabled modules. Ubuntu would chose from either
> 
>   (a) explicitly add security=apparmor to kernel command line
> 
> or
> 
>   (b) explicitly remove tomoyo from CONFIG_LSM at kernel config
> 
> in order not to enable TOMOYO for those who want to enable only one of 
> SELinux/Smack/AppArmor. And for those who want to enable TOMOYO, I think
> that (b) (in other words, add
> 
>   lsm="modules listed in CONFIG_LSM" + ",tomoyo"
> 
> ) will retain compatibility when it becomes possible to enable more than
> one of SELinux/Smack/AppArmor at the same time.
> 
> If we can know "possibly" enabled modules from dmesg, users don't need to
> look at e.g. /boot/config-`uname -r`. It is not essential, but it's handy.
> 

Well, thinking again, specifying

lsm="modules listed in /sys/kernel/security/lsm" + ",tomoyo"

makes sense, for there is no need to care about disabled modules when
enabling TOMOYO. Therefore,

+	pr_info("Security Framework initializing: %s\n", order);
-	pr_info("Security Framework initializing\n");

won't be needed.

On 2019/02/09 6:33, Kees Cook wrote:
> On Thu, Feb 7, 2019 at 8:24 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> I added Kees to the CC list. Kees, what to you think about
>> ignoring security= if lsm= is specified? I'm ambivalent.
> 
> This was one of many earlier suggestions, and the consensus seemed to
> be "don't mix security= and lsm=". Why would anyone use both?
> 

Then, can we add this change?

+	if (chosen_lsm_order) {
+		if (chosen_major_lsm) {
+			pr_info("security= is ignored because of lsm=\n");
+			chosen_major_lsm = NULL;
+		}
+	}

Patch
diff mbox series

diff --git a/security/security.c b/security/security.c
index ef03643..0632feb 100644
--- a/security/security.c
+++ b/security/security.c
@@ -346,12 +346,14 @@  int __init security_init(void)
 }
 
 /* Save user chosen LSM */
+#ifndef CONFIG_DEBUG_AID_FOR_SYZBOT
 static int __init choose_major_lsm(char *str)
 {
 	chosen_major_lsm = str;
 	return 1;
 }
 __setup("security=", choose_major_lsm);
+#endif
 
 /* Explicitly choose LSM initialization order. */
 static int __init choose_lsm_order(char *str)