Messages in this thread | | | Date | Wed, 26 Apr 2023 15:58:31 +0200 | Subject | Re: [PATCH -next v2 0/6] landlock: add chmod and chown support | From | Mickaël Salaün <> |
| |
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@huaweicloud.com/ >>> >>> 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(-) >>>>
| |