[PATCH] overlayfs: copyup security inode field

[PATCH] overlayfs: copyup security inode field

[Zakaria ElQotbi]

SELinux (and maybe other security frameworks) relies on inode->i_security field
to perform audit of security contexts.

I think this field must be the same as the underlying filesystem, instead of
creating new fresh one at ovl_new_inode() which give an UNLABELED sid.

The issue rised when certain process (for instance Zygote) fails to perform
some actions (for instance getxattr) on Android using SEAndroid and overlyafs
with empty uppdir mounted on /system, but it succeeds in case there is not
overlayfs.

Signed-off-by: Zakaria ElQotbi <zakaria.elqotbi@redbend.com>
---
 fs/overlayfs/overlayfs.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 3495a55..d28023a 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -60,6 +60,9 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
 {
 	to->i_uid = from->i_uid;
 	to->i_gid = from->i_gid;
+#ifdef CONFIG_SECURITY
+	to->i_security = from->i_security;
+#endif
 }
 
 /* dir.c */
-- 
1.7.9.5

Re: [PATCH] overlayfs: copyup security inode field

[J. R. Okajima]

Zakaria ElQotbi:
> I think this field must be the same as the underlying filesystem, instead of
> creating new fresh one at ovl_new_inode() which give an UNLABELED sid.

When two inodes refers to a single inode_security_struct (i_security),
won't it cause a "double free" problem?


J. R. Okajima

Re: [PATCH] overlayfs: copyup security inode field

[Zakaria ElQotbi]

J. R. Okajima wrote:
> 
> When two inodes refers to a single inode_security_struct (i_security),
> won't it cause a "double free" problem?
> 

Thanks for your feedback. Below another patch that set the inode_security_struct
correctly with help of security API.

Your review are welcome.

Best Regards,
Zakaria ElQotbi

>From 934602df4e9d26e77cf04f8514079c02fe592d4a Mon Sep 17 00:00:00 2001
From: Zakaria ElQotbi <zakaria.elqotbi@redbend.com>
Date: Wed, 19 Mar 2014 11:59:09 +0100
Subject: [PATCH] overlayfs: copyup security inode field

SELinux (and maybe other security frameworks) relies on inode->i_security field
to perform audit of security contexts.

I think this field must be the same as the underlying filesystem, instead of
creating new fresh one at ovl_new_inode() which give an UNLABELED sid.

The issue rised when certain process (for instance Zygote) fails to perform
some actions (for instance getxattr) on Android using SEAndroid and overlyafs
with empty uppdir mounted on /system, but it succeeds in case there is not
overlayfs.

Signed-off-by: Zakaria ElQotbi <zakaria.elqotbi@redbend.com>
---
 fs/overlayfs/inode.c     |   25 +++++++++++++++++++++++++
 fs/overlayfs/overlayfs.h |    7 ++-----
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index dbdf58e..a6f4813 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/xattr.h>
 #include "overlayfs.h"
+#include <linux/security.h>
 
 int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 {
@@ -332,6 +333,30 @@ static const struct inode_operations ovl_symlink_inode_operations = {
 	.removexattr	= ovl_removexattr,
 };
 
+void ovl_copyattr(struct inode *from, struct inode *to)
+{
+#ifdef CONFIG_SECURITY
+	void   *secctx;
+	size_t  ctxlen;
+	int     err = -1;
+
+	err = security_inode_getsecctx(from, &secctx, &ctxlen);
+	if (!err) {
+		/*
+		 * replace the fresh inode_security_struct because it should be
+		 * the same as the real underlying inode.
+		 */
+		err = security_inode_notifysecctx(to, secctx, ctxlen);
+		security_release_secctx(secctx, ctxlen);
+	}
+	if (err) {
+		WARN(1, "cannot set security context err:%d \n", err);
+	}
+#endif
+	to->i_uid = from->i_uid;
+	to->i_gid = from->i_gid;
+}
+
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode,
 			    struct ovl_entry *oe)
 {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 3495a55..0a1bac3 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -56,11 +56,8 @@ int ovl_removexattr(struct dentry *dentry, const char *name);
 
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode,
 			    struct ovl_entry *oe);
