Transmute flag is not inheritted on overlay fs

Transmute flag is not inheritted on overlay fs

[Mengchi Cheng]

Hello,

On the overlay ext4 file system, we found that transmute flag is not
inherited by newly created sub-directories. The issue can be recreated on
the newest kernel(6.3.0-rc6) on qemux86-64 with following steps.

/data directory is mounted on /dev/vdb which is a ext4 fs. It is remounted
as an overlay again to upperdir /home/root/data.
# mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work /data
Add a new smack rule and set label and flag to /data directory.
# echo "_ system rwxatl" > /sys/fs/smackfs/load2
# chsmack -a "system" /data
# chsmack -t /data
Create directories under /data.
# mkdir -p /data/dir1/dir2
And then check the smack label of dir1 and dir2.
# chsmack /data/dir1
/data/dir1 access="system"
# chsmack /data/dir1/dir2
/data/dir1/dir2 access="_"
We can see dir1 did not inherit transmute flag from data and dir2 got the
process label.

The transmute xattr of the inode is set inside the smack_d_instantiate
which depends on SMK_INODE_CHANGED bit of isp->smk_flags. But the bit is
not set in the overlay fs mkdir function call chain. So one simple solution
we have is passing inode ptr into smack_dentry_create_files_as and set the
SMK_INODE_CHANGED bit if parent dir is transmuting. Although it looks
reasonable to me and we did not meet any issue in testing, I am not sure if
there is a better solution to it. It will be great, if experts could take
a look.


Thanks,
Mengchi Cheng

Re: Transmute flag is not inheritted on overlay fs

[Casey Schaufler]

On 4/18/2023 5:23 PM, Mengchi Cheng wrote:
> Hello,
>
> On the overlay ext4 file system, we found that transmute flag is not
> inherited by newly created sub-directories. The issue can be recreated on
> the newest kernel(6.3.0-rc6) on qemux86-64 with following steps.
>
> /data directory is mounted on /dev/vdb which is a ext4 fs. It is remounted
> as an overlay again to upperdir /home/root/data.
> # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work /data
> Add a new smack rule and set label and flag to /data directory.
> # echo "_ system rwxatl" > /sys/fs/smackfs/load2
> # chsmack -a "system" /data
> # chsmack -t /data
> Create directories under /data.
> # mkdir -p /data/dir1/dir2
> And then check the smack label of dir1 and dir2.
> # chsmack /data/dir1
> /data/dir1 access="system"
> # chsmack /data/dir1/dir2
> /data/dir1/dir2 access="_"
> We can see dir1 did not inherit transmute flag from data and dir2 got the
> process label.
>
> The transmute xattr of the inode is set inside the smack_d_instantiate
> which depends on SMK_INODE_CHANGED bit of isp->smk_flags. But the bit is
> not set in the overlay fs mkdir function call chain. So one simple solution
> we have is passing inode ptr into smack_dentry_create_files_as and set the
> SMK_INODE_CHANGED bit if parent dir is transmuting. Although it looks
> reasonable to me and we did not meet any issue in testing, I am not sure if
> there is a better solution to it. It will be great, if experts could take
> a look.

I will be happy to look at your solution. Please post a patch.

>
>
> Thanks,
> Mengchi Cheng
>

[RFC PATCH] Set SMK_INODE_CHANGED inside smack_dentry_create_files_as

[Mengchi Cheng]

On the overlay fs with smack lsm enabled, new subdir did not inherit
transmute xattr form parent dir.

One solution that can solve it is passing inode into
smack_dentry_create_files_as. And Set SMK_INODE_CHANGED to smak_flags
if directory has transmute xattr.

Reported-by: Ryan Yoon <yoonjaeh@amazon.com>
---
 fs/overlayfs/dir.c            | 2 +-
 include/linux/lsm_hook_defs.h | 2 +-
 include/linux/security.h      | 4 ++--
 security/security.c           | 4 ++--
 security/selinux/hooks.c      | 2 +-
 security/smack/smack_lsm.c    | 8 ++++++--
 6 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index fc25fb95d5fc..1b3f7f3a5468 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -598,7 +598,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		override_cred->fsgid = inode->i_gid;
 		err = security_dentry_create_files_as(dentry,
 				attr->mode, &dentry->d_name, old_cred,
-				override_cred);
+				override_cred, inode);
 		if (err) {
 			put_cred(override_cred);
 			goto out_revert_creds;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 094b76dc7164..96f1fdc21cbc 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -84,7 +84,7 @@ LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
 	 int mode, const struct qstr *name, const char **xattr_name,
 	 void **ctx, u32 *ctxlen)
 LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
-	 struct qstr *name, const struct cred *old, struct cred *new)
+	 struct qstr *name, const struct cred *old, struct cred *new, struct inode *inode)
 
 #ifdef CONFIG_SECURITY_PATH
 LSM_HOOK(int, 0, path_unlink, const struct path *dir, struct dentry *dentry)
