[PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Xiu Jianfeng]

Hi,

I am working on adding xattr/attr support for landlock [1], so we can
control fs accesses such as chmod, chown, uptimes, setxattr, etc.. inside
landlock sandbox. the LSM hooks as following are invoved:
1.inode_setattr
2.inode_setxattr
3.inode_removexattr
4.inode_set_acl
5.inode_remove_acl
which are controlled by LANDLOCK_ACCESS_FS_WRITE_METADATA.

and
1.inode_getattr
2.inode_get_acl
3.inode_getxattr
4.inode_listxattr
which are controlled by LANDLOCK_ACCESS_FS_READ_METADATA

Some of these hooks only take struct dentry as a argument, However, for
path-based LSMs such Landlock, Apparmor and Tomoyo, struct path instead
of struct dentry required to make sense of attr/xattr accesses. So we
need to refactor these hooks to take a struct path argument.

This patchset only refators inode_setattr hook as part of whole work.

Also, I have a problem about file_dentry() in __file_remove_privs() of the
first patch, before changes in commit c1892c37769cf ("vfs: fix deadlock in
file_remove_privs() on overlayfs"), it gets dentry and inode as belows:

struct dentry *dentry = file->f_path.dentry;
struct inode *inode = d_inode(dentry);

That would be clear to change it to pass &file->f_path to
__remove_privs()->notify_change()->inode_setattr().
After that commit, it has been changed to:

struct dentry *dentry = file_dentry(file);
struct inode *inode = file_inode(file);

If I understand correctly, the dentry from file_dentry() maybe the upper
or the lower, it can be different from file->f_path.dentry. It can't just
go back to use &file->f_path otherwise the bug will come back for
overlayfs. So for such scenario, how to get a path from file if the file
maybe or not from overlayfs, and which kind of overlayfs path is ok for
Landlock?

Xiu Jianfeng (2):
  fs: Change notify_change() to take struct path argument
  lsm: Change inode_setattr hook to take struct path argument

 drivers/base/devtmpfs.c       |  5 +++--
 fs/attr.c                     |  7 ++++---
 fs/cachefiles/interface.c     |  4 ++--
 fs/coredump.c                 |  2 +-
 fs/ecryptfs/inode.c           | 18 +++++++++---------
 fs/fat/file.c                 |  2 +-
 fs/inode.c                    |  8 +++++---
 fs/ksmbd/smb2pdu.c            |  6 +++---
 fs/ksmbd/smbacl.c             |  2 +-
 fs/namei.c                    |  2 +-
 fs/nfsd/vfs.c                 | 12 ++++++++----
 fs/open.c                     | 19 ++++++++++---------
 fs/overlayfs/overlayfs.h      |  4 +++-
 fs/utimes.c                   |  2 +-
 include/linux/fs.h            |  4 ++--
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      |  4 ++--
 security/security.c           | 10 +++++-----
 security/selinux/hooks.c      |  3 ++-
 security/smack/smack_lsm.c    |  5 +++--
 20 files changed, 67 insertions(+), 54 deletions(-)

-- 
2.17.1

[PATCH -next 1/2] fs: Change notify_change() to take struct path argument

[Xiu Jianfeng]

For path-based LSMs such as Landlock, struct path instead of struct
dentry is required to make sense of attr/xattr accesses. notify_change()
is the main caller of security_inode_setattr(), so refactor it first
before lsm hook inode_setattr().

This patch also touches do_truncate() and other related callers.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 drivers/base/devtmpfs.c   |  5 +++--
 fs/attr.c                 |  5 +++--
 fs/cachefiles/interface.c |  4 ++--
 fs/coredump.c             |  2 +-
 fs/ecryptfs/inode.c       | 18 +++++++++---------
 fs/inode.c                |  8 +++++---
 fs/ksmbd/smb2pdu.c        |  6 +++---
 fs/ksmbd/smbacl.c         |  2 +-
 fs/namei.c                |  2 +-
 fs/nfsd/vfs.c             | 12 ++++++++----
 fs/open.c                 | 19 ++++++++++---------
 fs/overlayfs/overlayfs.h  |  4 +++-
 fs/utimes.c               |  2 +-
 include/linux/fs.h        |  4 ++--
 14 files changed, 52 insertions(+), 41 deletions(-)

diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index b848764ef018..e67dd258984c 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -220,13 +220,14 @@ static int handle_create(const char *nodename, umode_t mode, kuid_t uid,
 			dev->devt);
 	if (!err) {
 		struct iattr newattrs;
+		struct path new = { .mnt = path.mnt, .dentry = dentry };
 
 		newattrs.ia_mode = mode;
 		newattrs.ia_uid = uid;
 		newattrs.ia_gid = gid;
 		newattrs.ia_valid = ATTR_MODE|ATTR_UID|ATTR_GID;
 		inode_lock(d_inode(dentry));
-		notify_change(&nop_mnt_idmap, dentry, &newattrs, NULL);
+		notify_change(&nop_mnt_idmap, &new, &newattrs, NULL);
 		inode_unlock(d_inode(dentry));
 
 		/* mark as kernel-created inode */
@@ -334,7 +335,7 @@ static int handle_remove(const char *nodename, struct device *dev)
 			newattrs.ia_valid =
 				ATTR_UID|ATTR_GID|ATTR_MODE;
 			inode_lock(d_inode(dentry));
-			notify_change(&nop_mnt_idmap, dentry, &newattrs, NULL);
+			notify_change(&nop_mnt_idmap, &p, &newattrs, NULL);
 			inode_unlock(d_inode(dentry));
 			err = vfs_unlink(&nop_mnt_idmap, d_inode(parent.dentry),
 					 dentry, NULL);
diff --git a/fs/attr.c b/fs/attr.c
index d60dc1edb526..eecd78944b83 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -354,7 +354,7 @@ EXPORT_SYMBOL(may_setattr);
 /**
  * notify_change - modify attributes of a filesytem object
  * @idmap:	idmap of the mount the inode was found from
- * @dentry:	object affected
+ * @path:	path of object affected
  * @attr:	new attributes
  * @delegated_inode: returns inode, if the inode is delegated
  *
@@ -378,9 +378,10 @@ EXPORT_SYMBOL(may_setattr);
  * permissions. On non-idmapped mounts or if permission checking is to be
  * performed on the raw inode simply pass @nop_mnt_idmap.
  */
-int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
+int notify_change(struct mnt_idmap *idmap, const struct path *path,
 		  struct iattr *attr, struct inode **delegated_inode)
 {
+	struct dentry *dentry = path->dentry;
 	struct inode *inode = dentry->d_inode;
 	umode_t mode = inode->i_mode;
 	int error;
diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
index 40052bdb3365..4700285a76f0 100644
--- a/fs/cachefiles/interface.c
+++ b/fs/cachefiles/interface.c
@@ -138,7 +138,7 @@ static int cachefiles_adjust_size(struct cachefiles_object *object)
 		newattrs.ia_size = oi_size & PAGE_MASK;
 		ret = cachefiles_inject_remove_error();
 		if (ret == 0)
-			ret = notify_change(&nop_mnt_idmap, file->f_path.dentry,
+			ret = notify_change(&nop_mnt_idmap, &file->f_path,
 					    &newattrs, NULL);
 		if (ret < 0)
 			goto truncate_failed;
@@ -148,7 +148,7 @@ static int cachefiles_adjust_size(struct cachefiles_object *object)
 	newattrs.ia_size = ni_size;
 	ret = cachefiles_inject_write_error();
 	if (ret == 0)
-		ret = notify_change(&nop_mnt_idmap, file->f_path.dentry,
+		ret = notify_change(&nop_mnt_idmap, &file->f_path,
 				    &newattrs, NULL);
 
 truncate_failed:
diff --git a/fs/coredump.c b/fs/coredump.c
index ece7badf701b..01bef4830bfa 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -736,7 +736,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		}
 		if (!(cprm.file->f_mode & FMODE_CAN_WRITE))
 			goto close_fail;
-		if (do_truncate(idmap, cprm.file->f_path.dentry,
+		if (do_truncate(idmap, &cprm.file->f_path,
 				0, 0, cprm.file))
 			goto close_fail;
 	}
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 83274915ba6d..423bd457623e 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -853,12 +853,12 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length)
 
 	rc = truncate_upper(dentry, &ia, &lower_ia);
 	if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
-		struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
+		const struct path *lower_path = ecryptfs_dentry_to_lower_path(dentry);
 
-		inode_lock(d_inode(lower_dentry));
-		rc = notify_change(&nop_mnt_idmap, lower_dentry,
+		inode_lock(d_inode(lower_path->dentry));
+		rc = notify_change(&nop_mnt_idmap, lower_path,
 				   &lower_ia, NULL);
-		inode_unlock(d_inode(lower_dentry));
+		inode_unlock(d_inode(lower_path->dentry));
 	}
 	return rc;
 }
@@ -888,7 +888,7 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, struct iattr *ia)
 {
 	int rc = 0;
-	struct dentry *lower_dentry;
+	const struct path *lower_path;
 	struct iattr lower_ia;
 	struct inode *inode;
 	struct inode *lower_inode;
@@ -902,7 +902,7 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 	}
 	inode = d_inode(dentry);
 	lower_inode = ecryptfs_inode_to_lower(inode);
-	lower_dentry = ecryptfs_dentry_to_lower(dentry);
+	lower_path = ecryptfs_dentry_to_lower_path(dentry);
 	mutex_lock(&crypt_stat->cs_mutex);
 	if (d_is_dir(dentry))
 		crypt_stat->flags &= ~(ECRYPTFS_ENCRYPTED);
@@ -964,9 +964,9 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
 	if (lower_ia.ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID))
 		lower_ia.ia_valid &= ~ATTR_MODE;
 
-	inode_lock(d_inode(lower_dentry));
-	rc = notify_change(&nop_mnt_idmap, lower_dentry, &lower_ia, NULL);
-	inode_unlock(d_inode(lower_dentry));
+	inode_lock(d_inode(lower_path->dentry));
+	rc = notify_change(&nop_mnt_idmap, lower_path, &lower_ia, NULL);
+	inode_unlock(d_inode(lower_path->dentry));
 out:
 	fsstack_copy_attr_all(inode, lower_inode);
 	return rc;
diff --git a/fs/inode.c b/fs/inode.c
index 577799b7855f..4fa51d46f655 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1973,7 +1973,7 @@ int dentry_needs_remove_privs(struct mnt_idmap *idmap,
 }
 
 static int __remove_privs(struct mnt_idmap *idmap,
-			  struct dentry *dentry, int kill)
+			  struct path *path, int kill)
 {
 	struct iattr newattrs;
 
@@ -1982,13 +1982,15 @@ static int __remove_privs(struct mnt_idmap *idmap,
 	 * Note we call this on write, so notify_change will not
 	 * encounter any conflicting delegations:
 	 */
-	return notify_change(idmap, dentry, &newattrs, NULL);
+	return notify_change(idmap, path, &newattrs, NULL);
 }
 
 static int __file_remove_privs(struct file *file, unsigned int flags)
 {
 	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = file_inode(file);
+	/* this path maybe incorrect */
+	struct path path = {.mnt = file->f_path.mnt, .dentry = dentry};
 	int error = 0;
 	int kill;
 
@@ -2003,7 +2005,7 @@ static int __file_remove_privs(struct file *file, unsigned int flags)
 		if (flags & IOCB_NOWAIT)
 			return -EAGAIN;
 
-		error = __remove_privs(file_mnt_idmap(file), dentry, kill);
+		error = __remove_privs(file_mnt_idmap(file), &path, kill);
 	}
 
 	if (!error)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index cb93fd231f4e..2b7e5c446397 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -5644,8 +5644,8 @@ static int set_file_basic_info(struct ksmbd_file *fp,
 	}
 
 	if (attrs.ia_valid) {
-		struct dentry *dentry = filp->f_path.dentry;
-		struct inode *inode = d_inode(dentry);
+		struct path *path = &filp->f_path;
+		struct inode *inode = d_inode(path->dentry);
 
 		if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 			return -EACCES;
@@ -5653,7 +5653,7 @@ static int set_file_basic_info(struct ksmbd_file *fp,
 		inode_lock(inode);
 		inode->i_ctime = attrs.ia_ctime;
 		attrs.ia_valid &= ~ATTR_CTIME;
-		rc = notify_change(idmap, dentry, &attrs, NULL);
+		rc = notify_change(idmap, path, &attrs, NULL);
 		inode_unlock(inode);
 	}
 	return rc;
diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
index 6d6cfb6957a9..39d8aff3ae1b 100644
--- a/fs/ksmbd/smbacl.c
+++ b/fs/ksmbd/smbacl.c
@@ -1403,7 +1403,7 @@ int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
 	}
 
 	inode_lock(inode);
-	rc = notify_change(idmap, path->dentry, &newattrs, NULL);
+	rc = notify_change(idmap, path, &newattrs, NULL);
 	inode_unlock(inode);
 	if (rc)
 		goto out;
diff --git a/fs/namei.c b/fs/namei.c
index e4fe0879ae55..ec7075a8505d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3292,7 +3292,7 @@ static int handle_truncate(struct mnt_idmap *idmap, struct file *filp)
 
 	error = security_file_truncate(filp);
 	if (!error) {
-		error = do_truncate(idmap, path->dentry, 0,
+		error = do_truncate(idmap, path, 0,
 				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
 				    filp);
 	}
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index bb9d47172162..4b51fd2f05e3 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -410,7 +410,7 @@ nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	return nfserrno(get_write_access(inode));
 }
 
-static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
+static int __nfsd_setattr(struct path *path, struct iattr *iap)
 {
 	int host_err;
 
@@ -430,7 +430,7 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
 		if (iap->ia_size < 0)
 			return -EFBIG;
 
-		host_err = notify_change(&nop_mnt_idmap, dentry, &size_attr, NULL);
+		host_err = notify_change(&nop_mnt_idmap, path, &size_attr, NULL);
 		if (host_err)
 			return host_err;
 		iap->ia_valid &= ~ATTR_SIZE;
@@ -448,7 +448,7 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
 		return 0;
 
 	iap->ia_valid |= ATTR_CTIME;
-	return notify_change(&nop_mnt_idmap, dentry, iap, NULL);
+	return notify_change(&nop_mnt_idmap, path, iap, NULL);
 }
 
 /**
@@ -471,6 +471,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	     struct nfsd_attrs *attr,
 	     int check_guard, time64_t guardtime)
 {
+	struct path path;
 	struct dentry	*dentry;
 	struct inode	*inode;
 	struct iattr	*iap = attr->na_iattr;
@@ -534,9 +535,12 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			return err;
 	}
 
+	path.mnt = fhp->fh_export->ex_path.mnt;
+	path.dentry = fhp->fh_dentry;
+
 	inode_lock(inode);
 	for (retries = 1;;) {
-		host_err = __nfsd_setattr(dentry, iap);
+		host_err = __nfsd_setattr(&path, iap);
 		if (host_err != -EAGAIN || !retries--)
 			break;
 		if (!nfsd_wait_for_delegreturn(rqstp, inode))
diff --git a/fs/open.c b/fs/open.c
index 4478adcc4f3a..7a7841606285 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -37,11 +37,12 @@
 
 #include "internal.h"
 
-int do_truncate(struct mnt_idmap *idmap, struct dentry *dentry,
+int do_truncate(struct mnt_idmap *idmap, const struct path *path,
 		loff_t length, unsigned int time_attrs, struct file *filp)
 {
 	int ret;
 	struct iattr newattrs;
+	struct dentry *dentry = path->dentry;
 
 	/* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
 	if (length < 0)
@@ -63,7 +64,7 @@ int do_truncate(struct mnt_idmap *idmap, struct dentry *dentry,
 
 	inode_lock(dentry->d_inode);
 	/* Note any delegations or leases have already been broken: */
-	ret = notify_change(idmap, dentry, &newattrs, NULL);
+	ret = notify_change(idmap, path, &newattrs, NULL);
 	inode_unlock(dentry->d_inode);
 	return ret;
 }
