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

[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@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.


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