| Message ID | 20250409185019.238841-31-paul@paul-moore.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Rework the LSM initialization | expand |
On 4/9/2025 11:49 AM, Paul Moore wrote: > This is one of those patchsets that started out small and then quickly > expanded to what you see here. I will warn you that some of the > individual patches are a bit ugly to look at, but I believe the end > result is much cleaner than what we have now, fixes some odd/undesirable > behavior on boot, and enables some new functionality. > > The most obvious changes are the extraction of the LSM notifier and > initialization code out of security/security.c and into their own files, > security/lsm_notifier.c and security/lsm_init.c. While not strictly > necessary, I think we can all agree that security/security.c has grown > to be a bit of a mess, and these are two bits of functionality which > can be extracted out into their own files without too much fuss. I > personally find this to be a nice quality-of-life improvement, and while > I'm open to keeping everything in security.c, the argument for doing so > is going to need to be *very* persuasive. It's something I've considered doing as part of the stacking work, but that I have eschewed in the spirit of churn reduction. I've no problem with it. > The other significant change is moving all of the LSM initcalls into the > LSM framework. While I've always pushed to keep the LSM framework as > minimal as possible, there are some things that we really can't defer to > the individual LSMs and with the LSM framework responsible for enabling > or disabling the individual LSMs at boot, I believe management and > execution of the LSM initcalls needs to be handled in the framework as > well. Not only does this move ensure that we aren't running initcalls > for LSMs which are disabled, it also provides us with a convenient spot > to signal when all of the LSMs have been actived (see the LSM_STARTED_ALL > patch towards the end of the patchset). This is not a feature we > currently need, but I'm aware of some future work that does require this > so I thought it would be good to think about it now while doing this > work. > > Related to the LSM_STARTED_ALL patch, the final patch in this series > adds support for LSMs to indicate if they provide lsm_prop values for > subjects and/or objects. Casey needs this functionality for his recent > audit changes, and I personally find the counting approach presented > here to be ... less ugly I guess? The flags approach works for me. I was going to propose adding a call audit_lsm_secctx() that LSMs would call to identify that a secctx was being supported, but I had considered the flag approach as well. As for ugly, I can't say one way or the other. > This patchset is marked as a RFC for a number of reasons: additional > testing is required, the commit descriptions could benefit from some > extra attention, and I still have hopes that some of the individual > patches could be cleaned up a bit (I still like the end result, but how > we get there could be improved). I would really appreciate if the > individual LSM maintainers could give this a quick look, especially > the individual LSM patches that move the initcalls into the LSM > framework as some of those are non-trivial. General comments: Adjacent patches with no more commit message than "cleanup" should be combined, as that message is telling me "these aren't the changes you're looking for". And about that. I believe that missing or uninformative commit messages are on your list of things that displease you. You will need to improve them to get them past yourself. :) There's a lot of churn here due to unnecessary name changes. I can't say they're unjustified, but the patch set is bigger than it needs to be, and more disruptive. I haven't tested it, but I don't see any substantial problems so far. > Mimi and Roberto, the > IMA/EVM work here was particularly "fun"; from what I've seen thus far > it appears to work correctly, but I have no idea if that code is good > or bad from you perspective. It's perfectly okay if you want to > reject the approach taken in IMA/EVM, but we do need to move the > initcalls up to the LSM framework, so please suggest some code that > would allow us to do that for IMA/EVM. > > -- > Paul Moore (29): > lsm: split the notifier code out into lsm_notifier.c > lsm: split the init code out into lsm_init.c > lsm: simplify prepare_lsm() and rename to lsm_prep_single() > lsm: simplify ordered_lsm_init() and rename to lsm_init_ordered() > lsm: replace the name field with a pointer to the lsm_id struct > lsm: cleanup and normalize the LSM order symbols naming > lsm: rework lsm_active_cnt and lsm_idlist[] > lsm: get rid of the lsm_names list and do some cleanup > lsm: cleanup and normalize the LSM enabled functions > lsm: cleanup the LSM blob size code > lsm: cleanup initialize_lsm() and rename to lsm_init_single() > lsm: cleanup the LSM ordered parsing > lsm: fold lsm_init_ordered() into security_init() > lsm: add missing function header comment blocks in lsm_init.c > lsm: cleanup the debug and console output in lsm_init.c > lsm: output available LSMs when debugging > lsm: introduce an initcall mechanism into the LSM framework > loadpin: move initcalls to the LSM framework > ipe: move initcalls to the LSM framework > smack: move initcalls to the LSM framework > tomoyo: move initcalls to the LSM framework > safesetid: move initcalls to the LSM framework > apparmor: move initcalls to the LSM framework > lockdown: move initcalls to the LSM framework > ima,evm: move initcalls to the LSM framework > selinux: move initcalls to the LSM framework > lsm: consolidate all of the LSM framework initcalls > lsm: add a LSM_STARTED_ALL notification event > lsm: add support for counting lsm_prop support among LSMs > > include/linux/lsm_hooks.h | 73 - > include/linux/security.h | 3 > security/Makefile | 2 > security/apparmor/apparmorfs.c | 4 > security/apparmor/crypto.c | 4 > security/apparmor/include/apparmorfs.h | 2 > security/apparmor/include/crypto.h | 1 > security/apparmor/lsm.c | 12 > security/bpf/hooks.c | 3 > security/commoncap.c | 3 > security/inode.c | 29 > security/integrity/Makefile | 2 > security/integrity/evm/evm_main.c | 10 > security/integrity/iint.c | 4 > security/integrity/ima/ima_main.c | 10 > security/integrity/ima/ima_mok.c | 4 > security/integrity/initcalls.c | 97 + > security/integrity/initcalls.h | 23 > security/integrity/platform_certs/load_ipl_s390.c | 4 > security/integrity/platform_certs/load_powerpc.c | 4 > security/integrity/platform_certs/load_uefi.c | 4 > security/integrity/platform_certs/machine_keyring.c | 4 > security/integrity/platform_certs/platform_keyring.c | 14 > security/ipe/fs.c | 4 > security/ipe/ipe.c | 4 > security/ipe/ipe.h | 2 > security/landlock/setup.c | 3 > security/loadpin/loadpin.c | 16 > security/lockdown/lockdown.c | 6 > security/lsm.h | 46 > security/lsm_init.c | 566 ++++++++++ > security/lsm_notifier.c | 31 > security/lsm_syscalls.c | 8 > security/min_addr.c | 5 > security/safesetid/lsm.c | 4 > security/safesetid/lsm.h | 2 > security/safesetid/securityfs.c | 3 > security/security.c | 620 ----------- > security/selinux/Makefile | 2 > security/selinux/hooks.c | 12 > security/selinux/ibpkey.c | 5 > security/selinux/include/audit.h | 5 > security/selinux/include/initcalls.h | 19 > security/selinux/initcalls.c | 50 > security/selinux/netif.c | 5 > security/selinux/netlink.c | 5 > security/selinux/netnode.c | 5 > security/selinux/netport.c | 5 > security/selinux/selinuxfs.c | 5 > security/selinux/ss/services.c | 26 > security/smack/smack.h | 6 > security/smack/smack_lsm.c | 19 > security/smack/smack_netfilter.c | 4 > security/smack/smackfs.c | 4 > security/tomoyo/common.h | 2 > security/tomoyo/securityfs_if.c | 4 > security/tomoyo/tomoyo.c | 4 > security/yama/yama_lsm.c | 3 > 58 files changed, 1102 insertions(+), 724 deletions(-) >
On Thu, Apr 10, 2025 at 07:13:11AM -0700, Casey Schaufler wrote: > On 4/9/2025 11:49 AM, Paul Moore wrote: > > This is one of those patchsets that started out small and then quickly > > expanded to what you see here. I will warn you that some of the > > individual patches are a bit ugly to look at, but I believe the end > > result is much cleaner than what we have now, fixes some odd/undesirable > > behavior on boot, and enables some new functionality. > > > > The most obvious changes are the extraction of the LSM notifier and > > initialization code out of security/security.c and into their own files, > > security/lsm_notifier.c and security/lsm_init.c. While not strictly > > necessary, I think we can all agree that security/security.c has grown > > to be a bit of a mess, and these are two bits of functionality which > > can be extracted out into their own files without too much fuss. I > > personally find this to be a nice quality-of-life improvement, and while > > I'm open to keeping everything in security.c, the argument for doing so > > is going to need to be *very* persuasive. > > It's something I've considered doing as part of the stacking work, > but that I have eschewed in the spirit of churn reduction. I've no > problem with it. Yeah, to be clear, I'm a fan of these refactorings. :) > There's a lot of churn here due to unnecessary name changes. I can't > say they're unjustified, but the patch set is bigger than it needs to > be, and more disruptive. If renamings are desired, sure, let's do it, but I'd love to see them very distinctly separated from logical changes.
On Thu, Apr 10, 2025 at 10:13 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 4/9/2025 11:49 AM, Paul Moore wrote: ... > General comments: > > Adjacent patches with no more commit message than "cleanup" should > be combined, as that message is telling me "these aren't the changes > you're looking for". Things have been shuffled around quite a bit since this posting, and I expect there will likely be a few more adjustments before a v2 is posted. > And about that. I believe that missing or uninformative commit messages > are on your list of things that displease you. You will need to improve > them to get them past yourself. :) You'll notice that I highlighted the garbage commit messages in the list of things that made this a RFC patch. I'm well aware that this is a big problem in this patchset, but I know there are individuals on the LSM mailing list who have been anxiously awaiting a peek at this work, so I made a decision to post a very crude revision to satisfy that curiosity. If you can't appreciate that decision, I hope that you can at least understand it ;) While I hope to never post a proper (read "non RFC") patchset with such trash for commit messages, if I do, I would hope and expect that all of you wouldn't hesitate to chastise me! > There's a lot of churn here due to unnecessary name changes. I can't > say they're unjustified, but the patch set is bigger than it needs to > be, and more disruptive. Perhaps, but there was some pretty awful code, with some pretty awful names, in the initialization routines and if I was going to spend the time to clean it all up I felt the renames were justified. If I'm ever going to pull a "maintainer's privilege" card, it would probably be over stuff like this; I know it's trivial, and churns the code, but I can't tell you how much it bothers me when I keep reading/reviewing code with awful names. That's probably why one of my chief nitpicks with a lot of patches comes back to naming. > I haven't tested it, but I don't see any substantial problems so far. I appreciate the review, I know it's not an easy patchset to look at. The next revision should be cleaner.
This is one of those patchsets that started out small and then quickly expanded to what you see here. I will warn you that some of the individual patches are a bit ugly to look at, but I believe the end result is much cleaner than what we have now, fixes some odd/undesirable behavior on boot, and enables some new functionality. The most obvious changes are the extraction of the LSM notifier and initialization code out of security/security.c and into their own files, security/lsm_notifier.c and security/lsm_init.c. While not strictly necessary, I think we can all agree that security/security.c has grown to be a bit of a mess, and these are two bits of functionality which can be extracted out into their own files without too much fuss. I personally find this to be a nice quality-of-life improvement, and while I'm open to keeping everything in security.c, the argument for doing so is going to need to be *very* persuasive. The other significant change is moving all of the LSM initcalls into the LSM framework. While I've always pushed to keep the LSM framework as minimal as possible, there are some things that we really can't defer to the individual LSMs and with the LSM framework responsible for enabling or disabling the individual LSMs at boot, I believe management and execution of the LSM initcalls needs to be handled in the framework as well. Not only does this move ensure that we aren't running initcalls for LSMs which are disabled, it also provides us with a convenient spot to signal when all of the LSMs have been actived (see the LSM_STARTED_ALL patch towards the end of the patchset). This is not a feature we currently need, but I'm aware of some future work that does require this so I thought it would be good to think about it now while doing this work. Related to the LSM_STARTED_ALL patch, the final patch in this series adds support for LSMs to indicate if they provide lsm_prop values for subjects and/or objects. Casey needs this functionality for his recent audit changes, and I personally find the counting approach presented here to be ... less ugly I guess? This patchset is marked as a RFC for a number of reasons: additional testing is required, the commit descriptions could benefit from some extra attention, and I still have hopes that some of the individual patches could be cleaned up a bit (I still like the end result, but how we get there could be improved). I would really appreciate if the individual LSM maintainers could give this a quick look, especially the individual LSM patches that move the initcalls into the LSM framework as some of those are non-trivial. Mimi and Roberto, the IMA/EVM work here was particularly "fun"; from what I've seen thus far it appears to work correctly, but I have no idea if that code is good or bad from you perspective. It's perfectly okay if you want to reject the approach taken in IMA/EVM, but we do need to move the initcalls up to the LSM framework, so please suggest some code that would allow us to do that for IMA/EVM. -- Paul Moore (29): lsm: split the notifier code out into lsm_notifier.c lsm: split the init code out into lsm_init.c lsm: simplify prepare_lsm() and rename to lsm_prep_single() lsm: simplify ordered_lsm_init() and rename to lsm_init_ordered() lsm: replace the name field with a pointer to the lsm_id struct lsm: cleanup and normalize the LSM order symbols naming lsm: rework lsm_active_cnt and lsm_idlist[] lsm: get rid of the lsm_names list and do some cleanup lsm: cleanup and normalize the LSM enabled functions lsm: cleanup the LSM blob size code lsm: cleanup initialize_lsm() and rename to lsm_init_single() lsm: cleanup the LSM ordered parsing lsm: fold lsm_init_ordered() into security_init() lsm: add missing function header comment blocks in lsm_init.c lsm: cleanup the debug and console output in lsm_init.c lsm: output available LSMs when debugging lsm: introduce an initcall mechanism into the LSM framework loadpin: move initcalls to the LSM framework ipe: move initcalls to the LSM framework smack: move initcalls to the LSM framework tomoyo: move initcalls to the LSM framework safesetid: move initcalls to the LSM framework apparmor: move initcalls to the LSM framework lockdown: move initcalls to the LSM framework ima,evm: move initcalls to the LSM framework selinux: move initcalls to the LSM framework lsm: consolidate all of the LSM framework initcalls lsm: add a LSM_STARTED_ALL notification event lsm: add support for counting lsm_prop support among LSMs include/linux/lsm_hooks.h | 73 - include/linux/security.h | 3 security/Makefile | 2 security/apparmor/apparmorfs.c | 4 security/apparmor/crypto.c | 4 security/apparmor/include/apparmorfs.h | 2 security/apparmor/include/crypto.h | 1 security/apparmor/lsm.c | 12 security/bpf/hooks.c | 3 security/commoncap.c | 3 security/inode.c | 29 security/integrity/Makefile | 2 security/integrity/evm/evm_main.c | 10 security/integrity/iint.c | 4 security/integrity/ima/ima_main.c | 10 security/integrity/ima/ima_mok.c | 4 security/integrity/initcalls.c | 97 + security/integrity/initcalls.h | 23 security/integrity/platform_certs/load_ipl_s390.c | 4 security/integrity/platform_certs/load_powerpc.c | 4 security/integrity/platform_certs/load_uefi.c | 4 security/integrity/platform_certs/machine_keyring.c | 4 security/integrity/platform_certs/platform_keyring.c | 14 security/ipe/fs.c | 4 security/ipe/ipe.c | 4 security/ipe/ipe.h | 2 security/landlock/setup.c | 3 security/loadpin/loadpin.c | 16 security/lockdown/lockdown.c | 6 security/lsm.h | 46 security/lsm_init.c | 566 ++++++++++ security/lsm_notifier.c | 31 security/lsm_syscalls.c | 8 security/min_addr.c | 5 security/safesetid/lsm.c | 4 security/safesetid/lsm.h | 2 security/safesetid/securityfs.c | 3 security/security.c | 620 ----------- security/selinux/Makefile | 2 security/selinux/hooks.c | 12 security/selinux/ibpkey.c | 5 security/selinux/include/audit.h | 5 security/selinux/include/initcalls.h | 19 security/selinux/initcalls.c | 50 security/selinux/netif.c | 5 security/selinux/netlink.c | 5 security/selinux/netnode.c | 5 security/selinux/netport.c | 5 security/selinux/selinuxfs.c | 5 security/selinux/ss/services.c | 26 security/smack/smack.h | 6 security/smack/smack_lsm.c | 19 security/smack/smack_netfilter.c | 4 security/smack/smackfs.c | 4 security/tomoyo/common.h | 2 security/tomoyo/securityfs_if.c | 4 security/tomoyo/tomoyo.c | 4 security/yama/yama_lsm.c | 3 58 files changed, 1102 insertions(+), 724 deletions(-)