-static inline void ovl_copyattr(struct inode *from, struct inode *to)
-{
-	to->i_uid = from->i_uid;
-	to->i_gid = from->i_gid;
-}
+
+void ovl_copyattr(struct inode *from, struct inode *to);
 
 /* dir.c */
 extern const struct inode_operations ovl_dir_inode_operations;
-- 
1.7.9.5

Re: [PATCH] overlayfs: copyup security inode field

[J. R. Okajima]

Zakaria ElQotbi:
> Thanks for your feedback. Below another patch that set the inode_security_struct
> correctly with help of security API.

I don't know much about LSM, but don't we need to acquire i_mutex to
get/set (actually notify) i_security?


J. R. Okajima

Re: [PATCH] overlayfs: copyup security inode field

[Zakaria ElQotbi]

I omitted the lock because i assumed that is wasn't necessary at inode creation,
but the ovl_copyattr() can be called from elsewhere.

Below yet another patch.

Thanks,
Zakaria
---

>From 33ca41ec2a1b9e1ff295fb4d99be958b0c7a12db Mon Sep 17 00:00:00 2001
From: Zakaria ElQotbi <zakaria.elqotbi@redbend.com>
Date: Wed, 19 Mar 2014 11:59:09 +0100
Subject: [PATCH] overlayfs: copyup security inode field

SELinux (and maybe other security frameworks) relies on inode->i_security
field to perform audit of security contexts.

I think this field must be the same as the underlying filesystem, instead
of creating new fresh one at ovl_new_inode() which give an UNLABELED sid.

The issue rised when certain process (for instance Zygote) fails to perform
some actions (for instance getxattr) on Android using SEAndroid and
overlyafs with empty uppdir mounted on /system, but it succeeds in case
there is not overlayfs.

Signed-off-by: Zakaria ElQotbi <zakaria.elqotbi@redbend.com>
---
 fs/overlayfs/inode.c     |   27 +++++++++++++++++++++++++++
 fs/overlayfs/overlayfs.h |    7 ++-----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index dbdf58e..d95b8c6 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/xattr.h>
 #include "overlayfs.h"
+#include <linux/security.h>
 
 int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 {
@@ -332,6 +333,32 @@ static const struct inode_operations ovl_symlink_inode_operations = {
 	.removexattr	= ovl_removexattr,
 };
 
+void ovl_copyattr(struct inode *from, struct inode *to)
+{
+#ifdef CONFIG_SECURITY
+	void   *secctx;
+	size_t  ctxlen;
+	int     err = -1;
+
+	err = security_inode_getsecctx(from, &secctx, &ctxlen);
+	if (!err) {
+		/*
+		 * replace the fresh inode_security_struct because it should be
+		 * the same as the real underlying inode.
+		 */
+		mutex_lock(&to->i_mutex);
+		err = security_inode_notifysecctx(to, secctx, ctxlen);
+		mutex_unlock(&to->i_mutex);
+		security_release_secctx(secctx, ctxlen);
+	}
+	if (err)
+		WARN(1, "cannot copy up security context err:%d\n", err);
+
+#endif
+	to->i_uid = from->i_uid;
+	to->i_gid = from->i_gid;
+}
+
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode,
 			    struct ovl_entry *oe)
 {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 3495a55..0a1bac3 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -56,11 +56,8 @@ int ovl_removexattr(struct dentry *dentry, const char *name);
 
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode,
 			    struct ovl_entry *oe);
-static inline void ovl_copyattr(struct inode *from, struct inode *to)
-{
-	to->i_uid = from->i_uid;
-	to->i_gid = from->i_gid;
-}
+
+void ovl_copyattr(struct inode *from, struct inode *to);
 
 /* dir.c */
 extern const struct inode_operations ovl_dir_inode_operations;
-- 
1.7.9.5