@@ -109,7 +110,7 @@ long vfs_truncate(const struct path *path, loff_t length)
 
 	error = security_path_truncate(path);
 	if (!error)
-		error = do_truncate(idmap, path->dentry, length, 0, NULL);
+		error = do_truncate(idmap, path, length, 0, NULL);
 
 put_write_and_out:
 	put_write_access(inode);
@@ -157,7 +158,7 @@ COMPAT_SYSCALL_DEFINE2(truncate, const char __user *, path, compat_off_t, length
 long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 {
 	struct inode *inode;
-	struct dentry *dentry;
+	struct path *path;
 	struct fd f;
 	int error;
 
@@ -173,8 +174,8 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 	if (f.file->f_flags & O_LARGEFILE)
 		small = 0;
 
-	dentry = f.file->f_path.dentry;
-	inode = dentry->d_inode;
+	path = &f.file->f_path;
+	inode = d_inode(path->dentry);
 	error = -EINVAL;
 	if (!S_ISREG(inode->i_mode) || !(f.file->f_mode & FMODE_WRITE))
 		goto out_putf;
@@ -191,7 +192,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 	sb_start_write(inode->i_sb);
 	error = security_file_truncate(f.file);
 	if (!error)
-		error = do_truncate(file_mnt_idmap(f.file), dentry, length,
+		error = do_truncate(file_mnt_idmap(f.file), path, length,
 				    ATTR_MTIME | ATTR_CTIME, f.file);
 	sb_end_write(inode->i_sb);
 out_putf:
@@ -640,7 +641,7 @@ int chmod_common(const struct path *path, umode_t mode)
 		goto out_unlock;
 	newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
 	newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
-	error = notify_change(mnt_idmap(path->mnt), path->dentry,
+	error = notify_change(mnt_idmap(path->mnt), path,
 			      &newattrs, &delegated_inode);
 out_unlock:
 	inode_unlock(inode);
@@ -771,7 +772,7 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
 		from_vfsuid(idmap, fs_userns, newattrs.ia_vfsuid),
 		from_vfsgid(idmap, fs_userns, newattrs.ia_vfsgid));
 	if (!error)
-		error = notify_change(idmap, path->dentry, &newattrs,
+		error = notify_change(idmap, path, &newattrs,
 				      &delegated_inode);
 	inode_unlock(inode);
 	if (delegated_inode) {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 4d0b278f5630..d1a1eaa1c00c 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -141,7 +141,9 @@ static inline int ovl_do_notify_change(struct ovl_fs *ofs,
 				       struct dentry *upperdentry,
 				       struct iattr *attr)
 {
-	return notify_change(ovl_upper_mnt_idmap(ofs), upperdentry, attr, NULL);
+	struct path path = { .mnt = ovl_upper_mnt(ofs), .dentry = upperdentry };
+
+	return notify_change(ovl_upper_mnt_idmap(ofs), &path, attr, NULL);
 }
 
 static inline int ovl_do_rmdir(struct ovl_fs *ofs,
diff --git a/fs/utimes.c b/fs/utimes.c
index 3701b3946f88..1e6b82b27899 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -63,7 +63,7 @@ int vfs_utimes(const struct path *path, struct timespec64 *times)
 	}
 retry_deleg:
 	inode_lock(inode);
-	error = notify_change(mnt_idmap(path->mnt), path->dentry, &newattrs,
+	error = notify_change(mnt_idmap(path->mnt), path, &newattrs,
 			      &delegated_inode);
 	inode_unlock(inode);
 	if (delegated_inode) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 21a981680856..dbba36ab4a1b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2333,7 +2333,7 @@ static inline bool is_idmapped_mnt(const struct vfsmount *mnt)
 }
 
 extern long vfs_truncate(const struct path *, loff_t);
-int do_truncate(struct mnt_idmap *, struct dentry *, loff_t start,
+int do_truncate(struct mnt_idmap *, const struct path *, loff_t start,
 		unsigned int time_attrs, struct file *filp);
 extern int vfs_fallocate(struct file *file, int mode, loff_t offset,
 			loff_t len);
@@ -2488,7 +2488,7 @@ static inline int bmap(struct inode *inode,  sector_t *block)
 }
 #endif
 
-int notify_change(struct mnt_idmap *, struct dentry *,
+int notify_change(struct mnt_idmap *, const struct path *,
 		  struct iattr *, struct inode **);
 int inode_permission(struct mnt_idmap *, struct inode *, int);
 int generic_permission(struct mnt_idmap *, struct inode *, int);
-- 
2.17.1

[PATCH -next 2/2] lsm: Change inode_setattr hook to take struct path argument

[Xiu Jianfeng]

For path-based LSMs such as Landlock, struct path instead of struct
dentry is required to make sense of attr/xattr accesses. So change the
argument of lsm hook inode_setattr() from struct dentry * to struct
path *.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 fs/attr.c                     |  2 +-
 fs/fat/file.c                 |  2 +-
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      |  4 ++--
 security/security.c           | 10 +++++-----
 security/selinux/hooks.c      |  3 ++-
 security/smack/smack_lsm.c    |  5 +++--
 7 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index eecd78944b83..54d4334c350f 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -473,7 +473,7 @@ int notify_change(struct mnt_idmap *idmap, const struct path *path,
 	    !vfsgid_valid(i_gid_into_vfsgid(idmap, inode)))
 		return -EOVERFLOW;
 
-	error = security_inode_setattr(idmap, dentry, attr);
+	error = security_inode_setattr(idmap, path, attr);
 	if (error)
 		return error;
 	error = try_break_deleg(inode, delegated_inode);
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 795a4fad5c40..bb31663f99b5 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -91,7 +91,7 @@ static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr)
 	 * module, just because it maps to a file mode.
 	 */
 	err = security_inode_setattr(file_mnt_idmap(file),
-				     file->f_path.dentry, &ia);
+				     &file->f_path, &ia);
 	if (err)
 		goto out_unlock_inode;
 
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 6bb55e61e8e8..542fa6ab87c5 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -134,7 +134,7 @@ LSM_HOOK(int, 0, inode_readlink, struct dentry *dentry)
 LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
 	 bool rcu)
 LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
-LSM_HOOK(int, 0, inode_setattr, struct dentry *dentry, struct iattr *attr)
+LSM_HOOK(int, 0, inode_setattr, const struct path *path, struct iattr *attr)
 LSM_HOOK(int, 0, inode_getattr, const struct path *path)
 LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *name, const void *value,
diff --git a/include/linux/security.h b/include/linux/security.h
index e2734e9e44d5..9121f86feed1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -353,7 +353,7 @@ int security_inode_follow_link(struct dentry *dentry, struct inode *inode,
 			       bool rcu);
 int security_inode_permission(struct inode *inode, int mask);
 int security_inode_setattr(struct mnt_idmap *idmap,
-			   struct dentry *dentry, struct iattr *attr);
+			   const struct path *path, struct iattr *attr);
 int security_inode_getattr(const struct path *path);
 int security_inode_setxattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, const char *name,
@@ -849,7 +849,7 @@ static inline int security_inode_permission(struct inode *inode, int mask)
 }
 
 static inline int security_inode_setattr(struct mnt_idmap *idmap,
-					 struct dentry *dentry,
+					 const struct path *path,
 					 struct iattr *attr)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index d5ff7ff45b77..2ce7194fdb5c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2075,7 +2075,7 @@ int security_inode_permission(struct inode *inode, int mask)
 /**
  * security_inode_setattr() - Check if setting file attributes is allowed
  * @idmap: idmap of the mount
- * @dentry: file
+ * @path: path of file
  * @attr: new attributes
  *
  * Check permission before setting file attributes.  Note that the kernel call
@@ -2086,16 +2086,16 @@ int security_inode_permission(struct inode *inode, int mask)
  * Return: Returns 0 if permission is granted.
  */
 int security_inode_setattr(struct mnt_idmap *idmap,
-			   struct dentry *dentry, struct iattr *attr)
+			   const struct path *path, struct iattr *attr)
 {
 	int ret;
 
-	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
 		return 0;
-	ret = call_int_hook(inode_setattr, 0, dentry, attr);
+	ret = call_int_hook(inode_setattr, 0, path, attr);
 	if (ret)
 		return ret;
-	return evm_inode_setattr(idmap, dentry, attr);
+	return evm_inode_setattr(idmap, path->dentry, attr);
 }
 EXPORT_SYMBOL_GPL(security_inode_setattr);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 79b4890e9936..81abaea4dd63 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3051,9 +3051,10 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 	return rc;
 }
 
