Date:   Wed, 26 Apr 2023 15:58:31 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     xiujianfeng <xiujianfeng@...wei.com>, paul@...l-moore.com,
        jmorris@...ei.org, serge@...lyn.com, shuah@...nel.org,
        corbet@....net
Cc:     linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-doc@...r.kernel.org, roberto.sassu@...wei.com,
        Konstantin Meskhidze <konstantin.meskhidze@...wei.com>,
        Linux-Fsdevel <linux-fsdevel@...r.kernel.org>,
        Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH -next v2 0/6] landlock: add chmod and chown support



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

Powered by blists - more mailing lists