lkml.org 
[lkml]   [2023]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH -next v2 0/6] landlock: add chmod and chown support
From


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(-)
>>>>

\
 
 \ /
  Last update: 2023-04-26 15:59    [W:0.083 / U:0.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site