diff --git a/include/linux/security.h b/include/linux/security.h
index 5984d0d550b4..354d68dc69c5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,7 +325,7 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
 int security_dentry_create_files_as(struct dentry *dentry, int mode,
 					struct qstr *name,
 					const struct cred *old,
-					struct cred *new);
+					struct cred *new, struct inode *inode);
 int security_path_notify(const struct path *path, u64 mask,
 					unsigned int obj_type);
 int security_inode_alloc(struct inode *inode);
@@ -756,7 +756,7 @@ static inline int security_dentry_init_security(struct dentry *dentry,
 static inline int security_dentry_create_files_as(struct dentry *dentry,
 						  int mode, struct qstr *name,
 						  const struct cred *old,
-						  struct cred *new)
+						  struct cred *new, struct inode *inode)
 {
 	return 0;
 }
diff --git a/security/security.c b/security/security.c
index cf6cc576736f..0ffe98cc57fe 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1110,10 +1110,10 @@ EXPORT_SYMBOL(security_dentry_init_security);
 
 int security_dentry_create_files_as(struct dentry *dentry, int mode,
 				    struct qstr *name,
-				    const struct cred *old, struct cred *new)
+				    const struct cred *old, struct cred *new, struct inode *inode)
 {
 	return call_int_hook(dentry_create_files_as, 0, dentry, mode,
-				name, old, new);
+				name, old, new, inode);
 }
 EXPORT_SYMBOL(security_dentry_create_files_as);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9a5bdfc21314..2addc513bbb0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2848,7 +2848,7 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
 static int selinux_dentry_create_files_as(struct dentry *dentry, int mode,
 					  struct qstr *name,
 					  const struct cred *old,
-					  struct cred *new)
+					  struct cred *new, struct inode *inode)
 {
 	u32 newsid;
 	int rc;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index cfcbb748da25..e929e3e131c2 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4739,12 +4739,14 @@ static int smack_inode_copy_up_xattr(const char *name)
 static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
 					struct qstr *name,
 					const struct cred *old,
-					struct cred *new)
+					struct cred *new,
+					struct inode *inode)
 {
 	struct task_smack *otsp = smack_cred(old);
 	struct task_smack *ntsp = smack_cred(new);
 	struct inode_smack *isp;
 	int may;
+	struct inode_smack *issp = smack_inode(inode);
 
 	/*
 	 * Use the process credential unless all of
@@ -4769,8 +4771,10 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
 		 * providing access is transmuting use the containing
 		 * directory label instead of the process label.
 		 */
-		if (may > 0 && (may & MAY_TRANSMUTE))
+		if (may > 0 && (may & MAY_TRANSMUTE)) {
 			ntsp->smk_task = isp->smk_inode;
+			issp->smk_flags |= SMK_INODE_CHANGED;
+		}
 	}
 	return 0;
 }
-- 
2.25.1

Re: Transmute flag is not inheritted on overlay fs

[Mengchi Cheng]

On Wed, 2023-04-19 02:09:37 +0000, Casey Schaufler wrote:
>
> On 4/18/2023 5:23 PM, Mengchi Cheng wrote:
> > Hello,
> >
> > On the overlay ext4 file system, we found that transmute flag is not
> > inherited by newly created sub-directories. The issue can be recreated on
> > the newest kernel(6.3.0-rc6) on qemux86-64 with following steps.
> >
> > /data directory is mounted on /dev/vdb which is a ext4 fs. It is remounted
> > as an overlay again to upperdir /home/root/data.
> > # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work /data
> > Add a new smack rule and set label and flag to /data directory.
> > # echo "_ system rwxatl" > /sys/fs/smackfs/load2
> > # chsmack -a "system" /data
> > # chsmack -t /data
> > Create directories under /data.
> > # mkdir -p /data/dir1/dir2
> > And then check the smack label of dir1 and dir2.
> > # chsmack /data/dir1
> > /data/dir1 access="system"
> > # chsmack /data/dir1/dir2
> > /data/dir1/dir2 access="_"
> > We can see dir1 did not inherit transmute flag from data and dir2 got the
> > process label.
> >
> > The transmute xattr of the inode is set inside the smack_d_instantiate
> > which depends on SMK_INODE_CHANGED bit of isp->smk_flags. But the bit is
> > not set in the overlay fs mkdir function call chain. So one simple solution
> > we have is passing inode ptr into smack_dentry_create_files_as and set the
> > SMK_INODE_CHANGED bit if parent dir is transmuting. Although it looks
> > reasonable to me and we did not meet any issue in testing, I am not sure if
> > there is a better solution to it. It will be great, if experts could take
> > a look.
> 
> I will be happy to look at your solution. Please post a patch.
>

Sorry, it takes me a while to review and send out the patch.
It contains a few files because it breaks kernel API. But the core is only
in the change of smack_dentry_create_files_as.

If Roberto's patch will work, we can drop it. I posted my concern in that
thread.
https://lore.kernel.org/all/20230419192516.757220-1-mengcc@amazon.com/
 
> >
> >
> > Thanks,
> > Mengchi Cheng
> >
>