-static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
+static int selinux_inode_setattr(const struct path *path, struct iattr *iattr)
 {
 	const struct cred *cred = current_cred();
+	struct dentry *dentry = path->dentry;
 	struct inode *inode = d_backing_inode(dentry);
 	unsigned int ia_valid = iattr->ia_valid;
 	__u32 av = FILE__WRITE;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 7a3e9ab137d8..0b2931c87507 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1147,14 +1147,15 @@ static int smack_inode_permission(struct inode *inode, int mask)
 
 /**
  * smack_inode_setattr - Smack check for setting attributes
- * @dentry: the object
+ * @path: path of the object
  * @iattr: for the force flag
  *
  * Returns 0 if access is permitted, an error code otherwise
  */
-static int smack_inode_setattr(struct dentry *dentry, struct iattr *iattr)
+static int smack_inode_setattr(const struct path *path, struct iattr *iattr)
 {
 	struct smk_audit_info ad;
+	struct dentry *dentry = path->dentry;
 	int rc;
 
 	/*
-- 
2.17.1

Re: [PATCH -next 1/2] Change notify_change() to take struct path argument

[Chuck Lever III]

> On May 5, 2023, at 4:11 AM, Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
> 
> For path-based LSMs such as Landlock, struct path instead of struct
> dentry is required to make sense of attr/xattr accesses. notify_change()
> is the main caller of security_inode_setattr(), so refactor it first
> before lsm hook inode_setattr().
> 
> This patch also touches do_truncate() and other related callers.
> 
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
> drivers/base/devtmpfs.c   |  5 +++--
> fs/attr.c                 |  5 +++--
> fs/cachefiles/interface.c |  4 ++--
> fs/coredump.c             |  2 +-
> fs/ecryptfs/inode.c       | 18 +++++++++---------
> fs/inode.c                |  8 +++++---
> fs/ksmbd/smb2pdu.c        |  6 +++---
> fs/ksmbd/smbacl.c         |  2 +-
> fs/namei.c                |  2 +-
> fs/nfsd/vfs.c             | 12 ++++++++----
> fs/open.c                 | 19 ++++++++++---------
> fs/overlayfs/overlayfs.h  |  4 +++-
> fs/utimes.c               |  2 +-
> include/linux/fs.h        |  4 ++--
> 14 files changed, 52 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> index b848764ef018..e67dd258984c 100644
> --- a/drivers/base/devtmpfs.c
> +++ b/drivers/base/devtmpfs.c
> @@ -220,13 +220,14 @@ static int handle_create(const char *nodename, umode_t mode, kuid_t uid,
> dev->devt);
> if (!err) {
> struct iattr newattrs;
> + struct path new = { .mnt = path.mnt, .dentry = dentry };
> 
> newattrs.ia_mode = mode;
> newattrs.ia_uid = uid;
> newattrs.ia_gid = gid;
> newattrs.ia_valid = ATTR_MODE|ATTR_UID|ATTR_GID;
> inode_lock(d_inode(dentry));
> - notify_change(&nop_mnt_idmap, dentry, &newattrs, NULL);
> + notify_change(&nop_mnt_idmap, &new, &newattrs, NULL);
> inode_unlock(d_inode(dentry));
> 
> /* mark as kernel-created inode */
> @@ -334,7 +335,7 @@ static int handle_remove(const char *nodename, struct device *dev)
> newattrs.ia_valid =
> ATTR_UID|ATTR_GID|ATTR_MODE;
> inode_lock(d_inode(dentry));
> - notify_change(&nop_mnt_idmap, dentry, &newattrs, NULL);
> + notify_change(&nop_mnt_idmap, &p, &newattrs, NULL);
> inode_unlock(d_inode(dentry));
> err = vfs_unlink(&nop_mnt_idmap, d_inode(parent.dentry),
> dentry, NULL);
> diff --git a/fs/attr.c b/fs/attr.c
> index d60dc1edb526..eecd78944b83 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -354,7 +354,7 @@ EXPORT_SYMBOL(may_setattr);
> /**
>  * notify_change - modify attributes of a filesytem object
>  * @idmap: idmap of the mount the inode was found from
> - * @dentry: object affected
> + * @path: path of object affected
>  * @attr: new attributes
>  * @delegated_inode: returns inode, if the inode is delegated
>  *
> @@ -378,9 +378,10 @@ EXPORT_SYMBOL(may_setattr);
>  * permissions. On non-idmapped mounts or if permission checking is to be
>  * performed on the raw inode simply pass @nop_mnt_idmap.
>  */
> -int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
> +int notify_change(struct mnt_idmap *idmap, const struct path *path,
>  struct iattr *attr, struct inode **delegated_inode)
> {
> + struct dentry *dentry = path->dentry;
> struct inode *inode = dentry->d_inode;
> umode_t mode = inode->i_mode;
> int error;
> diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
> index 40052bdb3365..4700285a76f0 100644
> --- a/fs/cachefiles/interface.c
> +++ b/fs/cachefiles/interface.c
> @@ -138,7 +138,7 @@ static int cachefiles_adjust_size(struct cachefiles_object *object)
> newattrs.ia_size = oi_size & PAGE_MASK;
> ret = cachefiles_inject_remove_error();
> if (ret == 0)
> - ret = notify_change(&nop_mnt_idmap, file->f_path.dentry,
> + ret = notify_change(&nop_mnt_idmap, &file->f_path,
>    &newattrs, NULL);
> if (ret < 0)
> goto truncate_failed;
> @@ -148,7 +148,7 @@ static int cachefiles_adjust_size(struct cachefiles_object *object)
> newattrs.ia_size = ni_size;
> ret = cachefiles_inject_write_error();
> if (ret == 0)
> - ret = notify_change(&nop_mnt_idmap, file->f_path.dentry,
> + ret = notify_change(&nop_mnt_idmap, &file->f_path,
>    &newattrs, NULL);
> 
> truncate_failed:
> diff --git a/fs/coredump.c b/fs/coredump.c
> index ece7badf701b..01bef4830bfa 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -736,7 +736,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> }
> if (!(cprm.file->f_mode & FMODE_CAN_WRITE))
> goto close_fail;
> - if (do_truncate(idmap, cprm.file->f_path.dentry,
> + if (do_truncate(idmap, &cprm.file->f_path,
> 0, 0, cprm.file))
> goto close_fail;
> }
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 83274915ba6d..423bd457623e 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -853,12 +853,12 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length)
> 
> rc = truncate_upper(dentry, &ia, &lower_ia);
> if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
> - struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> + const struct path *lower_path = ecryptfs_dentry_to_lower_path(dentry);
> 
> - inode_lock(d_inode(lower_dentry));
> - rc = notify_change(&nop_mnt_idmap, lower_dentry,
> + inode_lock(d_inode(lower_path->dentry));
> + rc = notify_change(&nop_mnt_idmap, lower_path,
>   &lower_ia, NULL);
> - inode_unlock(d_inode(lower_dentry));
> + inode_unlock(d_inode(lower_path->dentry));
> }
> return rc;
> }
> @@ -888,7 +888,7 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
>    struct dentry *dentry, struct iattr *ia)
> {
> int rc = 0;
> - struct dentry *lower_dentry;
> + const struct path *lower_path;
> struct iattr lower_ia;
> struct inode *inode;
> struct inode *lower_inode;
> @@ -902,7 +902,7 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
> }
> inode = d_inode(dentry);
> lower_inode = ecryptfs_inode_to_lower(inode);
> - lower_dentry = ecryptfs_dentry_to_lower(dentry);
> + lower_path = ecryptfs_dentry_to_lower_path(dentry);
> mutex_lock(&crypt_stat->cs_mutex);
> if (d_is_dir(dentry))
> crypt_stat->flags &= ~(ECRYPTFS_ENCRYPTED);
> @@ -964,9 +964,9 @@ static int ecryptfs_setattr(struct mnt_idmap *idmap,
> if (lower_ia.ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID))
> lower_ia.ia_valid &= ~ATTR_MODE;
> 
> - inode_lock(d_inode(lower_dentry));
> - rc = notify_change(&nop_mnt_idmap, lower_dentry, &lower_ia, NULL);
> - inode_unlock(d_inode(lower_dentry));
> + inode_lock(d_inode(lower_path->dentry));
> + rc = notify_change(&nop_mnt_idmap, lower_path, &lower_ia, NULL);
> + inode_unlock(d_inode(lower_path->dentry));
> out:
> fsstack_copy_attr_all(inode, lower_inode);
> return rc;
> diff --git a/fs/inode.c b/fs/inode.c
> index 577799b7855f..4fa51d46f655 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1973,7 +1973,7 @@ int dentry_needs_remove_privs(struct mnt_idmap *idmap,
> }
> 
> static int __remove_privs(struct mnt_idmap *idmap,
> -  struct dentry *dentry, int kill)
> +  struct path *path, int kill)
> {
> struct iattr newattrs;
> 
> @@ -1982,13 +1982,15 @@ static int __remove_privs(struct mnt_idmap *idmap,
> * Note we call this on write, so notify_change will not
> * encounter any conflicting delegations:
> */
> - return notify_change(idmap, dentry, &newattrs, NULL);
> + return notify_change(idmap, path, &newattrs, NULL);
> }
> 
> static int __file_remove_privs(struct file *file, unsigned int flags)
> {
> struct dentry *dentry = file_dentry(file);
> struct inode *inode = file_inode(file);
> + /* this path maybe incorrect */
> + struct path path = {.mnt = file->f_path.mnt, .dentry = dentry};
> int error = 0;
> int kill;
> 
> @@ -2003,7 +2005,7 @@ static int __file_remove_privs(struct file *file, unsigned int flags)
> if (flags & IOCB_NOWAIT)
> return -EAGAIN;
> 
> - error = __remove_privs(file_mnt_idmap(file), dentry, kill);
> + error = __remove_privs(file_mnt_idmap(file), &path, kill);
> }
> 
> if (!error)
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index cb93fd231f4e..2b7e5c446397 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -5644,8 +5644,8 @@ static int set_file_basic_info(struct ksmbd_file *fp,
> }
> 
> if (attrs.ia_valid) {
> - struct dentry *dentry = filp->f_path.dentry;
> - struct inode *inode = d_inode(dentry);
> + struct path *path = &filp->f_path;
> + struct inode *inode = d_inode(path->dentry);
> 
> if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> return -EACCES;
> @@ -5653,7 +5653,7 @@ static int set_file_basic_info(struct ksmbd_file *fp,
> inode_lock(inode);
> inode->i_ctime = attrs.ia_ctime;
> attrs.ia_valid &= ~ATTR_CTIME;
> - rc = notify_change(idmap, dentry, &attrs, NULL);
> + rc = notify_change(idmap, path, &attrs, NULL);
> inode_unlock(inode);
> }
> return rc;
> diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
> index 6d6cfb6957a9..39d8aff3ae1b 100644
> --- a/fs/ksmbd/smbacl.c
> +++ b/fs/ksmbd/smbacl.c
> @@ -1403,7 +1403,7 @@ int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
> }
> 
> inode_lock(inode);
> - rc = notify_change(idmap, path->dentry, &newattrs, NULL);
> + rc = notify_change(idmap, path, &newattrs, NULL);
> inode_unlock(inode);
> if (rc)
> goto out;
> diff --git a/fs/namei.c b/fs/namei.c
> index e4fe0879ae55..ec7075a8505d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3292,7 +3292,7 @@ static int handle_truncate(struct mnt_idmap *idmap, struct file *filp)
> 
> error = security_file_truncate(filp);
> if (!error) {
> - error = do_truncate(idmap, path->dentry, 0,
> + error = do_truncate(idmap, path, 0,
>    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
>    filp);
> }
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index bb9d47172162..4b51fd2f05e3 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -410,7 +410,7 @@ nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
> return nfserrno(get_write_access(inode));
> }
> 
> -static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
> +static int __nfsd_setattr(struct path *path, struct iattr *iap)
> {
> int host_err;
> 
> @@ -430,7 +430,7 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
> if (iap->ia_size < 0)
> return -EFBIG;
> 
> - host_err = notify_change(&nop_mnt_idmap, dentry, &size_attr, NULL);
> + host_err = notify_change(&nop_mnt_idmap, path, &size_attr, NULL);
> if (host_err)
> return host_err;
> iap->ia_valid &= ~ATTR_SIZE;
> @@ -448,7 +448,7 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
> return 0;
> 
> iap->ia_valid |= ATTR_CTIME;
> - return notify_change(&nop_mnt_idmap, dentry, iap, NULL);
> + return notify_change(&nop_mnt_idmap, path, iap, NULL);
> }
> 
> /**
> @@ -471,6 +471,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>     struct nfsd_attrs *attr,
>     int check_guard, time64_t guardtime)
> {
> + struct path path;
> struct dentry *dentry;
> struct inode *inode;
> struct iattr *iap = attr->na_iattr;
> @@ -534,9 +535,12 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> return err;
> }
> 
> + path.mnt = fhp->fh_export->ex_path.mnt;
> + path.dentry = fhp->fh_dentry;
> +
> inode_lock(inode);
> for (retries = 1;;) {
> - host_err = __nfsd_setattr(dentry, iap);
> + host_err = __nfsd_setattr(&path, iap);
> if (host_err != -EAGAIN || !retries--)
> break;
> if (!nfsd_wait_for_delegreturn(rqstp, inode))

Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>


> diff --git a/fs/open.c b/fs/open.c
> index 4478adcc4f3a..7a7841606285 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -37,11 +37,12 @@
> 
> #include "internal.h"
> 
> -int do_truncate(struct mnt_idmap *idmap, struct dentry *dentry,
> +int do_truncate(struct mnt_idmap *idmap, const struct path *path,
> loff_t length, unsigned int time_attrs, struct file *filp)
> {
> int ret;
> struct iattr newattrs;
> + struct dentry *dentry = path->dentry;
> 
> /* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
> if (length < 0)
> @@ -63,7 +64,7 @@ int do_truncate(struct mnt_idmap *idmap, struct dentry *dentry,
> 
> inode_lock(dentry->d_inode);
> /* Note any delegations or leases have already been broken: */
> - ret = notify_change(idmap, dentry, &newattrs, NULL);
> + ret = notify_change(idmap, path, &newattrs, NULL);
> inode_unlock(dentry->d_inode);
> return ret;
> }
> @@ -109,7 +110,7 @@ long vfs_truncate(const struct path *path, loff_t length)
> 
> error = security_path_truncate(path);
> if (!error)
> - error = do_truncate(idmap, path->dentry, length, 0, NULL);
> + error = do_truncate(idmap, path, length, 0, NULL);
> 
> put_write_and_out:
> put_write_access(inode);
> @@ -157,7 +158,7 @@ COMPAT_SYSCALL_DEFINE2(truncate, const char __user *, path, compat_off_t, length
> long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
> {
> struct inode *inode;
> - struct dentry *dentry;
> + struct path *path;
> struct fd f;
> int error;
> 
> @@ -173,8 +174,8 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
> if (f.file->f_flags & O_LARGEFILE)
> small = 0;
> 
> - dentry = f.file->f_path.dentry;
> - inode = dentry->d_inode;
> + path = &f.file->f_path;
> + inode = d_inode(path->dentry);
> error = -EINVAL;
> if (!S_ISREG(inode->i_mode) || !(f.file->f_mode & FMODE_WRITE))
> goto out_putf;
> @@ -191,7 +192,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
> sb_start_write(inode->i_sb);
> error = security_file_truncate(f.file);
> if (!error)
> - error = do_truncate(file_mnt_idmap(f.file), dentry, length,
> + error = do_truncate(file_mnt_idmap(f.file), path, length,
>    ATTR_MTIME | ATTR_CTIME, f.file);
> sb_end_write(inode->i_sb);
> out_putf:
> @@ -640,7 +641,7 @@ int chmod_common(const struct path *path, umode_t mode)
> goto out_unlock;
> newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
> newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
> - error = notify_change(mnt_idmap(path->mnt), path->dentry,
> + error = notify_change(mnt_idmap(path->mnt), path,
>      &newattrs, &delegated_inode);
> out_unlock:
> inode_unlock(inode);
> @@ -771,7 +772,7 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
> from_vfsuid(idmap, fs_userns, newattrs.ia_vfsuid),
> from_vfsgid(idmap, fs_userns, newattrs.ia_vfsgid));
> if (!error)
> - error = notify_change(idmap, path->dentry, &newattrs,
> + error = notify_change(idmap, path, &newattrs,
>      &delegated_inode);
> inode_unlock(inode);
> if (delegated_inode) {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4d0b278f5630..d1a1eaa1c00c 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -141,7 +141,9 @@ static inline int ovl_do_notify_change(struct ovl_fs *ofs,
>       struct dentry *upperdentry,
>       struct iattr *attr)
> {
> - return notify_change(ovl_upper_mnt_idmap(ofs), upperdentry, attr, NULL);
> + struct path path = { .mnt = ovl_upper_mnt(ofs), .dentry = upperdentry };
> +
> + return notify_change(ovl_upper_mnt_idmap(ofs), &path, attr, NULL);
> }
> 
> static inline int ovl_do_rmdir(struct ovl_fs *ofs,
> diff --git a/fs/utimes.c b/fs/utimes.c
> index 3701b3946f88..1e6b82b27899 100644
> --- a/fs/utimes.c
> +++ b/fs/utimes.c
> @@ -63,7 +63,7 @@ int vfs_utimes(const struct path *path, struct timespec64 *times)
> }
> retry_deleg:
> inode_lock(inode);
> - error = notify_change(mnt_idmap(path->mnt), path->dentry, &newattrs,
> + error = notify_change(mnt_idmap(path->mnt), path, &newattrs,
>      &delegated_inode);
> inode_unlock(inode);
> if (delegated_inode) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 21a981680856..dbba36ab4a1b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2333,7 +2333,7 @@ static inline bool is_idmapped_mnt(const struct vfsmount *mnt)
> }
> 
> extern long vfs_truncate(const struct path *, loff_t);
> -int do_truncate(struct mnt_idmap *, struct dentry *, loff_t start,
> +int do_truncate(struct mnt_idmap *, const struct path *, loff_t start,
> unsigned int time_attrs, struct file *filp);
> extern int vfs_fallocate(struct file *file, int mode, loff_t offset,
> loff_t len);
> @@ -2488,7 +2488,7 @@ static inline int bmap(struct inode *inode,  sector_t *block)
> }
> #endif
> 
> -int notify_change(struct mnt_idmap *, struct dentry *,
> +int notify_change(struct mnt_idmap *, const struct path *,
>  struct iattr *, struct inode **);
> int inode_permission(struct mnt_idmap *, struct inode *, int);
> int generic_permission(struct mnt_idmap *, struct inode *, int);
> -- 
> 2.17.1
> 

--
Chuck Lever

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[xiujianfeng]

sorry, I forgot to add the link to preview discussion:

https://lore.kernel.org/all/20220827111215.131442-1-xiujianfeng@huawei.com/

On 2023/5/5 16:11, Xiu Jianfeng wrote:
> Hi,
> 
> I am working on adding xattr/attr support for landlock [1], so we can
> control fs accesses such as chmod, chown, uptimes, setxattr, etc.. inside
> landlock sandbox. the LSM hooks as following are invoved:
> 1.inode_setattr
> 2.inode_setxattr
> 3.inode_removexattr
> 4.inode_set_acl
> 5.inode_remove_acl
> which are controlled by LANDLOCK_ACCESS_FS_WRITE_METADATA.
> 
> and
> 1.inode_getattr
> 2.inode_get_acl
> 3.inode_getxattr
> 4.inode_listxattr
> which are controlled by LANDLOCK_ACCESS_FS_READ_METADATA
> 
> Some of these hooks only take struct dentry as a argument, However, for
> path-based LSMs such Landlock, Apparmor and Tomoyo, struct path instead
> of struct dentry required to make sense of attr/xattr accesses. So we
> need to refactor these hooks to take a struct path argument.
> 
> This patchset only refators inode_setattr hook as part of whole work.
> 
> Also, I have a problem about file_dentry() in __file_remove_privs() of the
> first patch, before changes in commit c1892c37769cf ("vfs: fix deadlock in
> file_remove_privs() on overlayfs"), it gets dentry and inode as belows:
> 
> struct dentry *dentry = file->f_path.dentry;
> struct inode *inode = d_inode(dentry);
> 
> That would be clear to change it to pass &file->f_path to
> __remove_privs()->notify_change()->inode_setattr().
> After that commit, it has been changed to:
> 
> struct dentry *dentry = file_dentry(file);
> struct inode *inode = file_inode(file);
> 
> If I understand correctly, the dentry from file_dentry() maybe the upper
> or the lower, it can be different from file->f_path.dentry. It can't just
> go back to use &file->f_path otherwise the bug will come back for
> overlayfs. So for such scenario, how to get a path from file if the file
> maybe or not from overlayfs, and which kind of overlayfs path is ok for
> Landlock?
> 
> Xiu Jianfeng (2):
>   fs: Change notify_change() to take struct path argument
>   lsm: Change inode_setattr hook to take struct path argument
> 
>  drivers/base/devtmpfs.c       |  5 +++--
>  fs/attr.c                     |  7 ++++---
>  fs/cachefiles/interface.c     |  4 ++--
>  fs/coredump.c                 |  2 +-
>  fs/ecryptfs/inode.c           | 18 +++++++++---------
>  fs/fat/file.c                 |  2 +-
>  fs/inode.c                    |  8 +++++---
>  fs/ksmbd/smb2pdu.c            |  6 +++---
>  fs/ksmbd/smbacl.c             |  2 +-
>  fs/namei.c                    |  2 +-
>  fs/nfsd/vfs.c                 | 12 ++++++++----
>  fs/open.c                     | 19 ++++++++++---------
>  fs/overlayfs/overlayfs.h      |  4 +++-
>  fs/utimes.c                   |  2 +-
>  include/linux/fs.h            |  4 ++--
>  include/linux/lsm_hook_defs.h |  2 +-
>  include/linux/security.h      |  4 ++--
>  security/security.c           | 10 +++++-----
>  security/selinux/hooks.c      |  3 ++-
>  security/smack/smack_lsm.c    |  5 +++--
>  20 files changed, 67 insertions(+), 54 deletions(-)
> 

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Christian Brauner]

On Fri, May 05, 2023 at 04:11:58PM +0800, Xiu Jianfeng wrote:
> Hi,
> 
> I am working on adding xattr/attr support for landlock [1], so we can
> control fs accesses such as chmod, chown, uptimes, setxattr, etc.. inside
> landlock sandbox. the LSM hooks as following are invoved:
> 1.inode_setattr
> 2.inode_setxattr
> 3.inode_removexattr
> 4.inode_set_acl
> 5.inode_remove_acl
> which are controlled by LANDLOCK_ACCESS_FS_WRITE_METADATA.
> 
> and
> 1.inode_getattr
> 2.inode_get_acl
> 3.inode_getxattr
> 4.inode_listxattr
> which are controlled by LANDLOCK_ACCESS_FS_READ_METADATA

It would be helpful to get the complete, full picture.

Piecemeal extending vfs helpers with struct path arguments is costly,
will cause a lot of churn and will require a lot of review time from us.

Please give us the list of all security hooks to which you want to pass
a struct path (if there are more to come apart from the ones listed
here). Then please follow all callchains and identify the vfs helpers
that would need to be updated. Then please figure out where those
vfs helpers are called from and follow all callchains finding all
inode_operations that would have to be updated and passed a struct path
argument. So ultimately we'll end up with a list of vfs helpers and
inode_operations that would have to be changed.

I'm very reluctant to see anything merged without knowing _exactly_ what
you're getting us into.

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Mickaël Salaün]

On 15/05/2023 17:12, Christian Brauner wrote:
> On Fri, May 05, 2023 at 04:11:58PM +0800, Xiu Jianfeng wrote:
>> Hi,
>>
>> I am working on adding xattr/attr support for landlock [1], so we can
>> control fs accesses such as chmod, chown, uptimes, setxattr, etc.. inside
>> landlock sandbox. the LSM hooks as following are invoved:
>> 1.inode_setattr
>> 2.inode_setxattr
>> 3.inode_removexattr
>> 4.inode_set_acl
>> 5.inode_remove_acl
>> which are controlled by LANDLOCK_ACCESS_FS_WRITE_METADATA.
>>
>> and
>> 1.inode_getattr
>> 2.inode_get_acl
>> 3.inode_getxattr
>> 4.inode_listxattr
>> which are controlled by LANDLOCK_ACCESS_FS_READ_METADATA
> 
> It would be helpful to get the complete, full picture.
> 
> Piecemeal extending vfs helpers with struct path arguments is costly,
> will cause a lot of churn and will require a lot of review time from us.
> 
> Please give us the list of all security hooks to which you want to pass
> a struct path (if there are more to come apart from the ones listed
> here). Then please follow all callchains and identify the vfs helpers
> that would need to be updated. Then please figure out where those
> vfs helpers are called from and follow all callchains finding all
> inode_operations that would have to be updated and passed a struct path
> argument. So ultimately we'll end up with a list of vfs helpers and
> inode_operations that would have to be changed.
> 
> I'm very reluctant to see anything merged without knowing _exactly_ what
> you're getting us into.

Ultimately we'd like the path-based LSMs to reach parity with the 
inode-based LSMs. This proposal's goal is to provide users the ability 
to control (in a complete and easy way) file metadata access. For these 
we need to extend the inode_*attr hooks and inode_*acl hooks to handle 
paths. The chown/chmod hooks are already good.

In the future, I'd also like to be able to control directory traversals 
(e.g. chdir), which currently only calls inode_permission().

What would be the best way to reach this goal?

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Christian Brauner]

On Fri, May 26, 2023 at 06:33:05PM +0200, Mickaël Salaün wrote:
> 
> On 15/05/2023 17:12, Christian Brauner wrote:
> > On Fri, May 05, 2023 at 04:11:58PM +0800, Xiu Jianfeng wrote:
> > > Hi,
> > > 
> > > I am working on adding xattr/attr support for landlock [1], so we can
> > > control fs accesses such as chmod, chown, uptimes, setxattr, etc.. inside
> > > landlock sandbox. the LSM hooks as following are invoved:
> > > 1.inode_setattr
> > > 2.inode_setxattr
> > > 3.inode_removexattr
> > > 4.inode_set_acl
> > > 5.inode_remove_acl
> > > which are controlled by LANDLOCK_ACCESS_FS_WRITE_METADATA.
> > > 
> > > and
> > > 1.inode_getattr
> > > 2.inode_get_acl
> > > 3.inode_getxattr
> > > 4.inode_listxattr
> > > which are controlled by LANDLOCK_ACCESS_FS_READ_METADATA
> > 
> > It would be helpful to get the complete, full picture.
> > 
> > Piecemeal extending vfs helpers with struct path arguments is costly,
> > will cause a lot of churn and will require a lot of review time from us.
> > 
> > Please give us the list of all security hooks to which you want to pass
> > a struct path (if there are more to come apart from the ones listed
> > here). Then please follow all callchains and identify the vfs helpers
> > that would need to be updated. Then please figure out where those
> > vfs helpers are called from and follow all callchains finding all
> > inode_operations that would have to be updated and passed a struct path
> > argument. So ultimately we'll end up with a list of vfs helpers and
> > inode_operations that would have to be changed.
> > 
> > I'm very reluctant to see anything merged without knowing _exactly_ what
> > you're getting us into.
> 
> Ultimately we'd like the path-based LSMs to reach parity with the
> inode-based LSMs. This proposal's goal is to provide users the ability to
> control (in a complete and easy way) file metadata access. For these we need
> to extend the inode_*attr hooks and inode_*acl hooks to handle paths. The
> chown/chmod hooks are already good.
> 
> In the future, I'd also like to be able to control directory traversals
> (e.g. chdir), which currently only calls inode_permission().
> 
> What would be the best way to reach this goal?

The main concern which was expressed on other patchsets before is that
modifying inode operations to take struct path is not the way to go.
Passing struct path into individual filesystems is a clear layering
violation for most inode operations, sometimes downright not feasible,
and in general exposing struct vfsmount to filesystems is a hard no. At
least as far as I'm concerned.

So the best way to achieve the landlock goal might be to add new hooks
in cases where you would be required to modify inode operations
otherwise. Taking the chdir() case as an example. That calls
path_permission(). Since inode_permission() and generic_permission() are
called in a lot of places where not even a dentry might be readily
available we will not extend them to take a struct path argument. This
would also involve extending the inode ->permission() method which is a
no go. That's neither feasible and would involve modifying a good chunk
of code for the sole purpose of an LSM.

So in path_permission() you might have the potential to add an LSM hook.
Or if you need to know what syscall this was called for you might have
to add a hook into chdir() itself. That is still unpleasant but since
the alternative to adding new LSM hooks might be endless layering
violations that's a compromise that at least I can live with. Ultimately
you have to convince more people.

Some concerns around passing struct path to LSM hooks in general that I
would like to just point out and ask you to keep in mind: As soon as
there's an LSM hook that takes a path argument it means all LSMs have
access to a struct path. At that point visibility into what's been done
to that struct path is lost for the fs layer.

One the one hand that's fine on the other hand sooner or later some LSM
will try to get creative and do things like starting to infer
relationships between mounts without understanding mount property and
mount handling enough, or start trying to infer the parent of a path and
perform permission checks on it in ways that aren't sane. And that sucks
because this only becomes obvious when fs wide changes are done that
affect LSM hooks as well.

And that's the other thing. The more objects the LSM layer gets access
to the greater the cost to do fs wide changes because the fs layer is
now even closer entangled with the LSM layer. For example, even simple
things like removing IOP_XATTR - even just for POSIX ACLs - suddenly
become complicated not because of the fs layer but because of how the
LSM layer makes use of it. It might start relying on internal flags that
would be revoked later and so on. That also goes for struct vfsmount. So
it means going through every LSM trying to figure out if a change is ok
or not. And we keep adding new LSMs without deprecating older ones (A
problem we also face in the fs layer.) and then they sit around but
still need to be taken into account when doing changes.

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Christoph Hellwig]

On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote:
> The main concern which was expressed on other patchsets before is that
> modifying inode operations to take struct path is not the way to go.
> Passing struct path into individual filesystems is a clear layering
> violation for most inode operations, sometimes downright not feasible,
> and in general exposing struct vfsmount to filesystems is a hard no. At
> least as far as I'm concerned.

Agreed.  Passing struct path into random places is not how the VFS works.

> So the best way to achieve the landlock goal might be to add new hooks

What is "the landlock goal", and why does it matter?

> or not. And we keep adding new LSMs without deprecating older ones (A
> problem we also face in the fs layer.) and then they sit around but
> still need to be taken into account when doing changes.

Yes, I'm really worried about th amount of LSMs we have, and the weird
things they do.

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Casey Schaufler]

On 5/30/2023 7:28 AM, Christoph Hellwig wrote:
> On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote:
>> The main concern which was expressed on other patchsets before is that
>> modifying inode operations to take struct path is not the way to go.
>> Passing struct path into individual filesystems is a clear layering
>> violation for most inode operations, sometimes downright not feasible,
>> and in general exposing struct vfsmount to filesystems is a hard no. At
>> least as far as I'm concerned.
> Agreed.  Passing struct path into random places is not how the VFS works.
>
>> So the best way to achieve the landlock goal might be to add new hooks
> What is "the landlock goal", and why does it matter?
>
>> or not. And we keep adding new LSMs without deprecating older ones (A
>> problem we also face in the fs layer.) and then they sit around but
>> still need to be taken into account when doing changes.
> Yes, I'm really worried about th amount of LSMs we have, and the weird
> things they do.

Which LSM(s) do you think ought to be deprecated? I only see one that I
might consider a candidate. As for weird behavior, that's what LSMs are
for, and the really weird ones proposed (e.g. pathname character set limitations)
(and excepting for BPF, of course) haven't gotten far.

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Christian Brauner]

On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote:
> On 5/30/2023 7:28 AM, Christoph Hellwig wrote:
> > On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote:
> >> The main concern which was expressed on other patchsets before is that
> >> modifying inode operations to take struct path is not the way to go.
> >> Passing struct path into individual filesystems is a clear layering
> >> violation for most inode operations, sometimes downright not feasible,
> >> and in general exposing struct vfsmount to filesystems is a hard no. At
> >> least as far as I'm concerned.
> > Agreed.  Passing struct path into random places is not how the VFS works.
> >
> >> So the best way to achieve the landlock goal might be to add new hooks
> > What is "the landlock goal", and why does it matter?
> >
> >> or not. And we keep adding new LSMs without deprecating older ones (A
> >> problem we also face in the fs layer.) and then they sit around but
> >> still need to be taken into account when doing changes.
> > Yes, I'm really worried about th amount of LSMs we have, and the weird
> > things they do.
> 
> Which LSM(s) do you think ought to be deprecated? I only see one that I

I don't have a good insight into what LSMs are actively used or are
effectively unused but I would be curious to hear what LSMs are
considered actively used/maintained from the LSM maintainer's
perspective.

> might consider a candidate. As for weird behavior, that's what LSMs are
> for, and the really weird ones proposed (e.g. pathname character set limitations)

If this is effectively saying that LSMs are licensed to step outside the
rules of the subsystem they're a guest in then it seems unlikely
subsystems will be very excited to let new LSM changes go in important
codepaths going forward. In fact this seems like a good argument against
it.

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Casey Schaufler]

On 5/30/2023 9:01 AM, Christian Brauner wrote:
> On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote:
>> On 5/30/2023 7:28 AM, Christoph Hellwig wrote:
>>> On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote:
>>>> The main concern which was expressed on other patchsets before is that
>>>> modifying inode operations to take struct path is not the way to go.
>>>> Passing struct path into individual filesystems is a clear layering
>>>> violation for most inode operations, sometimes downright not feasible,
>>>> and in general exposing struct vfsmount to filesystems is a hard no. At
>>>> least as far as I'm concerned.
>>> Agreed.  Passing struct path into random places is not how the VFS works.
>>>
>>>> So the best way to achieve the landlock goal might be to add new hooks
>>> What is "the landlock goal", and why does it matter?
>>>
>>>> or not. And we keep adding new LSMs without deprecating older ones (A
>>>> problem we also face in the fs layer.) and then they sit around but
>>>> still need to be taken into account when doing changes.
>>> Yes, I'm really worried about th amount of LSMs we have, and the weird
>>> things they do.
>> Which LSM(s) do you think ought to be deprecated? I only see one that I
> I don't have a good insight into what LSMs are actively used or are
> effectively unused but I would be curious to hear what LSMs are
> considered actively used/maintained from the LSM maintainer's
> perspective.

I'm not the LSM maintainer, but I've been working on the infrastructure
for quite some time. All the existing LSMs save one can readily be associated
with active systems, and the one that isn't is actively maintained. We have
not gotten into the habit of accepting LSMs upstream that don't have a real
world use.

>> might consider a candidate. As for weird behavior, that's what LSMs are
>> for, and the really weird ones proposed (e.g. pathname character set limitations)
> If this is effectively saying that LSMs are licensed to step outside the
> rules of the subsystem they're a guest in then it seems unlikely
> subsystems will be very excited to let new LSM changes go in important
> codepaths going forward. In fact this seems like a good argument against
> it.

This is an artifact of Linus' decision that security models should be
supported as add-on modules. On the one hand, all that a subsystem maintainer
needs to know about a security feature is what it needs in the way of hooks.
On the other hand, the subsystem maintainer loses control over what kinds of
things the security feature does with the available information. It's a
tension that we've had to deal with since the Orange Book days of the late
1980's. The deal has always been:

	You can have your security feature if:
	1. If I turn it off it has no performance impact
	2. I don't have to do anything to maintain it
	3. It doesn't interfere with any other system behavior
	4. You'll leave me alone

As a security developer from way back I would be delighted if maintainers of
other subsystems took an active interest in some of what we've been trying
to accomplish in the security space. If the VFS maintainers would like to
see the LSM interfaces for file systems changed I, for one, would like very
much to hear about what they'd prefer. 

We do a lot of crazy things to avoid interfering with the subsystems we
interact with. A closer developer relationship would be most welcome, so
long as it helps us achieve or goals. We get a lot of complaints about how
LSM feature perform, but no one wants to hear that a good deal of that comes
about because of what has to be done in support of 1, 2 and 3 above. Sometimes
we do stoopid things, but usually it's to avoid changes "outside our swim lane".

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Christian Brauner]

