Re: [PATCH -next v2 0/6] landlock: add chmod and chown support

[xiujianfeng]

On 2023/4/26 21:58, Mickaël Salaün wrote:
> 
> 
> 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@xxxxxxxxxxxxxxx/
>>>>
>>>> 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.

I was thinking to get everything done at once, but it looks like we are
having some troubles, so it's good advice to separate it into small
works and upstream them on by one, Thanks.

Currently inode_getattr already takes a struct path argument in the
mainline, I would send the patches about inode_setattr refactor and
discuss the problem of file_dentry() on that thread.

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