| Message ID | 52531a69-10ed-d263-be66-e707705597d6@i-love.sakura.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM: Allow syzbot to ignore security= parameter. | expand |
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 >
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.
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.
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.
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.
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]);
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.
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?
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]);
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.
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; + } + }
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)