On Tue, May 30, 2023 at 03:15:01PM -0700, Casey Schaufler wrote:
> On 5/30/2023 9:01 AM, Christian Brauner wrote:
> > On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote:
> >> On 5/30/2023 7:28 AM, Christoph Hellwig wrote:
> >>> On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote:
> >>>> The main concern which was expressed on other patchsets before is that
> >>>> modifying inode operations to take struct path is not the way to go.
> >>>> Passing struct path into individual filesystems is a clear layering
> >>>> violation for most inode operations, sometimes downright not feasible,
> >>>> and in general exposing struct vfsmount to filesystems is a hard no. At
> >>>> least as far as I'm concerned.
> >>> Agreed.  Passing struct path into random places is not how the VFS works.
> >>>
> >>>> So the best way to achieve the landlock goal might be to add new hooks
> >>> What is "the landlock goal", and why does it matter?
> >>>
> >>>> or not. And we keep adding new LSMs without deprecating older ones (A
> >>>> problem we also face in the fs layer.) and then they sit around but
> >>>> still need to be taken into account when doing changes.
> >>> Yes, I'm really worried about th amount of LSMs we have, and the weird
> >>> things they do.
> >> Which LSM(s) do you think ought to be deprecated? I only see one that I
> > I don't have a good insight into what LSMs are actively used or are
> > effectively unused but I would be curious to hear what LSMs are
> > considered actively used/maintained from the LSM maintainer's
> > perspective.
> 
> I'm not the LSM maintainer, but I've been working on the infrastructure
> for quite some time. All the existing LSMs save one can readily be associated
> with active systems, and the one that isn't is actively maintained. We have
> not gotten into the habit of accepting LSMs upstream that don't have a real
> world use.
> 
> >> might consider a candidate. As for weird behavior, that's what LSMs are
> >> for, and the really weird ones proposed (e.g. pathname character set limitations)
> > If this is effectively saying that LSMs are licensed to step outside the
> > rules of the subsystem they're a guest in then it seems unlikely
> > subsystems will be very excited to let new LSM changes go in important
> > codepaths going forward. In fact this seems like a good argument against
> > it.
> 
> This is an artifact of Linus' decision that security models should be
> supported as add-on modules. On the one hand, all that a subsystem maintainer
> needs to know about a security feature is what it needs in the way of hooks.
> On the other hand, the subsystem maintainer loses control over what kinds of
> things the security feature does with the available information. It's a
> tension that we've had to deal with since the Orange Book days of the late
> 1980's. The deal has always been:
> 
> 	You can have your security feature if:
> 	1. If I turn it off it has no performance impact
> 	2. I don't have to do anything to maintain it
> 	3. It doesn't interfere with any other system behavior
> 	4. You'll leave me alone
> 
> As a security developer from way back I would be delighted if maintainers of
> other subsystems took an active interest in some of what we've been trying
> to accomplish in the security space. If the VFS maintainers would like to
> see the LSM interfaces for file systems changed I, for one, would like very
> much to hear about what they'd prefer. 

