On 24/04/2023 10:52, xiujianfeng wrote:
On 2023/4/21 1:40, Mickaël Salaün wrote:
On 18/04/2023 12:53, xiujianfeng wrote:
Hi Mickael,
Sorry about the long silence on this work, As we known this work depends on another work about changing argument from struct dentry to struct path for some attr/xattr related lsm hooks, I'm stuck with this thing, because IMA/EVM is a special security module which is not LSM-based currently, and severely coupled with the file system. so I am waiting for Roberto Sassu' work (Move IMA and EVM to the LSM infrastructure) to be ready, I think it can make my work more easy. you can find Roberto'work here, https://lwn.net/ml/linux-kernel/20230303181842.1087717-1-roberto.sassu@huawe...
Any good idea are welcome, thanks.
Thanks for the update Xiu.
Which part would be needed from Roberto's patch series?
As we discussed before, the two access rights that need to be added and their usage is as below: LANDLOCK_ACCESS_FS_WRITE_METADATA controls 1.inode_setattr 2.inode_setxattr 3.inode_removexattr 4.inode_set_acl 5.inode_remove_acl LANDLOCK_ACCESS_FS_READ_METADATA controls 1.inode_getattr 2.inode_get_acl 3.inode_getxattr 4.inode_listxattr
all these APIs should be changed to use struct path instead of dentry, and then several vfs APIs as follows are invovled: notify_change, __vfs_setxattr_locked, __vfs_removexattr_locked, __vfs_setxattr_noperm vfs_set_acl vfs_remove_acl vfs_getxattr vfs_listxattr vfs_get_acl and also include some LSM hooks such as inode_post_setxattr and inode_setsecctx.
Since the original places where pass dentry to security_inode_xxx may not have any struct path, we have to pass it from the top caller, so this also touches lots of filesystems(e.g. cachefiles, ecryptfs, ksmbd, nfsd, overlayfs...).
Other LSMs such as selinux, smack can be easy to refator because they are LSM-based, and if VFS passes path to security_inode_xxx and they can just use path->dentry instead inside they own modules.
AS for IMA/EVM, unfortunately they are not LSM-based and coupled with the file system. To make things worse, there is a recursive dependency situation during the update of extended attribute which happen as follows:
__vfs_setxattr_noperm => security_inode_post_setxattr => evm_inode_post_setxattr => evm_update_evmxattr => __vfs_setxattr_noperm
To change the argument of __vfs_setxattr_noperm from a dentry to the path structure, the two EVM functions would have to be altered as well. However, evm_update_evmxattr is called by 3 other EVM functions who lives in the very heart of the complicated EVM framework. Any change to them would cause a nasty chain reaction in EVM and, as IMA would trigger EVM directly, in IMA as well.
There is another callchain as follow: ima_appraise_measurement =>evm_verifyxattr =>evm_verifyxattr =>evm_verify_hmac =>evm_calc_hash =>evm_calc_hmac_or_hash =>vfs_getxattr Passing struct path into vfs_getxattr() would also affect this callchain. Currently ima_appraise_measurment accepts a struct file, and dentry is generated from file_dentry(file) in order to mitigate a deadlock issue involving overlayfs(commit e71b9dff0634ed). Once &file->f_path is passed through this callchain, and someone wants the dentry, it will be using file->f_path.dentry, which is different from file_dentry(file). In the overlayfs scenario, may this cause an issue?
I might be OK, but this need to be tested.
The patchset of moving IMA and EVM into the LSM infrastructe would be helpfull but still can not completely resolve this situation. more refactor would be needed in EVM. That's all that's happening right now.
OK, thanks for the detailed explanation!
I guess you could start with easier hooks (e.g. inode_getattr and inode_setattr) to see if there is potentially other implications, and incrementally build on that.
On 2022/8/27 19:12, Xiu Jianfeng wrote:
v2: * abstract walk_to_visible_parent() helper * chmod and chown rights only take affect on directory's context * add testcase for fchmodat/lchown/fchownat * fix other review issues
Xiu Jianfeng (6): landlock: expand access_mask_t to u32 type landlock: abstract walk_to_visible_parent() helper landlock: add chmod and chown support landlock/selftests: add selftests for chmod and chown landlock/samples: add chmod and chown support landlock: update chmod and chown support in document
Documentation/userspace-api/landlock.rst | 9 +- include/uapi/linux/landlock.h | 10 +- samples/landlock/sandboxer.c | 13 +- security/landlock/fs.c | 110 ++++++-- security/landlock/limits.h | 2 +- security/landlock/ruleset.h | 2 +- security/landlock/syscalls.c | 2 +- tools/testing/selftests/landlock/base_test.c | 2 +- tools/testing/selftests/landlock/fs_test.c | 267 ++++++++++++++++++- 9 files changed, 386 insertions(+), 31 deletions(-)