On 29/08/2022 03:17, xiujianfeng wrote:

Hi,

在 2022/8/28 3:30, Günther Noack 写道:

...

Hello!

the mapping between Landlock rights to LSM hooks is now as follows in your patch set:

• LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
• LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown (this hook can restrict both the chown(2) and chgrp(2) syscalls)

Is this the desired mapping?

The previous discussion I found on the topic was in

[1] https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net... [2] https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-c... [3] https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net...

In my understanding the main arguments were the ones in [2] and [3].

There were no further responses to [3], so I was under the impression that we were gravitating towards an approach where the file-metadata-modification operations were grouped more coarsely?

For example with the approach suggested in [3], which would be to group the operations coarsely into (a) one Landlock right for modifying file metadata that is used in security contexts, and (b) one Landlock right for modifying metadata that was used in non-security contexts. That would mean that there would be:

(a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the following operations: * chmod(2)-variants through hook_path_chmod, * chown(2)-variants and chgrp(2)-variants through hook_path_chown, * setxattr(2)-variants and removexattr(2)-variants for extended attributes that are not "user extended attributes" as described in xattr(7) through hook_inode_setxattr and hook_inode_removexattr

(b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the following operations: * utimes(2) and other operations for setting other non-security sensitive attributes, probably through hook_inode_setattr(?) * xattr modifications like above, but for the "user extended attributes", though hook_inode_setxattr and hook_inode_removexattr

In my mind, this would be a sensible grouping, and it would also help to decouple the userspace-exposed API from the underlying implementation, as Casey suggested to do in [2].

Specifically for this patch set, if you want to use this grouping, you would only need to add one new Landlock right (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above under (a) (and maybe we can find a shorter name for it... :))?

Did I miss any operations here that would be necessary to restrict?

Would that make sense to you? Xiu, what is your opinion on how this should be grouped? Do you have use cases in mind where a more fine-grained grouping would be required?

I apologize I may missed that discussion when I prepared v2:(

Yes, agreed, this grouping is more sensible and resonnable. so in this patchset only one right will be added, and I suppose the first commit which expand access_mask_t to u32 can be droped.

...

—Günther

P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called on a variety on attribute changes including file ownership, file size and file mode, so it might potentially interact with a bunch of other existing Landlock rights. Maybe that is not the right approach. In any case, it seems like it might require more thinking and it might be sensible to do that in a separate patch set IMHO.

Thanks for you reminder, that seems it's more complicated to support utimes, so I think we'd better not support it in this patchset.

The issue with this approach is that it makes it impossible to properly group such access rights. Indeed, to avoid inconsistencies and much more complexity, we cannot extend a Landlock access right once it is defined.

We also need to consider that file ownership and permissions have a default (e.g. umask), which is also a way to set them. How to consistently manage that? What if the application wants to protect its files with chmod 0400?

About the naming, I think we can start with: - LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata); - LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr; - LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel (could change non-Landlock DAC or MAC, which could be considered as a policy bypass; or other various xattr that might be interpreted by filesystems), this should be denied most of the time.

CCing linux-fsdevel@vger.kernel.org

On 01/09/2022 15:06, xiujianfeng wrote:

Hi,

在 2022/8/30 0:01, Mickaël Salaün 写道:

...

On 29/08/2022 03:17, xiujianfeng wrote:

...

Hi,

在 2022/8/28 3:30, Günther Noack 写道:

...

Hello!

the mapping between Landlock rights to LSM hooks is now as follows in your patch set:

• LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
• LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown

(this hook can restrict both the chown(2) and chgrp(2) syscalls)

Is this the desired mapping?

The previous discussion I found on the topic was in

[1] https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net...

[2] https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-c...

[3] https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net...

In my understanding the main arguments were the ones in [2] and [3].

There were no further responses to [3], so I was under the impression that we were gravitating towards an approach where the file-metadata-modification operations were grouped more coarsely?

For example with the approach suggested in [3], which would be to group the operations coarsely into (a) one Landlock right for modifying file metadata that is used in security contexts, and (b) one Landlock right for modifying metadata that was used in non-security contexts. That would mean that there would be:

(a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the following operations:     * chmod(2)-variants through hook_path_chmod,     * chown(2)-variants and chgrp(2)-variants through hook_path_chown,     * setxattr(2)-variants and removexattr(2)-variants for extended       attributes that are not "user extended attributes" as described in       xattr(7) through hook_inode_setxattr and hook_inode_removexattr

(b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the following operations:     * utimes(2) and other operations for setting other non-security       sensitive attributes, probably through hook_inode_setattr(?)     * xattr modifications like above, but for the "user extended       attributes", though hook_inode_setxattr and hook_inode_removexattr

In my mind, this would be a sensible grouping, and it would also help to decouple the userspace-exposed API from the underlying implementation, as Casey suggested to do in [2].

Specifically for this patch set, if you want to use this grouping, you would only need to add one new Landlock right (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above under (a) (and maybe we can find a shorter name for it... :))?

Did I miss any operations here that would be necessary to restrict?

Would that make sense to you? Xiu, what is your opinion on how this should be grouped? Do you have use cases in mind where a more fine-grained grouping would be required?

I apologize I may missed that discussion when I prepared v2:(

Yes, agreed, this grouping is more sensible and resonnable. so in this patchset only one right will be added, and I suppose the first commit which expand access_mask_t to u32 can be droped.

...

—Günther

P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called on a variety on attribute changes including file ownership, file size and file mode, so it might potentially interact with a bunch of other existing Landlock rights. Maybe that is not the right approach. In any case, it seems like it might require more thinking and it might be sensible to do that in a separate patch set IMHO.

Thanks for you reminder, that seems it's more complicated to support utimes, so I think we'd better not support it in this patchset.

The issue with this approach is that it makes it impossible to properly group such access rights. Indeed, to avoid inconsistencies and much more complexity, we cannot extend a Landlock access right once it is defined.

We also need to consider that file ownership and permissions have a default (e.g. umask), which is also a way to set them. How to consistently manage that? What if the application wants to protect its files with chmod 0400?

what do you mean by this? do you mean that we should have a set of default permissions for files created by applications within the sandbox, so that it can update metadata of its own file.

I mean that we need a consistent access control system, and for this we need to consider all the ways an extended attribute can be set.

We can either extend the meaning of current access rights (controlled with a ruleset flag for compatibility reasons), or create new access rights. I think it would be better to add new dedicated rights to make it more explicit and flexible.

I'm not sure about the right approach to properly control file permission. We need to think about it. Do you have some ideas?

BTW, utimes can be controlled with the inode_setattr() LSM hook. Being able to control arbitrary file time modification could be part of the FS_WRITE_SAFE_METADATA, but modification and access time should always be updated according to the file operation.

...

About the naming, I think we can start with:

• LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
• LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr;

do you mean we should have permission controls on metadata level or operation level? e.g. should we allow update on user xattr but deny update on security xattr? or should we disallow update on any xattr?

...

• LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel

(could change non-Landlock DAC or MAC, which could be considered as a policy bypass; or other various xattr that might be interpreted by filesystems), this should be denied most of the time.

do you mean FS_WRITE_UNSAFE_METADATA is security-related? and FS_WRITE_SAFE_METADATA is non-security-related?

Yes, FS_WRITE_UNSAFE_METADATA would be for security related xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr. Both are mutually exclusive. This would involve the inode_setattr and inode_setxattr LSM hooks. Looking at the calling sites, it seems possible to replace all inode arguments with paths.

On 29/10/2022 10:33, xiujianfeng wrote:

Hi,

在 2022/9/2 1:34, Mickaël Salaün 写道:

...

CCing linux-fsdevel@vger.kernel.org

On 01/09/2022 15:06, xiujianfeng wrote:

...

Hi,

在 2022/8/30 0:01, Mickaël Salaün 写道:

...

On 29/08/2022 03:17, xiujianfeng wrote:

...

Hi,

在 2022/8/28 3:30, Günther Noack 写道:

...

Hello!

the mapping between Landlock rights to LSM hooks is now as follows in your patch set:

• LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
• LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown

(this hook can restrict both the chown(2) and chgrp(2) syscalls)

Is this the desired mapping?

The previous discussion I found on the topic was in

[1] https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net...

[2] https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-c...

[3] https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net...

In my understanding the main arguments were the ones in [2] and [3].

There were no further responses to [3], so I was under the impression that we were gravitating towards an approach where the file-metadata-modification operations were grouped more coarsely?

For example with the approach suggested in [3], which would be to group the operations coarsely into (a) one Landlock right for modifying file metadata that is used in security contexts, and (b) one Landlock right for modifying metadata that was used in non-security contexts. That would mean that there would be:

(a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the following operations:      * chmod(2)-variants through hook_path_chmod,      * chown(2)-variants and chgrp(2)-variants through hook_path_chown,      * setxattr(2)-variants and removexattr(2)-variants for extended        attributes that are not "user extended attributes" as described in        xattr(7) through hook_inode_setxattr and hook_inode_removexattr

(b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the following operations:      * utimes(2) and other operations for setting other non-security        sensitive attributes, probably through hook_inode_setattr(?)      * xattr modifications like above, but for the "user extended        attributes", though hook_inode_setxattr and hook_inode_removexattr

In my mind, this would be a sensible grouping, and it would also help to decouple the userspace-exposed API from the underlying implementation, as Casey suggested to do in [2].

Specifically for this patch set, if you want to use this grouping, you would only need to add one new Landlock right (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above under (a) (and maybe we can find a shorter name for it... :))?

Did I miss any operations here that would be necessary to restrict?

Would that make sense to you? Xiu, what is your opinion on how this should be grouped? Do you have use cases in mind where a more fine-grained grouping would be required?

I apologize I may missed that discussion when I prepared v2:(

Yes, agreed, this grouping is more sensible and resonnable. so in this patchset only one right will be added, and I suppose the first commit which expand access_mask_t to u32 can be droped.

...

—Günther

P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called on a variety on attribute changes including file ownership, file size and file mode, so it might potentially interact with a bunch of other existing Landlock rights. Maybe that is not the right approach. In any case, it seems like it might require more thinking and it might be sensible to do that in a separate patch set IMHO.

Thanks for you reminder, that seems it's more complicated to support utimes, so I think we'd better not support it in this patchset.

The issue with this approach is that it makes it impossible to properly group such access rights. Indeed, to avoid inconsistencies and much more complexity, we cannot extend a Landlock access right once it is defined.

We also need to consider that file ownership and permissions have a default (e.g. umask), which is also a way to set them. How to consistently manage that? What if the application wants to protect its files with chmod 0400?

what do you mean by this? do you mean that we should have a set of default permissions for files created by applications within the sandbox, so that it can update metadata of its own file.

I mean that we need a consistent access control system, and for this we need to consider all the ways an extended attribute can be set.

We can either extend the meaning of current access rights (controlled with a ruleset flag for compatibility reasons), or create new access rights. I think it would be better to add new dedicated rights to make it more explicit and flexible.

I'm not sure about the right approach to properly control file permission. We need to think about it. Do you have some ideas?

BTW, utimes can be controlled with the inode_setattr() LSM hook. Being able to control arbitrary file time modification could be part of the FS_WRITE_SAFE_METADATA, but modification and access time should always be updated according to the file operation.

...

...

About the naming, I think we can start with:

• LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
• LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr;

do you mean we should have permission controls on metadata level or operation level? e.g. should we allow update on user xattr but deny update on security xattr? or should we disallow update on any xattr?

...

• LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel

(could change non-Landlock DAC or MAC, which could be considered as a policy bypass; or other various xattr that might be interpreted by filesystems), this should be denied most of the time.

do you mean FS_WRITE_UNSAFE_METADATA is security-related? and FS_WRITE_SAFE_METADATA is non-security-related?

Yes, FS_WRITE_UNSAFE_METADATA would be for security related xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr. Both are mutually exclusive. This would involve the inode_setattr and inode_setxattr LSM hooks. Looking at the calling sites, it seems possible to replace all inode arguments with paths.

I though about differentiating user xattr, atime/mtime, DAC (chown/chmod, posix ACLs), and other xattr, but it would be too complex to get a consistent approach because of indirect consequences (e.g. controlling umask, setegid, settimeofday…). Let's make it simple for now.

Here is an update on my previous proposal:

LANDLOCK_ACCESS_FS_READ_METADATA to read any file/dir metadata (i.e. inode attr and xattr). In practice, for most use cases, this access right should be granted whenever LANDLOCK_ACCESS_READ_DIR is allowed.

LANDLOCK_ACCESS_FS_WRITE_METADATA to *explicitly* write any inode attr or xattr (i.e. chmod, chown, utime, and all xattr). It should be noted that file modification time and access time should always be updated according to the file operation (e.g. write, truncate) even when this access is not explicitly allowed (according to vfs_utimes(), ATTR_TIMES_SET and ATTR_TOUCH should enable to differentiate from implicit time changes).

Sorry for the late reply, I have problems with this work, for example, before: security_inode_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, struct iattr *attr) after: security_inode_setattr(struct user_namespace *mnt_userns, struct path *path, struct iattr *attr) then I change the second argument in notify_change() from struct *dentry to struct path *, that makes this kind of changes in fs/overlayfs/ spread to lots of places because overlayfs basicly uses dentry instead of path, the worst case may be here:

ovl_special_inode_operations.set_acl hook calls: --> ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry, struct posix_acl *acl, int type) --> ovl_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,struct iattr *attr) --> ovl_do_notify_change(struct ovl_fs *ofs, struct dentry *upperdentry, struct iattr *attr)

from the top of this callchain, I can not find a path to replace dentry, did I miss something? or do you have better idea?

I think this can be solved thanks to the ovl_path_real() helper.

On 18/11/2022 10:03, xiujianfeng wrote:

在 2022/11/14 22:12, Mickaël Salaün 写道:

...

On 29/10/2022 10:33, xiujianfeng wrote:

...

Hi,

在 2022/9/2 1:34, Mickaël Salaün 写道:

...

CCing linux-fsdevel@vger.kernel.org

On 01/09/2022 15:06, xiujianfeng wrote:

...

Hi,

在 2022/8/30 0:01, Mickaël Salaün 写道:

...

On 29/08/2022 03:17, xiujianfeng wrote: > > Hi, > > 在 2022/8/28 3:30, Günther Noack 写道: >> Hello! >> >> the mapping between Landlock rights to LSM hooks is now as >> follows in >> your patch set: >> >> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod >> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown >>       (this hook can restrict both the chown(2) and chgrp(2) >> syscalls) >> >> Is this the desired mapping? >> >> The previous discussion I found on the topic was in >> >> [1] >> https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net... >> >> >> >> [2] >> https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-c... >> >> >> >> [3] >> https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net... >> >> >> >> >> In my understanding the main arguments were the ones in [2] and [3]. >> >> There were no further responses to [3], so I was under the >> impression >> that we were gravitating towards an approach where the >> file-metadata-modification operations were grouped more coarsely? >> >> For example with the approach suggested in [3], which would be to >> group the operations coarsely into (a) one Landlock right for >> modifying file metadata that is used in security contexts, and >> (b) one >> Landlock right for modifying metadata that was used in non-security >> contexts. That would mean that there would be: >> >> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the >> following operations: >>       * chmod(2)-variants through hook_path_chmod, >>       * chown(2)-variants and chgrp(2)-variants through >> hook_path_chown, >>       * setxattr(2)-variants and removexattr(2)-variants for >> extended >>         attributes that are not "user extended attributes" as >> described in >>         xattr(7) through hook_inode_setxattr and >> hook_inode_removexattr >> >> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the >> following operations: >>       * utimes(2) and other operations for setting other >> non-security >>         sensitive attributes, probably through hook_inode_setattr(?) >>       * xattr modifications like above, but for the "user extended >>         attributes", though hook_inode_setxattr and >> hook_inode_removexattr >> >> In my mind, this would be a sensible grouping, and it would also >> help >> to decouple the userspace-exposed API from the underlying >> implementation, as Casey suggested to do in [2]. >> >> Specifically for this patch set, if you want to use this >> grouping, you >> would only need to add one new Landlock right >> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above >> under (a) (and maybe we can find a shorter name for it... :))? >> >> Did I miss any operations here that would be necessary to restrict? >> >> Would that make sense to you? Xiu, what is your opinion on how this >> should be grouped? Do you have use cases in mind where a more >> fine-grained grouping would be required? > > I apologize I may missed that discussion when I prepared v2:( > > Yes, agreed, this grouping is more sensible and resonnable. so in > this > patchset only one right will be added, and I suppose the first commit > which expand access_mask_t to u32 can be droped. > >> >> —Günther >> >> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets >> called >> on a variety on attribute changes including file ownership, file >> size >> and file mode, so it might potentially interact with a bunch of >> other >> existing Landlock rights. Maybe that is not the right approach. >> In any >> case, it seems like it might require more thinking and it might be >> sensible to do that in a separate patch set IMHO. > > Thanks for you reminder, that seems it's more complicated to support > utimes, so I think we'd better not support it in this patchset.

The issue with this approach is that it makes it impossible to properly group such access rights. Indeed, to avoid inconsistencies and much more complexity, we cannot extend a Landlock access right once it is defined.

We also need to consider that file ownership and permissions have a default (e.g. umask), which is also a way to set them. How to consistently manage that? What if the application wants to protect its files with chmod 0400?

what do you mean by this? do you mean that we should have a set of default permissions for files created by applications within the sandbox, so that it can update metadata of its own file.

I mean that we need a consistent access control system, and for this we need to consider all the ways an extended attribute can be set.

We can either extend the meaning of current access rights (controlled with a ruleset flag for compatibility reasons), or create new access rights. I think it would be better to add new dedicated rights to make it more explicit and flexible.

I'm not sure about the right approach to properly control file permission. We need to think about it. Do you have some ideas?

BTW, utimes can be controlled with the inode_setattr() LSM hook. Being able to control arbitrary file time modification could be part of the FS_WRITE_SAFE_METADATA, but modification and access time should always be updated according to the file operation.

...

...

About the naming, I think we can start with:

• LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
• LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user

xattr;

do you mean we should have permission controls on metadata level or operation level? e.g. should we allow update on user xattr but deny update on security xattr? or should we disallow update on any xattr?

...

• LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel

(could change non-Landlock DAC or MAC, which could be considered as a policy bypass; or other various xattr that might be interpreted by filesystems), this should be denied most of the time.

do you mean FS_WRITE_UNSAFE_METADATA is security-related? and FS_WRITE_SAFE_METADATA is non-security-related?

Yes, FS_WRITE_UNSAFE_METADATA would be for security related xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr. Both are mutually exclusive. This would involve the inode_setattr and inode_setxattr LSM hooks. Looking at the calling sites, it seems possible to replace all inode arguments with paths.

I though about differentiating user xattr, atime/mtime, DAC (chown/chmod, posix ACLs), and other xattr, but it would be too complex to get a consistent approach because of indirect consequences (e.g. controlling umask, setegid, settimeofday…). Let's make it simple for now.

Here is an update on my previous proposal:

LANDLOCK_ACCESS_FS_READ_METADATA to read any file/dir metadata (i.e. inode attr and xattr). In practice, for most use cases, this access right should be granted whenever LANDLOCK_ACCESS_READ_DIR is allowed.

LANDLOCK_ACCESS_FS_WRITE_METADATA to *explicitly* write any inode attr or xattr (i.e. chmod, chown, utime, and all xattr). It should be noted that file modification time and access time should always be updated according to the file operation (e.g. write, truncate) even when this access is not explicitly allowed (according to vfs_utimes(), ATTR_TIMES_SET and ATTR_TOUCH should enable to differentiate from implicit time changes).

Thanks, I analyzed the relevant functions and the use of lsm hooks. so I think what to do will be as follows:

LANDLOCK_ACCESS_FS_WRITE_METADATA controls the following hooks: 1.security_path_chmod 2.security_path_chown

These two chmod/chown hooks would be redundant with security_inode_setattr(). We then don't need to implement them.

3.security_inode_setattr 4.security_inode_setxattr 5.security_inode_removexattr > 6.security_inode_set_acl

Good catch. This new security_inode_set_acl hook is a good example of API refactoring. BTW, the related Cc list should be included in your next patch series.

LANDLOCK_ACCESS_FS_READ_METADATA controls the following hooks: 1.security_inode_getattr 2.security_inode_get_acl 3.security_inode_getxattr

Correct

and the following 7 hooks are using struct dentry * as parameter, should be changed to struct path *, and also their callers.

security_inode_setattr security_inode_setxattr security_inode_removexattr security_inode_set_acl security_inode_getattr security_inode_get_acl security_inode_getxattr

Looks like it's a big change.

Your proposed approach looks good, and this will indeed touch a lot of files.

Because it interacts a lot with the filesystem subsystem, I propose to first write a set of patches that refactor the security_inode_*attr and security_inode_*_acl hooks to use struct file (or struct path when it makes sense) instead of struct dentry/inode (and to remove struct user_namespace as argument because it can be inferred thanks to file_mnt_user_ns). As for [1], using struct file only makes sense for a specific set of calls, and struct path should be used otherwise (e.g. syscalls dealing with file descriptors vs. with file paths).

You need to base this work on Christian's branch to be up-to-date with ongoing FS changes. I suggest to create one patch per function API change e.g., notify_change (merge the mnt_userns and dentry in a file argument), struct inode_operations.setattr (use a file argument instead of dentry)…

Once this refactoring will be in -next, the landlock_file_security changes [1] will already be merged in master, and you will then be able to work on the Landlock specific parts with the new hooks.

[1] https://git.kernel.org/mic/c/b9f5ce27c8f8

...

...

Sorry for the late reply, I have problems with this work, for example, before: security_inode_setattr(struct user_namespace *mnt_userns,                                            struct dentry *dentry,                                            struct iattr *attr) after: security_inode_setattr(struct user_namespace *mnt_userns,                                            struct path *path,                                            struct iattr *attr) then I change the second argument in notify_change() from struct *dentry to struct path *, that makes this kind of changes in fs/overlayfs/ spread to lots of places because overlayfs basicly uses dentry instead of path, the worst case may be here:

ovl_special_inode_operations.set_acl hook calls: --> ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry, struct posix_acl *acl, int type) --> ovl_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,struct iattr *attr) --> ovl_do_notify_change(struct ovl_fs *ofs, struct dentry *upperdentry, struct iattr *attr)

from the top of this callchain, I can not find a path to replace dentry, did I miss something? or do you have better idea?

I think this can be solved thanks to the ovl_path_real() helper. .

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

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?

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.

...

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

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

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