On Tue, 2023-03-14 at 09:17 +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > One of the major goals of LSM stacking is to run multiple LSMs side by side > without interfering with each other. The ultimate decision will depend on > individual LSM decision. > > Several changes need to be made to the LSM infrastructure to be able to > support that. This patch set tackles one of them: gives to each LSM the > ability to specify one or multiple xattrs to be set at inode creation > time and, at the same time, gives to EVM the ability to access all those > xattrs and calculate the HMAC on them. > > The first problem that this patch set addresses is to make the > inode_init_security hook definition suitable to use with EVM which, unlike > other LSMs, needs to have visibility of all xattrs and not only the one > that the LSM infrastructure passes to the LSM to be set. > > The solution is to replace in the inode_init_security definition the > name/value/len parameters with the beginning of the array containing all > xattrs set by LSMs. Due to security_old_inode_init_security() API > limitation of setting only one xattr, it has been dropped and the remaining > users, ocfs2 and reiserfs, switch to security_inode_init_security(). > However, due to the complexity of the changes required to fully exploit the > ability of security_inode_init_security() to set multiple xattrs, those > users can still set only one xattr (the first set in the xattr array) where > previously they called security_old_inode_init_security(). > > Furthermore, while EVM is invoked unlike before, its xattr will not be set > as it would not be the first set in the xattr array, or if it is the first, > there would not be protected xattrs to calculate the HMAC on. > > The second problem this patch set addresses is the limitation of the > call_int_hook() of stopping the loop when the return value from a hook > implementation is not zero. Unfortunately, for the inode_init_security hook > it is a legitimate case to return -EOPNOTSUPP, but this would not > necessarily mean that there is an error to report to the LSM infrastructure > but just that an LSM does not will to set an xattr. Other LSMs should be > still consulted as well. > > The solution for this specific case is to replace the call_int_hook() with > the loop itself, so that -EOPNOTSUPP can be ignored. > > Next, this patch set removes the limitation of creating only two xattrs, > one by an active LSM and another by EVM. This patch set extends the > reservation mechanism of the LSM infrastructure, to allow each LSM to > request one or multiple xattrs. While this could potentially lead to > reaching the filesystem limits of number/size of the xattrs, it seems not > an issue that need to be solved by the LSM infrastructure but by the > filesystems themselves. Currently, if the limit is reached, the only > workaround would be to use fewer LSMs. > > The reservation mechanism concept makes it very easy for LSMs to position > themselves correctly in the xattr array, as the LSM infrastructure at > initialization time changes the number of xattrs requested by each LSM with > an offset. LSMs can just take that offset as the starting index in the > xattr array and fill the next slots depending on how many xattrs they > requested. > > However, while this concept is intuitive, it needs extra care. While for > security blobs (the main reason of the reservation mechanism) it is not > relevant for an LSM if other LSMs filled their portion, it matters for > xattrs, as both EVM and initxattrs() callbacks scan the entire array until > a terminator (xattr with NULL name). If an LSM did not provide an xattr, > which could happen if it is loaded but not initialized, consumers of the > xattr array would stop prematurely. > > This patch set avoids this problem by compacting the xattr array each time > after an LSM executed its implementation of the inode_init_security hook. > It needs to be done after each LSM, and not after all, since there might be > LSMs scanning that xattr array too. Compacting the array after all LSMs > would be too late. > > Finally, this patch set modifies the evm_inode_init_security() definition > to be compatible with the inode_init_security hook definition and adds > support for scanning the whole xattr array and for calculating the HMAC > on all xattrs provided by LSMs. > > This patch set has been tested by introducing several instances of a > TestLSM (some providing an xattr, some not, one with a wrong implementation > to see how the LSM infrastructure handles it, one providing multiple xattrs > and another providing an xattr but in a disabled state). The patch is not > included in this set but it is available here: > > https://github.com/robertosassu/linux/commit/311b83a98757915cc3ccb363f545578e7c38df54 > > The test, added to ima-evm-utils, is available here: > > https://github.com/robertosassu/ima-evm-utils/blob/evm-multiple-lsms-v8-devel-v2/tests/evm_multiple_lsms.test > > The test takes a UML kernel built by Github Actions and launches it several > times, each time with a different combination of LSMs and filesystems (ext4, > reiserfs, ocfs2). After boot, it first checks that there is an xattr for each > LSM providing it (for reiserfs and ocfs2 just the first LSM), and then (for > ext4) calculates the HMAC in user space and compares it with the HMAC > calculated by EVM in kernel space. > > A test report can be obtained here: > > https://github.com/robertosassu/ima-evm-utils/actions/runs/4403223954/jobs/7722101662 > > The patch set has been tested with both the SElinux and Smack test suites. > Below, there is the summary of the test results: > > SELinux Test Suite result (without patches): > All tests successful. > Files=76, Tests=1343, 255 wallclock secs ( 0.51 usr 0.19 sys + 6.30 cusr 58.15 csys = 65.15 CPU) > Result: PASS > > SELinux Test Suite result (with patches): > All tests successful. > Files=76, Tests=1343, 258 wallclock secs ( 0.49 usr 0.21 sys + 6.46 cusr 59.27 csys = 66.43 CPU) > Result: PASS Side note: https://github.com/SELinuxProject/selinux-testsuite/blob/master/tests/mmap/test#L51 This test fails if CONFIG_LSM_MMAP_MIN_ADDR=0 Roberto > Smack Test Suite result (without patches): > 95 Passed, 0 Failed, 100% Success rate > > Smack Test Suite result (with patches): > 95 Passed, 0 Failed, 100% Success rate > > Changelog > > v7: > - Add a patch dependency comment in patch 1 (suggested by Mimi) > - Restore check of -EOPNOTSUPP status in ocfs2_mknod() and ocfs2_symlink() > (reported by Mimi) > - Add explanation in evm_inode_init_security() why walking through the > xattrs array is safe (suggested by Mimi) > - Document the lbs_xattr field of struct lsm_blob_sizes (suggested by > Casey) > - Move documentation changes of the inode_init_security hook to security.c, > after LSM documentation reorganization by Paul > - Use attributes in plural form in the description of the xattrs parameter > of smack_inode_init_security() > - Check xattr name instead of xattr value in evm_inode_init_security(), > for consistency with evm_init_hmac(); equivalent, since > security_check_compact_filled_xattrs() rejects xattrs with xattr name > NULL and value not NULL, and viceversa > > v6: > - Add a comment in Smack to introduce its xattrs (suggested by Casey) > - Document the overloaded meaning of -EOPNOTSUPP in > security_inode_init_security() (suggested by Mimi) > > v5: > - Modify the cover letter to explain that the goal of this patch set is > supporting multiple per LSM xattrs in EVM, and not moving IMA and EVM to > the LSM infrastructure (suggested by Mimi) > - Remove references in the patches description about moving IMA and EVM > to the LSM infrastructure (suggested by Mimi) > - Explain that the additional EVM invocation due to the switch to > security_inode_init_security() will not cause the EVM xattr to be added > (suggested by Mimi) > > v4: > - Remove patch to call reiserfs_security_free(), already queued > - Switch ocfs2 and reiserfs to security_inode_init_security() (suggested by > Mimi) > - Remove security_old_inode_init_security() (suggested by Paul) > - Rename security_check_compact_xattrs() to > security_check_compact_filled_xattrs() and add function description > (suggested by Mimi) > - Rename checked_xattrs parameter of security_check_compact_filled_xattrs() > to num_filled_xattrs (suggested by Mimi) > - Rename cur_xattrs variable in security_inode_init_security() to > num_filled_xattrs (suggested by Mimi) > > v3: > - Don't free the xattr name in reiserfs_security_free() > - Don't include fs_data parameter in inode_init_security hook > - Don't change evm_inode_init_security(), as it will be removed if EVM is > stacked > - Fix inode_init_security hook documentation > - Drop lsm_find_xattr_slot(), use simple xattr reservation mechanism and > introduce security_check_compact_xattrs() to compact the xattr array > - Don't allocate xattr array if LSMs didn't reserve any xattr > - Return zero if initxattrs() is not provided to > security_inode_init_security(), -EOPNOTSUPP if value is not provided to > security_old_inode_init_security() > - Request LSMs to fill xattrs if only value (not the triple) is provided to > security_old_inode_init_security(), to avoid unnecessary memory > allocation > > v2: > - rewrite selinux_old_inode_init_security() to use > security_inode_init_security() > - add lbs_xattr field to lsm_blob_sizes structure, to give the ability to > LSMs to reserve slots in the xattr array (suggested by Casey) > - add new parameter base_slot to inode_init_security hook definition > > v1: > - add calls to reiserfs_security_free() and initialize sec->value to NULL > (suggested by Tetsuo and Mimi) > - change definition of inode_init_security hook, replace the name, value > and len triple with the xattr array (suggested by Casey) > - introduce lsm_find_xattr_slot() helper for LSMs to find an unused slot in > the passed xattr array > > Roberto Sassu (6): > reiserfs: Switch to security_inode_init_security() > ocfs2: Switch to security_inode_init_security() > security: Remove security_old_inode_init_security() > security: Allow all LSMs to provide xattrs for inode_init_security > hook > evm: Align evm_inode_init_security() definition with LSM > infrastructure > evm: Support multiple LSMs providing an xattr > > fs/ocfs2/namei.c | 2 + > fs/ocfs2/xattr.c | 30 ++++++- > fs/reiserfs/xattr_security.c | 23 +++-- > include/linux/evm.h | 12 +-- > include/linux/lsm_hook_defs.h | 3 +- > include/linux/lsm_hooks.h | 1 + > include/linux/security.h | 12 --- > security/integrity/evm/evm.h | 2 + > security/integrity/evm/evm_crypto.c | 9 +- > security/integrity/evm/evm_main.c | 33 +++++-- > security/security.c | 131 +++++++++++++++++++++------- > security/selinux/hooks.c | 19 ++-- > security/smack/smack_lsm.c | 33 ++++--- > 13 files changed, 224 insertions(+), 86 deletions(-) >