What is important for us is that the security layer must understand and
accept that some things cannot be done the way it envisions them to be
done because it would involve design compromises in the fs layer that
the fs maintainers are unwilling to make. The idea to pass struct path
to almost every security hook is a good example.

If the project is feature parity between inode and path based LSMs then
it must be clear from the start that this won't be achieved at the cost
of mixing up the layer where only dentries and inodes are relevant and
the layer where struct paths are most relevant.

> 
> We do a lot of crazy things to avoid interfering with the subsystems we
> interact with. A closer developer relationship would be most welcome, so
> long as it helps us achieve or goals. We get a lot of complaints about how
> LSM feature perform, but no one wants to hear that a good deal of that comes
> about because of what has to be done in support of 1, 2 and 3 above. Sometimes
> we do stoopid things, but usually it's to avoid changes "outside our swim lane".

I personally am not opposed to comment on patches but they will
naturally have lower priority than other things.

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Christoph Hellwig]

On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote:
> Which LSM(s) do you think ought to be deprecated?

I have no idea.  But what I want is less weirdo things messing with
VFS semantics.

>
> I only see one that I
> might consider a candidate. As for weird behavior, that's what LSMs are
> for, and the really weird ones proposed (e.g. pathname character set limitations)
> (and excepting for BPF, of course) haven't gotten far.

They haven't gotten far for a reason usually.  Trying to sneak things in
through the back door is exactly what is the problem with LSMs.

> 
---end quoted text---

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Casey Schaufler]

On 5/31/2023 6:22 AM, Christoph Hellwig wrote:
> On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote:
>> Which LSM(s) do you think ought to be deprecated?
> I have no idea.  But what I want is less weirdo things messing with
> VFS semantics.

I am curious what you consider a weirdo thing done by LSMs. Things like
io_uring are much stranger than anything an LSM does.

>
>> I only see one that I
>> might consider a candidate. As for weird behavior, that's what LSMs are
>> for, and the really weird ones proposed (e.g. pathname character set limitations)
>> (and excepting for BPF, of course) haven't gotten far.
> They haven't gotten far for a reason usually.  Trying to sneak things in
> through the back door is exactly what is the problem with LSMs.

Mostly developers play by the rules, and we don't let things sneak in. 

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Mickaël Salaün]

On 30/05/2023 16:28, Christoph Hellwig wrote:
> On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote:
>> The main concern which was expressed on other patchsets before is that
>> modifying inode operations to take struct path is not the way to go.
>> Passing struct path into individual filesystems is a clear layering
>> violation for most inode operations, sometimes downright not feasible,
>> and in general exposing struct vfsmount to filesystems is a hard no. At
>> least as far as I'm concerned.
> 
> Agreed.  Passing struct path into random places is not how the VFS works.

I understand, it makes sense for the FS layer to not get access to 
things not required. IIUC, the main issue is the layering, with LSM 
calls being sometime at the last layer.


> 
>> So the best way to achieve the landlock goal might be to add new hooks
> 
> What is "the landlock goal", and why does it matter?

Landlock's goal is to enable (unprivileged) users to set their own 
access rights for their (ephemeral) processes (on top of the existing 
access-controls of course) i.e., to sandbox applications. Landlock rules 
are defined by users, and then according to the FS topology they see. 
This means that Landlock relies on inodes and mount points to define and 
enforce a policy.


> 
>> or not. And we keep adding new LSMs without deprecating older ones (A
>> problem we also face in the fs layer.) and then they sit around but
>> still need to be taken into account when doing changes.
> 
> Yes, I'm really worried about th amount of LSMs we have, and the weird
> things they do.

About Landlock, it's a new LSM that fit an actual need. I'd be glad to 
hear about not recommended things and how to improve the situation. I 
don't know all the history between VFS and LSM.

Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

[Casey Schaufler]

On 5/31/2023 1:36 AM, Christian Brauner wrote:
> On Tue, May 30, 2023 at 03:15:01PM -0700, Casey Schaufler wrote:
>> On 5/30/2023 9:01 AM, Christian Brauner wrote:
>>> On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote:
>>>> On 5/30/2023 7:28 AM, Christoph Hellwig wrote:
>>>>> On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote:
>>>>>> The main concern which was expressed on other patchsets before is that
>>>>>> modifying inode operations to take struct path is not the way to go.
>>>>>> Passing struct path into individual filesystems is a clear layering
>>>>>> violation for most inode operations, sometimes downright not feasible,
>>>>>> and in general exposing struct vfsmount to filesystems is a hard no. At
>>>>>> least as far as I'm concerned.
>>>>> Agreed.  Passing struct path into random places is not how the VFS works.
>>>>>
>>>>>> So the best way to achieve the landlock goal might be to add new hooks
>>>>> What is "the landlock goal", and why does it matter?
>>>>>
>>>>>> or not. And we keep adding new LSMs without deprecating older ones (A
>>>>>> problem we also face in the fs layer.) and then they sit around but
>>>>>> still need to be taken into account when doing changes.
>>>>> Yes, I'm really worried about th amount of LSMs we have, and the weird
>>>>> things they do.
>>>> Which LSM(s) do you think ought to be deprecated? I only see one that I
>>> I don't have a good insight into what LSMs are actively used or are
>>> effectively unused but I would be curious to hear what LSMs are
>>> considered actively used/maintained from the LSM maintainer's
>>> perspective.
>> I'm not the LSM maintainer, but I've been working on the infrastructure
>> for quite some time. All the existing LSMs save one can readily be associated
>> with active systems, and the one that isn't is actively maintained. We have
>> not gotten into the habit of accepting LSMs upstream that don't have a real
>> world use.
>>
>>>> might consider a candidate. As for weird behavior, that's what LSMs are
>>>> for, and the really weird ones proposed (e.g. pathname character set limitations)
>>> If this is effectively saying that LSMs are licensed to step outside the
>>> rules of the subsystem they're a guest in then it seems unlikely
>>> subsystems will be very excited to let new LSM changes go in important
>>> codepaths going forward. In fact this seems like a good argument against
>>> it.
>> This is an artifact of Linus' decision that security models should be
>> supported as add-on modules. On the one hand, all that a subsystem maintainer
>> needs to know about a security feature is what it needs in the way of hooks.
>> On the other hand, the subsystem maintainer loses control over what kinds of
>> things the security feature does with the available information. It's a
>> tension that we've had to deal with since the Orange Book days of the late
>> 1980's. The deal has always been:
>>
>> 	You can have your security feature if:
>> 	1. If I turn it off it has no performance impact
>> 	2. I don't have to do anything to maintain it
>> 	3. It doesn't interfere with any other system behavior
>> 	4. You'll leave me alone
>>
>> As a security developer from way back I would be delighted if maintainers of
>> other subsystems took an active interest in some of what we've been trying
>> to accomplish in the security space. If the VFS maintainers would like to
>> see the LSM interfaces for file systems changed I, for one, would like very
>> much to hear about what they'd prefer. 
> What is important for us is that the security layer must understand and
> accept that some things cannot be done the way it envisions them to be
> done because it would involve design compromises in the fs layer that
> the fs maintainers are unwilling to make. The idea to pass struct path
> to almost every security hook is a good example.

Yes, and that's completely acceptable. What would be really great is some
guidance about what to do instead. Fishing for NAKs isn't fun for anybody.

> If the project is feature parity between inode and path based LSMs then
> it must be clear from the start that this won't be achieved at the cost
> of mixing up the layer where only dentries and inodes are relevant and
> the layer where struct paths are most relevant.

Which is a fair point, and helps those of us who don't work in the VFS
layer daily understand the rationale.

>
>> We do a lot of crazy things to avoid interfering with the subsystems we
>> interact with. A closer developer relationship would be most welcome, so
>> long as it helps us achieve or goals. We get a lot of complaints about how
>> LSM feature perform, but no one wants to hear that a good deal of that comes
>> about because of what has to be done in support of 1, 2 and 3 above. Sometimes
>> we do stoopid things, but usually it's to avoid changes "outside our swim lane".
> I personally am not opposed to comment on patches but they will
> naturally have lower priority than other things.

I can't say that I see how security features "naturally have lower priority",
but everyone has to balance things.