[PATCH 0/5][RFC] Overlayfs SELinux Support

[PATCH 0/5][RFC] Overlayfs SELinux Support

[Vivek Goyal]

Hi,

Following are RFC patches to support SELinux with overlayfs. I started
with David Howells's latest posting on this topic and started modifying
patches. These patches apply on top of overlayfs-next branch of miklos
vfs git tree.

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next

These patches can be pulled from my branch too.

https://github.com/rhvgoyal/linux/commits/overlayfs-selinux-mounter-next

Thanks to Dan Walsh, Stephen Smalley and Miklos Szeredi for numerous
conversation and ideas in helping figuring out what one reasonable
implementation might look like.

Dan Walsh has been writing tests for selinux overlayfs in selinux-testsuite.
These patches pass those tests except one. I think that test/policy need
to be fixed. 

https://github.com/rhatdan/selinux-testsuite/commits/master

Posting these patches for review and comments.

These patches introduce 3 new security hooks.

- security_inode_copy_up(), is called when a file is copied up. This hook
  prepares a new set of cred which is used for copy up operation. And
  new set of creds are prepared so that ->create_sid can be set appropriately
  and newly created file is labeled properly. 

  When a file is copied up, label of lower file is retained except for the
  case of context= mount where new file gets the label from context= option.

- security_inode_copy_up_xattr(), is called when xattrs of a file are
  being copied up. Before this we already called security_inode_copy_up()
  and created new file and copied up data. That means file already got
  labeled properly and there is no need to take SELINUX xattr of lower
  file and overwrite the upper file xattr. So this hook is used to avoid
  copying up of SELINUX xattr.

- dentry_create_files_as(), is called when a new file is about to be created.
  This hook determines what the label of the file should be if task had
  created that file in upper/ and sets create_sid accordingly in the passed
  in creds.

  Normal transition rules don't work for the case of context mounts as
  underlying file system is not aware of context option which only overlay
  layer is aware of. For non-context mounts, creation can happen in work/
  dir first and then file might be renamed into upper/, and it might get
  label based on work/ dir. So this hooks helps avoiding all these issues.

  When a new file is created in upper/, it gets its label based on transition
  rules. For the case of context mount, it gets the label from context=
  option.

Apart from hooks, also changed overlay code to not do getxattr checks on
underlying inode so that overlay inode selinux label does not fail
initializaiton.
 
Any feedback is welcome.

Thanks
Vivek

Vivek Goyal (5):
  security, overlayfs: provide copy up security hook for unioned files
  security,overlayfs: Provide security hook for copy up of xattrs for
    overlay file
  selinux: Pass security pointer to determine_inode_label()
  overlayfs: Correctly label newly created file over whiteout
  overlayfs: Use vfs_getxattr_noperm() for real inode

 fs/overlayfs/copy_up.c    | 16 ++++++++++
 fs/overlayfs/dir.c        | 10 ++++++
 fs/overlayfs/inode.c      |  7 +----
 fs/xattr.c                | 28 +++++++++++------
 include/linux/lsm_hooks.h | 41 ++++++++++++++++++++++++
 include/linux/security.h  | 28 +++++++++++++++++
 include/linux/xattr.h     |  1 +
 security/security.c       | 28 +++++++++++++++++
 security/selinux/hooks.c  | 80 ++++++++++++++++++++++++++++++++++++++++++-----
 9 files changed, 216 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Vivek Goyal]

Provide a security hook to label new file correctly when a file is copied
up from lower layer to upper layer of a overlay/union mount.

This hook can prepare and switch to a new set of creds which are suitable
for new file creation during copy up. Caller should revert to old creds
after file creation.

In SELinux, newly copied up file gets same label as lower file for
non-context mounts. But it gets label specified in mount option context=
for context mounts.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c    |  8 ++++++++
 include/linux/lsm_hooks.h | 13 +++++++++++++
 include/linux/security.h  |  6 ++++++
 security/security.c       |  8 ++++++++
 security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
 5 files changed, 62 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 80aa6f1..90dc362 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	struct dentry *upper = NULL;
 	umode_t mode = stat->mode;
 	int err;
+	const struct cred *old_creds = NULL;
 
 	newdentry = ovl_lookup_temp(workdir, dentry);
 	err = PTR_ERR(newdentry);
@@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	if (IS_ERR(upper))
 		goto out1;
 
+	err = security_inode_copy_up(dentry, &old_creds);
+	if (err < 0)
+		goto out2;
+
 	/* Can't properly set mode on creation because of the umask */
 	stat->mode &= S_IFMT;
 	err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
 	stat->mode = mode;
+	if (old_creds)
+		revert_creds(old_creds);
+
 	if (err)
 		goto out2;
 
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 7ae3976..fcde9b9 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -401,6 +401,17 @@
  *	@inode contains a pointer to the inode.
  *	@secid contains a pointer to the location where result will be saved.
  *	In case of failure, @secid will be set to zero.
+ * @inode_copy_up:
+ *	A file is about to be copied up from lower layer to upper layer of
+ *	overlay filesystem. Prepare a new set of creds and set file creation
+ *	secid in such a way so that copied up file gets the appropriate
+ *	label. Switch to this newly prepared creds and return old creds. This
+ *	returns with only one reference to newly prepared creds. So as soon
+ *	as caller calls revert_cred(old_creds), creds allocated by this hook
+ *	should be freed.
+ *	@src indicates the union dentry of file that is being copied up.
+ *	@old indicates the pointer to old_cred returned to caller.
+ *	Returns 0 on success or a negative error code on error.
  *
  * Security hooks for file operations
  *
@@ -1425,6 +1436,7 @@ union security_list_options {
 	int (*inode_listsecurity)(struct inode *inode, char *buffer,
 					size_t buffer_size);
 	void (*inode_getsecid)(struct inode *inode, u32 *secid);
+	int (*inode_copy_up) (struct dentry *src, const struct cred **old);
 
 	int (*file_permission)(struct file *file, int mask);
 	int (*file_alloc_security)(struct file *file);
@@ -1696,6 +1708,7 @@ struct security_hook_heads {
 	struct list_head inode_setsecurity;
 	struct list_head inode_listsecurity;
 	struct list_head inode_getsecid;
+	struct list_head inode_copy_up;
 	struct list_head file_permission;
 	struct list_head file_alloc_security;
 	struct list_head file_free_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 14df373..3445df2 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -282,6 +282,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
+int security_inode_copy_up(struct dentry *src, const struct cred **old);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -758,6 +759,11 @@ static inline void security_inode_getsecid(struct inode *inode, u32 *secid)
 	*secid = 0;
 }
 
+static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+	return 0;
+}
+
 static inline int security_file_permission(struct file *file, int mask)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 7095693..7c1ce29 100644
--- a/security/security.c
+++ b/security/security.c
@@ -727,6 +727,12 @@ void security_inode_getsecid(struct inode *inode, u32 *secid)
 	call_void_hook(inode_getsecid, inode, secid);
 }
 
+int security_inode_copy_up(struct dentry *src, const struct cred **old)
+{
+	return call_int_hook(inode_copy_up, 0, src, old);
+}
+EXPORT_SYMBOL(security_inode_copy_up);
+
 int security_file_permission(struct file *file, int mask)
 {
 	int ret;
@@ -1663,6 +1669,8 @@ struct security_hook_heads security_hook_heads = {
 		LIST_HEAD_INIT(security_hook_heads.inode_listsecurity),
 	.inode_getsecid =
 		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
+	.inode_copy_up =
+		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
 	.file_permission =
 		LIST_HEAD_INIT(security_hook_heads.file_permission),
 	.file_alloc_security =
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a86d537..1b1a1e5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
 	*secid = isec->sid;
 }
 
+static int selinux_inode_copy_up(struct dentry *src, const struct cred **old)
+{
+	u32 sid;
+	struct cred *new_creds;
+	struct task_security_struct *tsec;
+
+	new_creds = prepare_creds();
+	if (!new_creds)
+		return -ENOMEM;
+
+	/* Get label from overlay inode and set it in create_sid */
+	selinux_inode_getsecid(d_inode(src), &sid);
+	tsec = new_creds->security;
+	tsec->create_sid = sid;
+	*old = override_creds(new_creds);
+
+	/*
+	 * At this point of time we have 2 refs on new_creds. One by
+	 * prepare_creds and other by override_creds. Drop one reference
+	 * so that as soon as caller calls revert_creds(old), this cred
+	 * will be freed.
+	 */
+	put_cred(new_creds);
+	return 0;
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -6056,6 +6082,7 @@ static struct security_hook_list selinux_hooks[] = {
 	LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity),
 	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
 	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
+	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
 
 	LSM_HOOK_INIT(file_permission, selinux_file_permission),
 	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
-- 
2.7.4

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

Hi,

[auto build test ERROR on miklos-vfs/overlayfs-next]
[also build test ERROR on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vivek-Goyal/Overlayfs-SELinux-Support/20160706-000037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: i386-randconfig-s0-201627 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   fs/overlayfs/copy_up.c: In function 'ovl_copy_up_locked':
>> fs/overlayfs/copy_up.c:262:39: error: passing argument 2 of 'security_inode_copy_up' from incompatible pointer type [-Werror=incompatible-pointer-types]
     err = security_inode_copy_up(dentry, &old_creds);
                                          ^
   In file included from fs/overlayfs/copy_up.c:16:0:
   include/linux/security.h:762:19: note: expected 'struct dentry *' but argument is of type 'const struct cred **'
    static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
                      ^~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/security_inode_copy_up +262 fs/overlayfs/copy_up.c

   256		upper = lookup_one_len(dentry->d_name.name, upperdir,
   257				       dentry->d_name.len);
   258		err = PTR_ERR(upper);
   259		if (IS_ERR(upper))
   260			goto out1;
   261	
 > 262		err = security_inode_copy_up(dentry, &old_creds);
   263		if (err < 0)
   264			goto out2;
   265	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24417 bytes --]

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

Hi,

[auto build test ERROR on miklos-vfs/overlayfs-next]
[also build test ERROR on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vivek-Goyal/Overlayfs-SELinux-Support/20160706-000037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: i386-randconfig-s0-201627 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   fs/overlayfs/copy_up.c: In function 'ovl_copy_up_locked':
>> fs/overlayfs/copy_up.c:262:39: error: passing argument 2 of 'security_inode_copy_up' from incompatible pointer type [-Werror=incompatible-pointer-types]
     err = security_inode_copy_up(dentry, &old_creds);
                                          ^
   In file included from fs/overlayfs/copy_up.c:16:0:
   include/linux/security.h:762:19: note: expected 'struct dentry *' but argument is of type 'const struct cred **'
    static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
                      ^~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/security_inode_copy_up +262 fs/overlayfs/copy_up.c

   256		upper = lookup_one_len(dentry->d_name.name, upperdir,
   257				       dentry->d_name.len);
   258		err = PTR_ERR(upper);
   259		if (IS_ERR(upper))
   260			goto out1;
   261	
 > 262		err = security_inode_copy_up(dentry, &old_creds);
   263		if (err < 0)
   264			goto out2;
   265	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24417 bytes --]

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Vivek Goyal]

On Wed, Jul 06, 2016 at 12:53:57AM +0800, kbuild test robot wrote:
> Hi,
> 
> [auto build test ERROR on miklos-vfs/overlayfs-next]
> [also build test ERROR on v4.7-rc6 next-20160705]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Vivek-Goyal/Overlayfs-SELinux-Support/20160706-000037
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
> config: i386-randconfig-s0-201627 (attached as .config)
> compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>    fs/overlayfs/copy_up.c: In function 'ovl_copy_up_locked':
> >> fs/overlayfs/copy_up.c:262:39: error: passing argument 2 of 'security_inode_copy_up' from incompatible pointer type [-Werror=incompatible-pointer-types]
>      err = security_inode_copy_up(dentry, &old_creds);
>                                           ^
>    In file included from fs/overlayfs/copy_up.c:16:0:
>    include/linux/security.h:762:19: note: expected 'struct dentry *' but argument is of type 'const struct cred **'
>     static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
>                       ^~~~~~~~~~~~~~~~~~~~~~
>    cc1: some warnings being treated as errors
> 
> vim +/security_inode_copy_up +262 fs/overlayfs/copy_up.c
> 
>    256		upper = lookup_one_len(dentry->d_name.name, upperdir,
>    257				       dentry->d_name.len);
>    258		err = PTR_ERR(upper);
>    259		if (IS_ERR(upper))
>    260			goto out1;
>    261	
>  > 262		err = security_inode_copy_up(dentry, &old_creds);
>    263		if (err < 0)
>    264			goto out2;
>    265	
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Oops, wrong function signatures for CONFIG_SECURITY=n. Following is the
new patch attached.

Vivek


Subject: security, overlayfs: provide copy up security hook for unioned files

Provide a security hook to label new file correctly when a file is copied
up from lower layer to upper layer of a overlay/union mount.

This hook can prepare and switch to a new set of creds which are suitable
for new file creation during copy up. Caller should revert to old creds
after file creation.

In SELinux, newly copied up file gets same label as lower file for
non-context mounts. But it gets label specified in mount option context=
for context mounts.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---

 fs/overlayfs/copy_up.c    |    8 ++++++++
 include/linux/lsm_hooks.h |   13 +++++++++++++
 include/linux/security.h  |    7 +++++++
 security/security.c       |    8 ++++++++
 security/selinux/hooks.c  |   27 +++++++++++++++++++++++++++
 5 files changed, 63 insertions(+)

Index: rhvgoyal-linux/include/linux/lsm_hooks.h
===================================================================
--- rhvgoyal-linux.orig/include/linux/lsm_hooks.h	2016-07-05 13:31:45.988514243 -0400
+++ rhvgoyal-linux/include/linux/lsm_hooks.h	2016-07-05 13:31:47.917514243 -0400
@@ -401,6 +401,17 @@
  *	@inode contains a pointer to the inode.
  *	@secid contains a pointer to the location where result will be saved.
  *	In case of failure, @secid will be set to zero.
+ * @inode_copy_up:
+ *	A file is about to be copied up from lower layer to upper layer of
+ *	overlay filesystem. Prepare a new set of creds and set file creation
+ *	secid in such a way so that copied up file gets the appropriate
+ *	label. Switch to this newly prepared creds and return old creds. This
+ *	returns with only one reference to newly prepared creds. So as soon
+ *	as caller calls revert_cred(old_creds), creds allocated by this hook
+ *	should be freed.
+ *	@src indicates the union dentry of file that is being copied up.
+ *	@old indicates the pointer to old_cred returned to caller.
+ *	Returns 0 on success or a negative error code on error.
  *
  * Security hooks for file operations
  *
@@ -1425,6 +1436,7 @@ union security_list_options {
 	int (*inode_listsecurity)(struct inode *inode, char *buffer,
 					size_t buffer_size);
 	void (*inode_getsecid)(struct inode *inode, u32 *secid);
+	int (*inode_copy_up) (struct dentry *src, const struct cred **old);
 
 	int (*file_permission)(struct file *file, int mask);
 	int (*file_alloc_security)(struct file *file);
@@ -1696,6 +1708,7 @@ struct security_hook_heads {
 	struct list_head inode_setsecurity;
 	struct list_head inode_listsecurity;
 	struct list_head inode_getsecid;
+	struct list_head inode_copy_up;
 	struct list_head file_permission;
 	struct list_head file_alloc_security;
 	struct list_head file_free_security;
Index: rhvgoyal-linux/include/linux/security.h
===================================================================
--- rhvgoyal-linux.orig/include/linux/security.h	2016-07-05 13:31:45.988514243 -0400
+++ rhvgoyal-linux/include/linux/security.h	2016-07-05 13:32:45.954514243 -0400
@@ -282,6 +282,7 @@ int security_inode_getsecurity(struct in
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
+int security_inode_copy_up(struct dentry *src, const struct cred **old);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -758,6 +759,12 @@ static inline void security_inode_getsec
 	*secid = 0;
 }
 
+static inline int security_inode_copy_up(struct dentry *src,
+					 const struct cred **old)
+{
+	return 0;
+}
+
 static inline int security_file_permission(struct file *file, int mask)
 {
 	return 0;
Index: rhvgoyal-linux/security/security.c
===================================================================
--- rhvgoyal-linux.orig/security/security.c	2016-07-05 13:31:45.990514243 -0400
+++ rhvgoyal-linux/security/security.c	2016-07-05 13:31:47.920514243 -0400
@@ -727,6 +727,12 @@ void security_inode_getsecid(struct inod
 	call_void_hook(inode_getsecid, inode, secid);
 }
 
+int security_inode_copy_up(struct dentry *src, const struct cred **old)
+{
+	return call_int_hook(inode_copy_up, 0, src, old);
+}
+EXPORT_SYMBOL(security_inode_copy_up);
+
 int security_file_permission(struct file *file, int mask)
 {
 	int ret;
@@ -1663,6 +1669,8 @@ struct security_hook_heads security_hook
 		LIST_HEAD_INIT(security_hook_heads.inode_listsecurity),
 	.inode_getsecid =
 		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
+	.inode_copy_up =
+		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
 	.file_permission =
 		LIST_HEAD_INIT(security_hook_heads.file_permission),
 	.file_alloc_security =
Index: rhvgoyal-linux/fs/overlayfs/copy_up.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/copy_up.c	2016-07-05 13:31:45.985514243 -0400
+++ rhvgoyal-linux/fs/overlayfs/copy_up.c	2016-07-05 13:31:47.921514243 -0400
@@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct den
 	struct dentry *upper = NULL;
 	umode_t mode = stat->mode;
 	int err;
+	const struct cred *old_creds = NULL;
 
 	newdentry = ovl_lookup_temp(workdir, dentry);
 	err = PTR_ERR(newdentry);
@@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct den
 	if (IS_ERR(upper))
 		goto out1;
 
+	err = security_inode_copy_up(dentry, &old_creds);
+	if (err < 0)
+		goto out2;
+
 	/* Can't properly set mode on creation because of the umask */
 	stat->mode &= S_IFMT;
 	err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
 	stat->mode = mode;
+	if (old_creds)
+		revert_creds(old_creds);
+
 	if (err)
 		goto out2;
 
Index: rhvgoyal-linux/security/selinux/hooks.c
===================================================================
--- rhvgoyal-linux.orig/security/selinux/hooks.c	2016-07-05 13:31:45.992514243 -0400
+++ rhvgoyal-linux/security/selinux/hooks.c	2016-07-05 13:31:47.923514243 -0400
@@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struc
 	*secid = isec->sid;
 }
 
+static int selinux_inode_copy_up(struct dentry *src, const struct cred **old)
+{
+	u32 sid;
+	struct cred *new_creds;
+	struct task_security_struct *tsec;
+
+	new_creds = prepare_creds();
+	if (!new_creds)
+		return -ENOMEM;
+
+	/* Get label from overlay inode and set it in create_sid */
+	selinux_inode_getsecid(d_inode(src), &sid);
+	tsec = new_creds->security;
+	tsec->create_sid = sid;
+	*old = override_creds(new_creds);
+
+	/*
+	 * At this point of time we have 2 refs on new_creds. One by
+	 * prepare_creds and other by override_creds. Drop one reference
+	 * so that as soon as caller calls revert_creds(old), this cred
+	 * will be freed.
+	 */
+	put_cred(new_creds);
+	return 0;
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -6056,6 +6082,7 @@ static struct security_hook_list selinux
 	LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity),
 	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
 	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
+	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
 
 	LSM_HOOK_INIT(file_permission, selinux_file_permission),
 	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[-- Attachment #1: Type: text/plain, Size: 2332 bytes --]

Hi,

[auto build test WARNING on miklos-vfs/overlayfs-next]
[also build test WARNING on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vivek-Goyal/Overlayfs-SELinux-Support/20160706-000037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: m68k-sun3_defconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All warnings (new ones prefixed by >>):

   fs/overlayfs/copy_up.c: In function 'ovl_copy_up_locked':
>> fs/overlayfs/copy_up.c:262:8: warning: passing argument 2 of 'security_inode_copy_up' from incompatible pointer type
     err = security_inode_copy_up(dentry, &old_creds);
           ^
   In file included from fs/overlayfs/copy_up.c:16:0:
   include/linux/security.h:762:19: note: expected 'struct dentry *' but argument is of type 'const struct cred **'
    static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
                      ^

vim +/security_inode_copy_up +262 fs/overlayfs/copy_up.c

   246		struct dentry *upper = NULL;
   247		umode_t mode = stat->mode;
   248		int err;
   249		const struct cred *old_creds = NULL;
   250	
   251		newdentry = ovl_lookup_temp(workdir, dentry);
   252		err = PTR_ERR(newdentry);
   253		if (IS_ERR(newdentry))
   254			goto out;
   255	
   256		upper = lookup_one_len(dentry->d_name.name, upperdir,
   257				       dentry->d_name.len);
   258		err = PTR_ERR(upper);
   259		if (IS_ERR(upper))
   260			goto out1;
   261	
 > 262		err = security_inode_copy_up(dentry, &old_creds);
   263		if (err < 0)
   264			goto out2;
   265	
   266		/* Can't properly set mode on creation because of the umask */
   267		stat->mode &= S_IFMT;
   268		err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
   269		stat->mode = mode;
   270		if (old_creds)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 11731 bytes --]

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2332 bytes --]

Hi,

[auto build test WARNING on miklos-vfs/overlayfs-next]
[also build test WARNING on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vivek-Goyal/Overlayfs-SELinux-Support/20160706-000037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: m68k-sun3_defconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All warnings (new ones prefixed by >>):

   fs/overlayfs/copy_up.c: In function 'ovl_copy_up_locked':
>> fs/overlayfs/copy_up.c:262:8: warning: passing argument 2 of 'security_inode_copy_up' from incompatible pointer type
     err = security_inode_copy_up(dentry, &old_creds);
           ^
   In file included from fs/overlayfs/copy_up.c:16:0:
   include/linux/security.h:762:19: note: expected 'struct dentry *' but argument is of type 'const struct cred **'
    static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
                      ^

vim +/security_inode_copy_up +262 fs/overlayfs/copy_up.c

   246		struct dentry *upper = NULL;
   247		umode_t mode = stat->mode;
   248		int err;
   249		const struct cred *old_creds = NULL;
   250	
   251		newdentry = ovl_lookup_temp(workdir, dentry);
   252		err = PTR_ERR(newdentry);
   253		if (IS_ERR(newdentry))
   254			goto out;
   255	
   256		upper = lookup_one_len(dentry->d_name.name, upperdir,
   257				       dentry->d_name.len);
   258		err = PTR_ERR(upper);
   259		if (IS_ERR(upper))
   260			goto out1;
   261	
 > 262		err = security_inode_copy_up(dentry, &old_creds);
   263		if (err < 0)
   264			goto out2;
   265	
   266		/* Can't properly set mode on creation because of the umask */
   267		stat->mode &= S_IFMT;
   268		err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
   269		stat->mode = mode;
   270		if (old_creds)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 11731 bytes --]

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Casey Schaufler]

On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> Provide a security hook to label new file correctly when a file is copied
> up from lower layer to upper layer of a overlay/union mount.
>
> This hook can prepare and switch to a new set of creds which are suitable
> for new file creation during copy up. Caller should revert to old creds
> after file creation.
>
> In SELinux, newly copied up file gets same label as lower file for
> non-context mounts. But it gets label specified in mount option context=
> for context mounts.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c    |  8 ++++++++
>  include/linux/lsm_hooks.h | 13 +++++++++++++
>  include/linux/security.h  |  6 ++++++
>  security/security.c       |  8 ++++++++
>  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 80aa6f1..90dc362 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>  	struct dentry *upper = NULL;
>  	umode_t mode = stat->mode;
>  	int err;
> +	const struct cred *old_creds = NULL;
>  
>  	newdentry = ovl_lookup_temp(workdir, dentry);
>  	err = PTR_ERR(newdentry);
> @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>  	if (IS_ERR(upper))
>  		goto out1;
>  
> +	err = security_inode_copy_up(dentry, &old_creds);
> +	if (err < 0)
> +		goto out2;
> +
>  	/* Can't properly set mode on creation because of the umask */
>  	stat->mode &= S_IFMT;
>  	err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
>  	stat->mode = mode;
> +	if (old_creds)
> +		revert_creds(old_creds);
> +
>  	if (err)
>  		goto out2;

I don't much care for the way part of the credential manipulation
is done in the caller and part is done the the security module.
If the caller is going to restore the old state, the caller should
save the old state. 

>  
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7ae3976..fcde9b9 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -401,6 +401,17 @@
>   *	@inode contains a pointer to the inode.
>   *	@secid contains a pointer to the location where result will be saved.
>   *	In case of failure, @secid will be set to zero.
> + * @inode_copy_up:
> + *	A file is about to be copied up from lower layer to upper layer of
> + *	overlay filesystem. Prepare a new set of creds and set file creation
> + *	secid in such a way so that copied up file gets the appropriate

The details of what goes on the the SELinux case don't belong here.

> + *	label. Switch to this newly prepared creds and return old creds. This
> + *	returns with only one reference to newly prepared creds. So as soon
> + *	as caller calls revert_cred(old_creds), creds allocated by this hook
> + *	should be freed.
> + *	@src indicates the union dentry of file that is being copied up.
> + *	@old indicates the pointer to old_cred returned to caller.
> + *	Returns 0 on success or a negative error code on error.
>   *
>   * Security hooks for file operations
>   *
> @@ -1425,6 +1436,7 @@ union security_list_options {
>  	int (*inode_listsecurity)(struct inode *inode, char *buffer,
>  					size_t buffer_size);
>  	void (*inode_getsecid)(struct inode *inode, u32 *secid);
> +	int (*inode_copy_up) (struct dentry *src, const struct cred **old);
>  
>  	int (*file_permission)(struct file *file, int mask);
>  	int (*file_alloc_security)(struct file *file);
> @@ -1696,6 +1708,7 @@ struct security_hook_heads {
>  	struct list_head inode_setsecurity;
>  	struct list_head inode_listsecurity;
>  	struct list_head inode_getsecid;
> +	struct list_head inode_copy_up;
>  	struct list_head file_permission;
>  	struct list_head file_alloc_security;
>  	struct list_head file_free_security;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 14df373..3445df2 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -282,6 +282,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
>  int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
>  int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
>  void security_inode_getsecid(struct inode *inode, u32 *secid);
> +int security_inode_copy_up(struct dentry *src, const struct cred **old);
>  int security_file_permission(struct file *file, int mask);
>  int security_file_alloc(struct file *file);
>  void security_file_free(struct file *file);
> @@ -758,6 +759,11 @@ static inline void security_inode_getsecid(struct inode *inode, u32 *secid)
>  	*secid = 0;
>  }
>  
> +static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
> +{
> +	return 0;
> +}
> +
>  static inline int security_file_permission(struct file *file, int mask)
>  {
>  	return 0;
> diff --git a/security/security.c b/security/security.c
> index 7095693..7c1ce29 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -727,6 +727,12 @@ void security_inode_getsecid(struct inode *inode, u32 *secid)
>  	call_void_hook(inode_getsecid, inode, secid);
>  }
>  
> +int security_inode_copy_up(struct dentry *src, const struct cred **old)
> +{
> +	return call_int_hook(inode_copy_up, 0, src, old);
> +}
> +EXPORT_SYMBOL(security_inode_copy_up);
> +
>  int security_file_permission(struct file *file, int mask)
>  {
>  	int ret;
> @@ -1663,6 +1669,8 @@ struct security_hook_heads security_hook_heads = {
>  		LIST_HEAD_INIT(security_hook_heads.inode_listsecurity),
>  	.inode_getsecid =
>  		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
> +	.inode_copy_up =
> +		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
>  	.file_permission =
>  		LIST_HEAD_INIT(security_hook_heads.file_permission),
>  	.file_alloc_security =
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a86d537..1b1a1e5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
>  	*secid = isec->sid;
>  }
>  
> +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old)
> +{
> +	u32 sid;
> +	struct cred *new_creds;
> +	struct task_security_struct *tsec;
> +
> +	new_creds = prepare_creds();
> +	if (!new_creds)
> +		return -ENOMEM;
> +
> +	/* Get label from overlay inode and set it in create_sid */
> +	selinux_inode_getsecid(d_inode(src), &sid);
> +	tsec = new_creds->security;
> +	tsec->create_sid = sid;
> +	*old = override_creds(new_creds);
> +
> +	/*
> +	 * At this point of time we have 2 refs on new_creds. One by
> +	 * prepare_creds and other by override_creds. Drop one reference
> +	 * so that as soon as caller calls revert_creds(old), this cred
> +	 * will be freed.
> +	 */
> +	put_cred(new_creds);
> +	return 0;
> +}
> +
>  /* file security operations */
>  
>  static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -6056,6 +6082,7 @@ static struct security_hook_list selinux_hooks[] = {
>  	LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity),
>  	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
>  	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
> +	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
>  
>  	LSM_HOOK_INIT(file_permission, selinux_file_permission),
>  	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Vivek Goyal]

On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote:
> 
> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> > Provide a security hook to label new file correctly when a file is copied
> > up from lower layer to upper layer of a overlay/union mount.
> >
> > This hook can prepare and switch to a new set of creds which are suitable
> > for new file creation during copy up. Caller should revert to old creds
> > after file creation.
> >
> > In SELinux, newly copied up file gets same label as lower file for
> > non-context mounts. But it gets label specified in mount option context=
> > for context mounts.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c    |  8 ++++++++
> >  include/linux/lsm_hooks.h | 13 +++++++++++++
> >  include/linux/security.h  |  6 ++++++
> >  security/security.c       |  8 ++++++++
> >  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
> >  5 files changed, 62 insertions(+)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 80aa6f1..90dc362 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> >  	struct dentry *upper = NULL;
> >  	umode_t mode = stat->mode;
> >  	int err;
> > +	const struct cred *old_creds = NULL;
> >  
> >  	newdentry = ovl_lookup_temp(workdir, dentry);
> >  	err = PTR_ERR(newdentry);
> > @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> >  	if (IS_ERR(upper))
> >  		goto out1;
> >  
> > +	err = security_inode_copy_up(dentry, &old_creds);
> > +	if (err < 0)
> > +		goto out2;
> > +
> >  	/* Can't properly set mode on creation because of the umask */
> >  	stat->mode &= S_IFMT;
> >  	err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
> >  	stat->mode = mode;
> > +	if (old_creds)
> > +		revert_creds(old_creds);
> > +
> >  	if (err)
> >  		goto out2;
> 
> I don't much care for the way part of the credential manipulation
> is done in the caller and part is done the the security module.
> If the caller is going to restore the old state, the caller should
> save the old state. 

Ok, I am fine either way. Smalley had preferred switching creds in 
security module, that's why I did it this way. I will change back
to allocating and overriding creds in caller.

> 
> >  
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 7ae3976..fcde9b9 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -401,6 +401,17 @@
> >   *	@inode contains a pointer to the inode.
> >   *	@secid contains a pointer to the location where result will be saved.
> >   *	In case of failure, @secid will be set to zero.
> > + * @inode_copy_up:
> > + *	A file is about to be copied up from lower layer to upper layer of
> > + *	overlay filesystem. Prepare a new set of creds and set file creation
> > + *	secid in such a way so that copied up file gets the appropriate
> 
> The details of what goes on the the SELinux case don't belong here.

Ok, will remove selinux specific details from here.

Thanks
Vivek

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Vivek Goyal]

On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote:
> 
> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> > Provide a security hook to label new file correctly when a file is copied
> > up from lower layer to upper layer of a overlay/union mount.
> >
> > This hook can prepare and switch to a new set of creds which are suitable
> > for new file creation during copy up. Caller should revert to old creds
> > after file creation.
> >
> > In SELinux, newly copied up file gets same label as lower file for
> > non-context mounts. But it gets label specified in mount option context=
> > for context mounts.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c    |  8 ++++++++
> >  include/linux/lsm_hooks.h | 13 +++++++++++++
> >  include/linux/security.h  |  6 ++++++
> >  security/security.c       |  8 ++++++++
> >  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
> >  5 files changed, 62 insertions(+)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 80aa6f1..90dc362 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> >  	struct dentry *upper = NULL;
> >  	umode_t mode = stat->mode;
> >  	int err;
> > +	const struct cred *old_creds = NULL;
> >  
> >  	newdentry = ovl_lookup_temp(workdir, dentry);
> >  	err = PTR_ERR(newdentry);
> > @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> >  	if (IS_ERR(upper))
> >  		goto out1;
> >  
> > +	err = security_inode_copy_up(dentry, &old_creds);
> > +	if (err < 0)
> > +		goto out2;
> > +
> >  	/* Can't properly set mode on creation because of the umask */
> >  	stat->mode &= S_IFMT;
> >  	err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
> >  	stat->mode = mode;
> > +	if (old_creds)
> > +		revert_creds(old_creds);
> > +
> >  	if (err)
> >  		goto out2;
> 
> I don't much care for the way part of the credential manipulation
> is done in the caller and part is done the the security module.
> If the caller is going to restore the old state, the caller should
> save the old state. 

One advantage of current patches is that we switch to new creds only if
it is needed. For example, if there are no LSMs loaded, then there is
no need to modify creds and make a switch to new creds.

But if I start allocating new creds and save old state in caller, then
caller always has to do it (irrespective of the fact whether any LSM
modified the creds or not).

Thanks
Vivek

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Casey Schaufler]

On 7/7/2016 1:33 PM, Vivek Goyal wrote:
> On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote:
>> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
>>> Provide a security hook to label new file correctly when a file is copied
>>> up from lower layer to upper layer of a overlay/union mount.
>>>
>>> This hook can prepare and switch to a new set of creds which are suitable
>>> for new file creation during copy up. Caller should revert to old creds
>>> after file creation.
>>>
>>> In SELinux, newly copied up file gets same label as lower file for
>>> non-context mounts. But it gets label specified in mount option context=
>>> for context mounts.
>>>
>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>> ---
>>>  fs/overlayfs/copy_up.c    |  8 ++++++++
>>>  include/linux/lsm_hooks.h | 13 +++++++++++++
>>>  include/linux/security.h  |  6 ++++++
>>>  security/security.c       |  8 ++++++++
>>>  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
>>>  5 files changed, 62 insertions(+)
>>>
>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>>> index 80aa6f1..90dc362 100644
>>> --- a/fs/overlayfs/copy_up.c
>>> +++ b/fs/overlayfs/copy_up.c
>>> @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>>>  	struct dentry *upper = NULL;
>>>  	umode_t mode = stat->mode;
>>>  	int err;
>>> +	const struct cred *old_creds = NULL;
>>>  
>>>  	newdentry = ovl_lookup_temp(workdir, dentry);
>>>  	err = PTR_ERR(newdentry);
>>> @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>>>  	if (IS_ERR(upper))
>>>  		goto out1;
>>>  
>>> +	err = security_inode_copy_up(dentry, &old_creds);
>>> +	if (err < 0)
>>> +		goto out2;
>>> +
>>>  	/* Can't properly set mode on creation because of the umask */
>>>  	stat->mode &= S_IFMT;
>>>  	err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
>>>  	stat->mode = mode;
>>> +	if (old_creds)
>>> +		revert_creds(old_creds);
>>> +
>>>  	if (err)
>>>  		goto out2;
>> I don't much care for the way part of the credential manipulation
>> is done in the caller and part is done the the security module.
>> If the caller is going to restore the old state, the caller should
>> save the old state. 
> One advantage of current patches is that we switch to new creds only if
> it is needed. For example, if there are no LSMs loaded,

Point.

>  then there is
> no need to modify creds and make a switch to new creds.

I'm not a fan of cred flipping. There are too many ways for it to go
wrong. Consider interrupts. I assume you've ruled that out as a possibility
in the caller, but I still think the practice is dangerous.

I greatly prefer "create and set attributes" to "change cred, create and
reset cred". I know that has it's own set of problems, including races
and faking privilege. 
 

> But if I start allocating new creds and save old state in caller, then
> caller always has to do it (irrespective of the fact whether any LSM
> modified the creds or not).

It starts getting messy when I have two modules that want to
change change the credential. Each module will have to check to
see if a module called before it has allocated a new cred.

>
> Thanks
> Vivek
>

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Miklos Szeredi]

On Thu, Jul 7, 2016 at 11:44 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/7/2016 1:33 PM, Vivek Goyal wrote:
>> On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote:
>>> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
>>>> Provide a security hook to label new file correctly when a file is copied
>>>> up from lower layer to upper layer of a overlay/union mount.
>>>>
>>>> This hook can prepare and switch to a new set of creds which are suitable
>>>> for new file creation during copy up. Caller should revert to old creds
>>>> after file creation.
>>>>
>>>> In SELinux, newly copied up file gets same label as lower file for
>>>> non-context mounts. But it gets label specified in mount option context=
>>>> for context mounts.
>>>>
>>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>>> ---
>>>>  fs/overlayfs/copy_up.c    |  8 ++++++++
>>>>  include/linux/lsm_hooks.h | 13 +++++++++++++
>>>>  include/linux/security.h  |  6 ++++++
>>>>  security/security.c       |  8 ++++++++
>>>>  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
>>>>  5 files changed, 62 insertions(+)
>>>>
>>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>>>> index 80aa6f1..90dc362 100644
>>>> --- a/fs/overlayfs/copy_up.c
>>>> +++ b/fs/overlayfs/copy_up.c
>>>> @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>>>>     struct dentry *upper = NULL;
>>>>     umode_t mode = stat->mode;
>>>>     int err;
>>>> +   const struct cred *old_creds = NULL;
>>>>
>>>>     newdentry = ovl_lookup_temp(workdir, dentry);
>>>>     err = PTR_ERR(newdentry);
>>>> @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>>>>     if (IS_ERR(upper))
>>>>             goto out1;
>>>>
>>>> +   err = security_inode_copy_up(dentry, &old_creds);
>>>> +   if (err < 0)
>>>> +           goto out2;
>>>> +
>>>>     /* Can't properly set mode on creation because of the umask */
>>>>     stat->mode &= S_IFMT;
>>>>     err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
>>>>     stat->mode = mode;
>>>> +   if (old_creds)
>>>> +           revert_creds(old_creds);
>>>> +
>>>>     if (err)
>>>>             goto out2;
>>> I don't much care for the way part of the credential manipulation
>>> is done in the caller and part is done the the security module.
>>> If the caller is going to restore the old state, the caller should
>>> save the old state.

Conversely if the SM is setting the state it should restore it.
This needs yet another hook, but that's fine, I think.

>> One advantage of current patches is that we switch to new creds only if
>> it is needed. For example, if there are no LSMs loaded,
>
> Point.
>
>>  then there is
>> no need to modify creds and make a switch to new creds.
>
> I'm not a fan of cred flipping. There are too many ways for it to go
> wrong. Consider interrupts. I assume you've ruled that out as a possibility
> in the caller, but I still think the practice is dangerous.
>
> I greatly prefer "create and set attributes" to "change cred, create and
> reset cred". I know that has it's own set of problems, including races
> and faking privilege.

Yeah, we've talked about this. The races can be eliminated by always
doing the create in a the temporary "workdir" area and atomically
renaming to the final destination after everything has been set up.
OTOH that has a performance impact that the cred flipping eliminates.

>> But if I start allocating new creds and save old state in caller, then
>> caller always has to do it (irrespective of the fact whether any LSM
>> modified the creds or not).
>
> It starts getting messy when I have two modules that want to
> change change the credential. Each module will have to check to
> see if a module called before it has allocated a new cred.

Doesn't seem to me too difficult: check if *credp == NULL and allocate
if so.  Can even invent a heper for this if needed.

Thanks,
Miklos

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Vivek Goyal]

On Fri, Jul 08, 2016 at 09:21:13AM +0200, Miklos Szeredi wrote:
> On Thu, Jul 7, 2016 at 11:44 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 7/7/2016 1:33 PM, Vivek Goyal wrote:
> >> On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote:
> >>> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> >>>> Provide a security hook to label new file correctly when a file is copied
> >>>> up from lower layer to upper layer of a overlay/union mount.
> >>>>
> >>>> This hook can prepare and switch to a new set of creds which are suitable
> >>>> for new file creation during copy up. Caller should revert to old creds
> >>>> after file creation.
> >>>>
> >>>> In SELinux, newly copied up file gets same label as lower file for
> >>>> non-context mounts. But it gets label specified in mount option context=
> >>>> for context mounts.
> >>>>
> >>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >>>> ---
> >>>>  fs/overlayfs/copy_up.c    |  8 ++++++++
> >>>>  include/linux/lsm_hooks.h | 13 +++++++++++++
> >>>>  include/linux/security.h  |  6 ++++++
> >>>>  security/security.c       |  8 ++++++++
> >>>>  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
> >>>>  5 files changed, 62 insertions(+)
> >>>>
> >>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> >>>> index 80aa6f1..90dc362 100644
> >>>> --- a/fs/overlayfs/copy_up.c
> >>>> +++ b/fs/overlayfs/copy_up.c
> >>>> @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> >>>>     struct dentry *upper = NULL;
> >>>>     umode_t mode = stat->mode;
> >>>>     int err;
> >>>> +   const struct cred *old_creds = NULL;
> >>>>
> >>>>     newdentry = ovl_lookup_temp(workdir, dentry);
> >>>>     err = PTR_ERR(newdentry);
> >>>> @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
> >>>>     if (IS_ERR(upper))
> >>>>             goto out1;
> >>>>
> >>>> +   err = security_inode_copy_up(dentry, &old_creds);
> >>>> +   if (err < 0)
> >>>> +           goto out2;
> >>>> +
> >>>>     /* Can't properly set mode on creation because of the umask */
> >>>>     stat->mode &= S_IFMT;
> >>>>     err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
> >>>>     stat->mode = mode;
> >>>> +   if (old_creds)
> >>>> +           revert_creds(old_creds);
> >>>> +
> >>>>     if (err)
> >>>>             goto out2;
> >>> I don't much care for the way part of the credential manipulation
> >>> is done in the caller and part is done the the security module.
> >>> If the caller is going to restore the old state, the caller should
> >>> save the old state.
> 
> Conversely if the SM is setting the state it should restore it.
> This needs yet another hook, but that's fine, I think.
> 
> >> One advantage of current patches is that we switch to new creds only if
> >> it is needed. For example, if there are no LSMs loaded,
> >
> > Point.
> >
> >>  then there is
> >> no need to modify creds and make a switch to new creds.
> >
> > I'm not a fan of cred flipping. There are too many ways for it to go
> > wrong. Consider interrupts. I assume you've ruled that out as a possibility
> > in the caller, but I still think the practice is dangerous.
> >
> > I greatly prefer "create and set attributes" to "change cred, create and
> > reset cred". I know that has it's own set of problems, including races
> > and faking privilege.
> 
> Yeah, we've talked about this. The races can be eliminated by always
> doing the create in a the temporary "workdir" area and atomically
> renaming to the final destination after everything has been set up.
> OTOH that has a performance impact that the cred flipping eliminates.
> 
> >> But if I start allocating new creds and save old state in caller, then
> >> caller always has to do it (irrespective of the fact whether any LSM
> >> modified the creds or not).
> >
> > It starts getting messy when I have two modules that want to
> > change change the credential. Each module will have to check to
> > see if a module called before it has allocated a new cred.
> 
> Doesn't seem to me too difficult: check if *credp == NULL and allocate
> if so.  Can even invent a heper for this if needed.

Right. I like this approach. So cred allocation happens in LSM and
switching to new creds and freeing of new creds is done by caller.

That way, if no new creds are allocated, then caller does not have to
switch creds. Also all LSMs can work on single copy of newly allocated
cred and modify it. Also all LSMs can check if creds have already been
allocated otherwise allocate new one.

Thanks
Vivek

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Vivek Goyal]

On Fri, Jul 08, 2016 at 08:45:34AM -0400, Vivek Goyal wrote:

[..]
> > >>> I don't much care for the way part of the credential manipulation
> > >>> is done in the caller and part is done the the security module.
> > >>> If the caller is going to restore the old state, the caller should
> > >>> save the old state.
> > 
> > Conversely if the SM is setting the state it should restore it.
> > This needs yet another hook, but that's fine, I think.
> > 
> > >> One advantage of current patches is that we switch to new creds only if
> > >> it is needed. For example, if there are no LSMs loaded,
> > >
> > > Point.
> > >
> > >>  then there is
> > >> no need to modify creds and make a switch to new creds.
> > >
> > > I'm not a fan of cred flipping. There are too many ways for it to go
> > > wrong. Consider interrupts. I assume you've ruled that out as a possibility
> > > in the caller, but I still think the practice is dangerous.
> > >
> > > I greatly prefer "create and set attributes" to "change cred, create and
> > > reset cred". I know that has it's own set of problems, including races
> > > and faking privilege.
> > 
> > Yeah, we've talked about this. The races can be eliminated by always
> > doing the create in a the temporary "workdir" area and atomically
> > renaming to the final destination after everything has been set up.
> > OTOH that has a performance impact that the cred flipping eliminates.
> > 
> > >> But if I start allocating new creds and save old state in caller, then
> > >> caller always has to do it (irrespective of the fact whether any LSM
> > >> modified the creds or not).
> > >
> > > It starts getting messy when I have two modules that want to
> > > change change the credential. Each module will have to check to
> > > see if a module called before it has allocated a new cred.
> > 
> > Doesn't seem to me too difficult: check if *credp == NULL and allocate
> > if so.  Can even invent a heper for this if needed.
> 
> Right. I like this approach. So cred allocation happens in LSM and
> switching to new creds and freeing of new creds is done by caller.
> 
> That way, if no new creds are allocated, then caller does not have to
> switch creds. Also all LSMs can work on single copy of newly allocated
> cred and modify it. Also all LSMs can check if creds have already been
> allocated otherwise allocate new one.

How about something like this. This should allow stacking modules nicely
at the same time if no LSMs are loaded or LSM decide not to allocate new
creds,there is no need to switch creds.


Subject: security, overlayfs: provide copy up security hook for unioned files

Provide a security hook to label new file correctly when a file is copied
up from lower layer to upper layer of a overlay/union mount.

This hook can prepare a new set of creds which are suitable for new file
creation during copy up. Caller will use new creds to create file and then
revert back to old creds and release new creds.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---

 fs/overlayfs/copy_up.c    |   18 ++++++++++++++++++
 include/linux/lsm_hooks.h |   11 +++++++++++
 include/linux/security.h  |    6 ++++++
 security/security.c       |    8 ++++++++
 security/selinux/hooks.c  |   21 +++++++++++++++++++++
 5 files changed, 64 insertions(+)

Index: rhvgoyal-linux/include/linux/lsm_hooks.h
===================================================================
--- rhvgoyal-linux.orig/include/linux/lsm_hooks.h	2016-07-07 15:43:57.885498096 -0400
+++ rhvgoyal-linux/include/linux/lsm_hooks.h	2016-07-08 09:39:22.052676141 -0400
@@ -401,6 +401,15 @@
  *	@inode contains a pointer to the inode.
  *	@secid contains a pointer to the location where result will be saved.
  *	In case of failure, @secid will be set to zero.
+ * @inode_copy_up:
+ *	A file is about to be copied up from lower layer to upper layer of
+ *	overlay filesystem. Security module can prepare a set of new creds
+ *	and modify as need be and return new creds. Caller will switch to
+ *	new creds temporarily to create new file and release newly allocated
+ *	creds.
+ *	@src indicates the union dentry of file that is being copied up.
+ *	@new pointer to pointer to return newly allocated creds.
+ *	Returns 0 on success or a negative error code on error.
  *
  * Security hooks for file operations
  *
@@ -1425,6 +1434,7 @@ union security_list_options {
 	int (*inode_listsecurity)(struct inode *inode, char *buffer,
 					size_t buffer_size);
 	void (*inode_getsecid)(struct inode *inode, u32 *secid);
+	int (*inode_copy_up) (struct dentry *src, struct cred **new);
 
 	int (*file_permission)(struct file *file, int mask);
 	int (*file_alloc_security)(struct file *file);
@@ -1696,6 +1706,7 @@ struct security_hook_heads {
 	struct list_head inode_setsecurity;
 	struct list_head inode_listsecurity;
 	struct list_head inode_getsecid;
+	struct list_head inode_copy_up;
 	struct list_head file_permission;
 	struct list_head file_alloc_security;
 	struct list_head file_free_security;
Index: rhvgoyal-linux/include/linux/security.h
===================================================================
--- rhvgoyal-linux.orig/include/linux/security.h	2016-07-07 15:43:57.885498096 -0400
+++ rhvgoyal-linux/include/linux/security.h	2016-07-08 09:39:22.052676141 -0400
@@ -282,6 +282,7 @@ int security_inode_getsecurity(struct in
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
+int security_inode_copy_up(struct dentry *src, struct cred **new);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -758,6 +759,11 @@ static inline void security_inode_getsec
 	*secid = 0;
 }
 
+static inline int security_inode_copy_up(struct dentry *src, struct cred **new)
+{
+	return 0;
+}
+
 static inline int security_file_permission(struct file *file, int mask)
 {
 	return 0;
Index: rhvgoyal-linux/security/security.c
===================================================================
--- rhvgoyal-linux.orig/security/security.c	2016-07-07 15:43:57.885498096 -0400
+++ rhvgoyal-linux/security/security.c	2016-07-08 09:39:22.052676141 -0400
@@ -727,6 +727,12 @@ void security_inode_getsecid(struct inod
 	call_void_hook(inode_getsecid, inode, secid);
 }
 
+int security_inode_copy_up(struct dentry *src, struct cred **new)
+{
+	return call_int_hook(inode_copy_up, 0, src, new);
+}
+EXPORT_SYMBOL(security_inode_copy_up);
+
 int security_file_permission(struct file *file, int mask)
 {
 	int ret;
@@ -1663,6 +1669,8 @@ struct security_hook_heads security_hook
 		LIST_HEAD_INIT(security_hook_heads.inode_listsecurity),
 	.inode_getsecid =
 		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
+	.inode_copy_up =
+		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
 	.file_permission =
 		LIST_HEAD_INIT(security_hook_heads.file_permission),
 	.file_alloc_security =
Index: rhvgoyal-linux/fs/overlayfs/copy_up.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/copy_up.c	2016-07-07 15:43:57.885498096 -0400
+++ rhvgoyal-linux/fs/overlayfs/copy_up.c	2016-07-08 09:39:22.052676141 -0400
@@ -246,6 +246,8 @@ static int ovl_copy_up_locked(struct den
 	struct dentry *upper = NULL;
 	umode_t mode = stat->mode;
 	int err;
+	const struct cred *old_creds = NULL;
+	struct cred *new_creds = NULL;
 
 	newdentry = ovl_lookup_temp(workdir, dentry);
 	err = PTR_ERR(newdentry);
@@ -258,10 +260,26 @@ static int ovl_copy_up_locked(struct den
 	if (IS_ERR(upper))
 		goto out1;
 
+	err = security_inode_copy_up(dentry, &new_creds);
+	if (err < 0) {
+		if (new_creds)
+			put_cred(new_creds);
+		goto out2;
+	}
+
+	if (new_creds)
+		old_creds = override_creds(new_creds);
+
 	/* Can't properly set mode on creation because of the umask */
 	stat->mode &= S_IFMT;
 	err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
 	stat->mode = mode;
+
+	if (new_creds) {
+		revert_creds(old_creds);
+		put_cred(new_creds);
+	}
+
 	if (err)
 		goto out2;
 
Index: rhvgoyal-linux/security/selinux/hooks.c
===================================================================
--- rhvgoyal-linux.orig/security/selinux/hooks.c	2016-07-08 09:39:24.261676141 -0400
+++ rhvgoyal-linux/security/selinux/hooks.c	2016-07-08 09:39:32.651676141 -0400
@@ -3270,6 +3270,26 @@ static void selinux_inode_getsecid(struc
 	*secid = isec->sid;
 }
 
+static int selinux_inode_copy_up(struct dentry *src, struct cred **new)
+{
+	u32 sid;
+	struct task_security_struct *tsec;
+	struct cred *new_creds = *new;
+
+	if (new_creds == NULL) {
+		new_creds = prepare_creds();
+		if (!new_creds)
+			return -ENOMEM;
+	}
+
+	tsec = new_creds->security;
+	/* Get label from overlay inode and set it in create_sid */
+	selinux_inode_getsecid(d_inode(src), &sid);
+	tsec->create_sid = sid;
+	*new = new_creds;
+	return 0;
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -6056,6 +6076,7 @@ static struct security_hook_list selinux
 	LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity),
 	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
 	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
+	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
 
 	LSM_HOOK_INIT(file_permission, selinux_file_permission),
 	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Casey Schaufler]

On 7/8/2016 6:42 AM, Vivek Goyal wrote:
> On Fri, Jul 08, 2016 at 08:45:34AM -0400, Vivek Goyal wrote:
>
> [..]
>>>>>> I don't much care for the way part of the credential manipulation
>>>>>> is done in the caller and part is done the the security module.
>>>>>> If the caller is going to restore the old state, the caller should
>>>>>> save the old state.
>>> Conversely if the SM is setting the state it should restore it.
>>> This needs yet another hook, but that's fine, I think.
>>>
>>>>> One advantage of current patches is that we switch to new creds only if
>>>>> it is needed. For example, if there are no LSMs loaded,
>>>> Point.
>>>>
>>>>>  then there is
>>>>> no need to modify creds and make a switch to new creds.
>>>> I'm not a fan of cred flipping. There are too many ways for it to go
>>>> wrong. Consider interrupts. I assume you've ruled that out as a possibility
>>>> in the caller, but I still think the practice is dangerous.
>>>>
>>>> I greatly prefer "create and set attributes" to "change cred, create and
>>>> reset cred". I know that has it's own set of problems, including races
>>>> and faking privilege.
>>> Yeah, we've talked about this. The races can be eliminated by always
>>> doing the create in a the temporary "workdir" area and atomically
>>> renaming to the final destination after everything has been set up.
>>> OTOH that has a performance impact that the cred flipping eliminates.
>>>
>>>>> But if I start allocating new creds and save old state in caller, then
>>>>> caller always has to do it (irrespective of the fact whether any LSM
>>>>> modified the creds or not).
>>>> It starts getting messy when I have two modules that want to
>>>> change change the credential. Each module will have to check to
>>>> see if a module called before it has allocated a new cred.
>>> Doesn't seem to me too difficult: check if *credp == NULL and allocate
>>> if so.  Can even invent a heper for this if needed.
>> Right. I like this approach. So cred allocation happens in LSM and
>> switching to new creds and freeing of new creds is done by caller.
>>
>> That way, if no new creds are allocated, then caller does not have to
>> switch creds. Also all LSMs can work on single copy of newly allocated
>> cred and modify it. Also all LSMs can check if creds have already been
>> allocated otherwise allocate new one.
> How about something like this. This should allow stacking modules nicely
> at the same time if no LSMs are loaded or LSM decide not to allocate new
> creds,there is no need to switch creds.
>
>
> Subject: security, overlayfs: provide copy up security hook for unioned files
>
> Provide a security hook to label new file correctly when a file is copied
> up from lower layer to upper layer of a overlay/union mount.
>
> This hook can prepare a new set of creds which are suitable for new file
> creation during copy up. Caller will use new creds to create file and then
> revert back to old creds and release new creds.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

I like this better. 

> ---
>
>  fs/overlayfs/copy_up.c    |   18 ++++++++++++++++++
>  include/linux/lsm_hooks.h |   11 +++++++++++
>  include/linux/security.h  |    6 ++++++
>  security/security.c       |    8 ++++++++
>  security/selinux/hooks.c  |   21 +++++++++++++++++++++
>  5 files changed, 64 insertions(+)
>
> Index: rhvgoyal-linux/include/linux/lsm_hooks.h
> ===================================================================
> --- rhvgoyal-linux.orig/include/linux/lsm_hooks.h	2016-07-07 15:43:57.885498096 -0400
> +++ rhvgoyal-linux/include/linux/lsm_hooks.h	2016-07-08 09:39:22.052676141 -0400
> @@ -401,6 +401,15 @@
>   *	@inode contains a pointer to the inode.
>   *	@secid contains a pointer to the location where result will be saved.
>   *	In case of failure, @secid will be set to zero.
> + * @inode_copy_up:
> + *	A file is about to be copied up from lower layer to upper layer of
> + *	overlay filesystem. Security module can prepare a set of new creds
> + *	and modify as need be and return new creds. Caller will switch to
> + *	new creds temporarily to create new file and release newly allocated
> + *	creds.
> + *	@src indicates the union dentry of file that is being copied up.
> + *	@new pointer to pointer to return newly allocated creds.
> + *	Returns 0 on success or a negative error code on error.
>   *
>   * Security hooks for file operations
>   *
> @@ -1425,6 +1434,7 @@ union security_list_options {
>  	int (*inode_listsecurity)(struct inode *inode, char *buffer,
>  					size_t buffer_size);
>  	void (*inode_getsecid)(struct inode *inode, u32 *secid);
> +	int (*inode_copy_up) (struct dentry *src, struct cred **new);
>  
>  	int (*file_permission)(struct file *file, int mask);
>  	int (*file_alloc_security)(struct file *file);
> @@ -1696,6 +1706,7 @@ struct security_hook_heads {
>  	struct list_head inode_setsecurity;
>  	struct list_head inode_listsecurity;
>  	struct list_head inode_getsecid;
> +	struct list_head inode_copy_up;
>  	struct list_head file_permission;
>  	struct list_head file_alloc_security;
>  	struct list_head file_free_security;
> Index: rhvgoyal-linux/include/linux/security.h
> ===================================================================
> --- rhvgoyal-linux.orig/include/linux/security.h	2016-07-07 15:43:57.885498096 -0400
> +++ rhvgoyal-linux/include/linux/security.h	2016-07-08 09:39:22.052676141 -0400
> @@ -282,6 +282,7 @@ int security_inode_getsecurity(struct in
>  int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
>  int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
>  void security_inode_getsecid(struct inode *inode, u32 *secid);
> +int security_inode_copy_up(struct dentry *src, struct cred **new);
>  int security_file_permission(struct file *file, int mask);
>  int security_file_alloc(struct file *file);
>  void security_file_free(struct file *file);
> @@ -758,6 +759,11 @@ static inline void security_inode_getsec
>  	*secid = 0;
>  }
>  
> +static inline int security_inode_copy_up(struct dentry *src, struct cred **new)
> +{
> +	return 0;
> +}
> +
>  static inline int security_file_permission(struct file *file, int mask)
>  {
>  	return 0;
> Index: rhvgoyal-linux/security/security.c
> ===================================================================
> --- rhvgoyal-linux.orig/security/security.c	2016-07-07 15:43:57.885498096 -0400
> +++ rhvgoyal-linux/security/security.c	2016-07-08 09:39:22.052676141 -0400
> @@ -727,6 +727,12 @@ void security_inode_getsecid(struct inod
>  	call_void_hook(inode_getsecid, inode, secid);
>  }
>  
> +int security_inode_copy_up(struct dentry *src, struct cred **new)
> +{
> +	return call_int_hook(inode_copy_up, 0, src, new);
> +}
> +EXPORT_SYMBOL(security_inode_copy_up);
> +
>  int security_file_permission(struct file *file, int mask)
>  {
>  	int ret;
> @@ -1663,6 +1669,8 @@ struct security_hook_heads security_hook
>  		LIST_HEAD_INIT(security_hook_heads.inode_listsecurity),
>  	.inode_getsecid =
>  		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
> +	.inode_copy_up =
> +		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
>  	.file_permission =
>  		LIST_HEAD_INIT(security_hook_heads.file_permission),
>  	.file_alloc_security =
> Index: rhvgoyal-linux/fs/overlayfs/copy_up.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/copy_up.c	2016-07-07 15:43:57.885498096 -0400
> +++ rhvgoyal-linux/fs/overlayfs/copy_up.c	2016-07-08 09:39:22.052676141 -0400
> @@ -246,6 +246,8 @@ static int ovl_copy_up_locked(struct den
>  	struct dentry *upper = NULL;
>  	umode_t mode = stat->mode;
>  	int err;
> +	const struct cred *old_creds = NULL;
> +	struct cred *new_creds = NULL;
>  
>  	newdentry = ovl_lookup_temp(workdir, dentry);
>  	err = PTR_ERR(newdentry);
> @@ -258,10 +260,26 @@ static int ovl_copy_up_locked(struct den
>  	if (IS_ERR(upper))
>  		goto out1;
>  
> +	err = security_inode_copy_up(dentry, &new_creds);
> +	if (err < 0) {
> +		if (new_creds)
> +			put_cred(new_creds);
> +		goto out2;
> +	}
> +
> +	if (new_creds)
> +		old_creds = override_creds(new_creds);
> +
>  	/* Can't properly set mode on creation because of the umask */
>  	stat->mode &= S_IFMT;
>  	err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
>  	stat->mode = mode;
> +
> +	if (new_creds) {
> +		revert_creds(old_creds);
> +		put_cred(new_creds);
> +	}
> +
>  	if (err)
>  		goto out2;
>  
> Index: rhvgoyal-linux/security/selinux/hooks.c
> ===================================================================
> --- rhvgoyal-linux.orig/security/selinux/hooks.c	2016-07-08 09:39:24.261676141 -0400
> +++ rhvgoyal-linux/security/selinux/hooks.c	2016-07-08 09:39:32.651676141 -0400
> @@ -3270,6 +3270,26 @@ static void selinux_inode_getsecid(struc
>  	*secid = isec->sid;
>  }
>  
> +static int selinux_inode_copy_up(struct dentry *src, struct cred **new)
> +{
> +	u32 sid;
> +	struct task_security_struct *tsec;
> +	struct cred *new_creds = *new;
> +
> +	if (new_creds == NULL) {
> +		new_creds = prepare_creds();
> +		if (!new_creds)
> +			return -ENOMEM;
> +	}
> +
> +	tsec = new_creds->security;
> +	/* Get label from overlay inode and set it in create_sid */
> +	selinux_inode_getsecid(d_inode(src), &sid);
> +	tsec->create_sid = sid;
> +	*new = new_creds;
> +	return 0;
> +}
> +
>  /* file security operations */
>  
>  static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -6056,6 +6076,7 @@ static struct security_hook_list selinux
>  	LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity),
>  	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
>  	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
> +	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
>  
>  	LSM_HOOK_INIT(file_permission, selinux_file_permission),
>  	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
>

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Paul Moore]

On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Provide a security hook to label new file correctly when a file is copied
> up from lower layer to upper layer of a overlay/union mount.
>
> This hook can prepare and switch to a new set of creds which are suitable
> for new file creation during copy up. Caller should revert to old creds
> after file creation.
>
> In SELinux, newly copied up file gets same label as lower file for
> non-context mounts. But it gets label specified in mount option context=
> for context mounts.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c    |  8 ++++++++
>  include/linux/lsm_hooks.h | 13 +++++++++++++
>  include/linux/security.h  |  6 ++++++
>  security/security.c       |  8 ++++++++
>  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+)

..

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a86d537..1b1a1e5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
>         *secid = isec->sid;
>  }
>
> +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old)
> +{
> +       u32 sid;
> +       struct cred *new_creds;
> +       struct task_security_struct *tsec;
> +
> +       new_creds = prepare_creds();
> +       if (!new_creds)
> +               return -ENOMEM;
> +
> +       /* Get label from overlay inode and set it in create_sid */
> +       selinux_inode_getsecid(d_inode(src), &sid);
> +       tsec = new_creds->security;
> +       tsec->create_sid = sid;
> +       *old = override_creds(new_creds);
> +
> +       /*
> +        * At this point of time we have 2 refs on new_creds. One by
> +        * prepare_creds and other by override_creds. Drop one reference
> +        * so that as soon as caller calls revert_creds(old), this cred
> +        * will be freed.
> +        */
> +       put_cred(new_creds);
> +       return 0;
> +}

One quick point of clarification: in addition to the SELinux specific
comments in lsm_hooks.h, I think it would be a good idea if the
SELinux hook implementation, e.g. security/selinux/hooks.c, was in its
own patch and not part of the hook definition.

Beyond that, I'm not overly excited about reusing create_sid for this
purpose.  I understand why you did it, but what if the process had
explicitly set create_sid?  I think I would prefer the creation of a
new field (create_up_sid?) to track this new label and then add an
additional check to selinux_inode_init_security() to prefer the
existing create_sid to this new field when both are set.

Sound reasonable?

-- 
paul moore
security @ redhat

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Vivek Goyal]

On Tue, Jul 05, 2016 at 05:35:22PM -0400, Paul Moore wrote:
> On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Provide a security hook to label new file correctly when a file is copied
> > up from lower layer to upper layer of a overlay/union mount.
> >
> > This hook can prepare and switch to a new set of creds which are suitable
> > for new file creation during copy up. Caller should revert to old creds
> > after file creation.
> >
> > In SELinux, newly copied up file gets same label as lower file for
> > non-context mounts. But it gets label specified in mount option context=
> > for context mounts.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c    |  8 ++++++++
> >  include/linux/lsm_hooks.h | 13 +++++++++++++
> >  include/linux/security.h  |  6 ++++++
> >  security/security.c       |  8 ++++++++
> >  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
> >  5 files changed, 62 insertions(+)
> 
> ..
> 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index a86d537..1b1a1e5 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
> >         *secid = isec->sid;
> >  }
> >
> > +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old)
> > +{
> > +       u32 sid;
> > +       struct cred *new_creds;
> > +       struct task_security_struct *tsec;
> > +
> > +       new_creds = prepare_creds();
> > +       if (!new_creds)
> > +               return -ENOMEM;
> > +
> > +       /* Get label from overlay inode and set it in create_sid */
> > +       selinux_inode_getsecid(d_inode(src), &sid);
> > +       tsec = new_creds->security;
> > +       tsec->create_sid = sid;
> > +       *old = override_creds(new_creds);
> > +
> > +       /*
> > +        * At this point of time we have 2 refs on new_creds. One by
> > +        * prepare_creds and other by override_creds. Drop one reference
> > +        * so that as soon as caller calls revert_creds(old), this cred
> > +        * will be freed.
> > +        */
> > +       put_cred(new_creds);
> > +       return 0;
> > +}
> 
> One quick point of clarification: in addition to the SELinux specific
> comments in lsm_hooks.h, I think it would be a good idea if the
> SELinux hook implementation, e.g. security/selinux/hooks.c, was in its
> own patch and not part of the hook definition.

Ok, may be I will break every hook in three parts.

- lsm hook declaration.
- selinux implementation
- overlay implementation of call to hook.

> 
> Beyond that, I'm not overly excited about reusing create_sid for this
> purpose.  I understand why you did it, but what if the process had
> explicitly set create_sid?

When a file is copied up, either we retain the label of lower file or
set the new label from context=. If any create_sid is set in task, that's
ignored.

And as we are setting create_sid in a new set of credentials, task will
get to retain its create_sid for future operations.

As task does not know we are creating a new file, create_sid of task
should not matter at all. Task does not know if file is on upper or
file is being copied up. For task this file already exists, so task
should not expect create_sid label to be present.

Am I missing something.

Vivek

> I think I would prefer the creation of a
> new field (create_up_sid?) to track this new label and then add an
> additional check to selinux_inode_init_security() to prefer the
> existing create_sid to this new field when both are set.
> 
> Sound reasonable?
> 
> -- 
> paul moore
> security @ redhat

Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Paul Moore]

On Tue, Jul 5, 2016 at 5:52 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Jul 05, 2016 at 05:35:22PM -0400, Paul Moore wrote:
>> On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > Provide a security hook to label new file correctly when a file is copied
>> > up from lower layer to upper layer of a overlay/union mount.
>> >
>> > This hook can prepare and switch to a new set of creds which are suitable
>> > for new file creation during copy up. Caller should revert to old creds
>> > after file creation.
>> >
>> > In SELinux, newly copied up file gets same label as lower file for
>> > non-context mounts. But it gets label specified in mount option context=
>> > for context mounts.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/copy_up.c    |  8 ++++++++
>> >  include/linux/lsm_hooks.h | 13 +++++++++++++
>> >  include/linux/security.h  |  6 ++++++
>> >  security/security.c       |  8 ++++++++
>> >  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
>> >  5 files changed, 62 insertions(+)
>>
>> ..
>>
>> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> > index a86d537..1b1a1e5 100644
>> > --- a/security/selinux/hooks.c
>> > +++ b/security/selinux/hooks.c
>> > @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
>> >         *secid = isec->sid;
>> >  }
>> >
>> > +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old)
>> > +{
>> > +       u32 sid;
>> > +       struct cred *new_creds;
>> > +       struct task_security_struct *tsec;
>> > +
>> > +       new_creds = prepare_creds();
>> > +       if (!new_creds)
>> > +               return -ENOMEM;
>> > +
>> > +       /* Get label from overlay inode and set it in create_sid */
>> > +       selinux_inode_getsecid(d_inode(src), &sid);
>> > +       tsec = new_creds->security;
>> > +       tsec->create_sid = sid;
>> > +       *old = override_creds(new_creds);
>> > +
>> > +       /*
>> > +        * At this point of time we have 2 refs on new_creds. One by
>> > +        * prepare_creds and other by override_creds. Drop one reference
>> > +        * so that as soon as caller calls revert_creds(old), this cred
>> > +        * will be freed.
>> > +        */
>> > +       put_cred(new_creds);
>> > +       return 0;
>> > +}

...

>> Beyond that, I'm not overly excited about reusing create_sid for this
>> purpose.  I understand why you did it, but what if the process had
>> explicitly set create_sid?
>
> When a file is copied up, either we retain the label of lower file or
> set the new label from context=. If any create_sid is set in task, that's
> ignored.
>
> And as we are setting create_sid in a new set of credentials, task will
> get to retain its create_sid for future operations.
>
> As task does not know we are creating a new file, create_sid of task
> should not matter at all. Task does not know if file is on upper or
> file is being copied up. For task this file already exists, so task
> should not expect create_sid label to be present.
>
> Am I missing something.

I forgot that you are manufacturing a new set of credentials; I must
have lost track of that when I was walking through some of the VFS
code, my mistake.  I'm still rather uneasy about this, but at least
you aren't overwriting a previously stored create_sid value.

-- 
paul moore
security @ redhat

[PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Vivek Goyal]

Provide a security hook which is called when xattrs of a file are being
copied up. This hook is called once for each xattr and one can either
accept or reject xattr. If 0 is returned, xattr will be copied up, if 1
is returned, xattr will not be copied up and if negative error code
is returned, copy up will be aborted.

In SELinux, label of lower file is not copied up. File already has been
set with right label at the time of creation and we don't want to overwrite
that label.

Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c    |  8 ++++++++
 include/linux/lsm_hooks.h | 13 +++++++++++++
 include/linux/security.h  | 10 ++++++++++
 security/security.c       |  9 +++++++++
 security/selinux/hooks.c  | 14 ++++++++++++++
 5 files changed, 54 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 90dc362..2c31938 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -103,6 +103,14 @@ retry:
 			goto retry;
 		}
 
+		error = security_inode_copy_up_xattr(old, new,
+						     name, value, size);
+		if (error < 0)
+			break;
+		if (error == 1) {
+			error = 0;
+			continue; /* Discard */
+		}
 		error = vfs_setxattr(new, name, value, size, 0);
 		if (error)
 			break;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index fcde9b9..2a8ee8c 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -412,6 +412,16 @@
  *	@src indicates the union dentry of file that is being copied up.
  *	@old indicates the pointer to old_cred returned to caller.
  *	Returns 0 on success or a negative error code on error.
+ * @inode_copy_up_xattr:
+ *	Filter the xattrs being copied up when a unioned file is copied
+ *	up from a lower layer to the union/overlay layer.
+ *	@src indicates the file that is being copied up.
+ *	@dst indicates the file that has being created by the copy up.
+ *	@name indicates the name of the xattr.
+ *	@value, @size indicate the payload of the xattr.
+ *	Returns 0 to accept the xattr, 1 to discard the xattr or a negative
+ *	error code to abort the copy up. Note that the caller is responsible
+ *	for reading and writing the xattrs as this hook is merely a filter.
  *
  * Security hooks for file operations
  *
@@ -1437,6 +1447,8 @@ union security_list_options {
 					size_t buffer_size);
 	void (*inode_getsecid)(struct inode *inode, u32 *secid);
 	int (*inode_copy_up) (struct dentry *src, const struct cred **old);
+	int (*inode_copy_up_xattr) (struct dentry *src, struct dentry *dst,
+				    const char *name, void *value, size_t size);
 
 	int (*file_permission)(struct file *file, int mask);
 	int (*file_alloc_security)(struct file *file);
@@ -1709,6 +1721,7 @@ struct security_hook_heads {
 	struct list_head inode_listsecurity;
 	struct list_head inode_getsecid;
 	struct list_head inode_copy_up;
+	struct list_head inode_copy_up_xattr;
 	struct list_head file_permission;
 	struct list_head file_alloc_security;
 	struct list_head file_free_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 3445df2..663ca15 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -283,6 +283,8 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
 int security_inode_copy_up(struct dentry *src, const struct cred **old);
+int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				 const char *name, void *value, size_t size);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -764,6 +766,14 @@ static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
 	return 0;
 }
 
+static inline int security_inode_copy_up_xattr(struct dentry *src,
+					       struct dentry *dst,
+					       const char *name,
+					       const void *value, size_t size)
+{
+	return 0;
+}
+
 static inline int security_file_permission(struct file *file, int mask)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 7c1ce29..87712c6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -733,6 +733,13 @@ int security_inode_copy_up(struct dentry *src, const struct cred **old)
 }
 EXPORT_SYMBOL(security_inode_copy_up);
 
+int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				 const char *name, void *value, size_t size)
+{
+	return call_int_hook(inode_copy_up_xattr, 0, src, dst, name, value, size);
+}
+EXPORT_SYMBOL(security_inode_copy_up_xattr);
+
 int security_file_permission(struct file *file, int mask)
 {
 	int ret;
@@ -1671,6 +1678,8 @@ struct security_hook_heads security_hook_heads = {
 		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
 	.inode_copy_up =
 		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
+	.inode_copy_up_xattr =
+		LIST_HEAD_INIT(security_hook_heads.inode_copy_up_xattr),
 	.file_permission =
 		LIST_HEAD_INIT(security_hook_heads.file_permission),
 	.file_alloc_security =
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1b1a1e5..c68223c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3296,6 +3296,19 @@ static int selinux_inode_copy_up(struct dentry *src, const struct cred **old)
 	return 0;
 }
 
+static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				       const char *name, void *value,
+				       size_t size)
+{
+	/* The copy_up hook above sets the initial context on an inode, but we
+	 * don't then want to overwrite it by blindly copying all the lower
+	 * xattrs up.  Instead, we have to filter out SELinux-related xattrs.
+	 */
+	if (strcmp(name, XATTR_NAME_SELINUX) == 0)
+		return 1; /* Discard */
+	return 0;
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -6083,6 +6096,7 @@ static struct security_hook_list selinux_hooks[] = {
 	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
 	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
 	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
+	LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
 
 	LSM_HOOK_INIT(file_permission, selinux_file_permission),
 	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
-- 
2.7.4

Re: [PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Casey Schaufler]

On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> Provide a security hook which is called when xattrs of a file are being
> copied up. This hook is called once for each xattr and one can either
> accept or reject xattr. If 0 is returned, xattr will be copied up, if 1
> is returned, xattr will not be copied up and if negative error code
> is returned, copy up will be aborted.
>
> In SELinux, label of lower file is not copied up. File already has been
> set with right label at the time of creation and we don't want to overwrite
> that label.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c    |  8 ++++++++
>  include/linux/lsm_hooks.h | 13 +++++++++++++
>  include/linux/security.h  | 10 ++++++++++
>  security/security.c       |  9 +++++++++
>  security/selinux/hooks.c  | 14 ++++++++++++++
>  5 files changed, 54 insertions(+)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 90dc362..2c31938 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -103,6 +103,14 @@ retry:
>  			goto retry;
>  		}
>  
> +		error = security_inode_copy_up_xattr(old, new,
> +						     name, value, size);
> +		if (error < 0)
> +			break;
> +		if (error == 1) {
> +			error = 0;
> +			continue; /* Discard */
> +		}
>  		error = vfs_setxattr(new, name, value, size, 0);
>  		if (error)
>  			break;
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index fcde9b9..2a8ee8c 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -412,6 +412,16 @@
>   *	@src indicates the union dentry of file that is being copied up.
>   *	@old indicates the pointer to old_cred returned to caller.
>   *	Returns 0 on success or a negative error code on error.
> + * @inode_copy_up_xattr:
> + *	Filter the xattrs being copied up when a unioned file is copied
> + *	up from a lower layer to the union/overlay layer.
> + *	@src indicates the file that is being copied up.
> + *	@dst indicates the file that has being created by the copy up.
> + *	@name indicates the name of the xattr.
> + *	@value, @size indicate the payload of the xattr.
> + *	Returns 0 to accept the xattr, 1 to discard the xattr or a negative
> + *	error code to abort the copy up. Note that the caller is responsible
> + *	for reading and writing the xattrs as this hook is merely a filter.

The return should be -EOPNOTSUPP from security modules that don't
support the attribute "name". This will make it possible to support
multiple modules that provide attributes. (patches pending)

If the only use to which this hook is put is to identify attributes
that should be discarded it's unnecessary overhead to pass the
parameters that are never used.

>   *
>   * Security hooks for file operations
>   *
> @@ -1437,6 +1447,8 @@ union security_list_options {
>  					size_t buffer_size);
>  	void (*inode_getsecid)(struct inode *inode, u32 *secid);
>  	int (*inode_copy_up) (struct dentry *src, const struct cred **old);
> +	int (*inode_copy_up_xattr) (struct dentry *src, struct dentry *dst,
> +				    const char *name, void *value, size_t size);
>  
>  	int (*file_permission)(struct file *file, int mask);
>  	int (*file_alloc_security)(struct file *file);
> @@ -1709,6 +1721,7 @@ struct security_hook_heads {
>  	struct list_head inode_listsecurity;
>  	struct list_head inode_getsecid;
>  	struct list_head inode_copy_up;
> +	struct list_head inode_copy_up_xattr;
>  	struct list_head file_permission;
>  	struct list_head file_alloc_security;
>  	struct list_head file_free_security;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 3445df2..663ca15 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -283,6 +283,8 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
>  int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
>  void security_inode_getsecid(struct inode *inode, u32 *secid);
>  int security_inode_copy_up(struct dentry *src, const struct cred **old);
> +int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
> +				 const char *name, void *value, size_t size);
>  int security_file_permission(struct file *file, int mask);
>  int security_file_alloc(struct file *file);
>  void security_file_free(struct file *file);
> @@ -764,6 +766,14 @@ static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
>  	return 0;
>  }
>  
> +static inline int security_inode_copy_up_xattr(struct dentry *src,
> +					       struct dentry *dst,
> +					       const char *name,
> +					       const void *value, size_t size)
> +{
> +	return 0;
> +}
> +
>  static inline int security_file_permission(struct file *file, int mask)
>  {
>  	return 0;
> diff --git a/security/security.c b/security/security.c
> index 7c1ce29..87712c6 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -733,6 +733,13 @@ int security_inode_copy_up(struct dentry *src, const struct cred **old)
>  }
>  EXPORT_SYMBOL(security_inode_copy_up);
>  
> +int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
> +				 const char *name, void *value, size_t size)
> +{
> +	return call_int_hook(inode_copy_up_xattr, 0, src, dst, name, value, size);
> +}
> +EXPORT_SYMBOL(security_inode_copy_up_xattr);
> +
>  int security_file_permission(struct file *file, int mask)
>  {
>  	int ret;
> @@ -1671,6 +1678,8 @@ struct security_hook_heads security_hook_heads = {
>  		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
>  	.inode_copy_up =
>  		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
> +	.inode_copy_up_xattr =
> +		LIST_HEAD_INIT(security_hook_heads.inode_copy_up_xattr),
>  	.file_permission =
>  		LIST_HEAD_INIT(security_hook_heads.file_permission),
>  	.file_alloc_security =
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1b1a1e5..c68223c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3296,6 +3296,19 @@ static int selinux_inode_copy_up(struct dentry *src, const struct cred **old)
>  	return 0;
>  }
>  
> +static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
> +				       const char *name, void *value,
> +				       size_t size)
> +{
> +	/* The copy_up hook above sets the initial context on an inode, but we
> +	 * don't then want to overwrite it by blindly copying all the lower
> +	 * xattrs up.  Instead, we have to filter out SELinux-related xattrs.
> +	 */
> +	if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> +		return 1; /* Discard */
> +	return 0;
> +}
> +
>  /* file security operations */
>  
>  static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -6083,6 +6096,7 @@ static struct security_hook_list selinux_hooks[] = {
>  	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
>  	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
>  	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
> +	LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
>  
>  	LSM_HOOK_INIT(file_permission, selinux_file_permission),
>  	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),

Re: [PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Vivek Goyal]

On Tue, Jul 05, 2016 at 01:22:22PM -0700, Casey Schaufler wrote:
> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> > Provide a security hook which is called when xattrs of a file are being
> > copied up. This hook is called once for each xattr and one can either
> > accept or reject xattr. If 0 is returned, xattr will be copied up, if 1
> > is returned, xattr will not be copied up and if negative error code
> > is returned, copy up will be aborted.
> >
> > In SELinux, label of lower file is not copied up. File already has been
> > set with right label at the time of creation and we don't want to overwrite
> > that label.
> >
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c    |  8 ++++++++
> >  include/linux/lsm_hooks.h | 13 +++++++++++++
> >  include/linux/security.h  | 10 ++++++++++
> >  security/security.c       |  9 +++++++++
> >  security/selinux/hooks.c  | 14 ++++++++++++++
> >  5 files changed, 54 insertions(+)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 90dc362..2c31938 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -103,6 +103,14 @@ retry:
> >  			goto retry;
> >  		}
> >  
> > +		error = security_inode_copy_up_xattr(old, new,
> > +						     name, value, size);
> > +		if (error < 0)
> > +			break;
> > +		if (error == 1) {
> > +			error = 0;
> > +			continue; /* Discard */
> > +		}
> >  		error = vfs_setxattr(new, name, value, size, 0);
> >  		if (error)
> >  			break;
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index fcde9b9..2a8ee8c 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -412,6 +412,16 @@
> >   *	@src indicates the union dentry of file that is being copied up.
> >   *	@old indicates the pointer to old_cred returned to caller.
> >   *	Returns 0 on success or a negative error code on error.
> > + * @inode_copy_up_xattr:
> > + *	Filter the xattrs being copied up when a unioned file is copied
> > + *	up from a lower layer to the union/overlay layer.
> > + *	@src indicates the file that is being copied up.
> > + *	@dst indicates the file that has being created by the copy up.
> > + *	@name indicates the name of the xattr.
> > + *	@value, @size indicate the payload of the xattr.
> > + *	Returns 0 to accept the xattr, 1 to discard the xattr or a negative
> > + *	error code to abort the copy up. Note that the caller is responsible
> > + *	for reading and writing the xattrs as this hook is merely a filter.
> 
> The return should be -EOPNOTSUPP from security modules that don't
> support the attribute "name". This will make it possible to support
> multiple modules that provide attributes. (patches pending)

Hmm.., Sorry I did not understand this one. 

So all modules will not understand all xattrs. So if they start returning
-EOPNOTSUPP, then as per current implementation, copy up operation will
be aborted. 

Current implementation relies on that a security module, returns 0 if
every thing is "name" xattr should be copied up or lsm does not care.
Negative error code is returned only if something is wrong. Given every
lsm will not understand/care about all the xattrs, we can't return 
error code if lsm does not own/understand the "name". In fact
call_int_hook() will bail out the very first time negative error code
is returned. 

IOW, current implementation will work with multiple modules providing
implementation for same hook as long as module returns 0 for the xattrs
it does not understand. 

I guess I am missing something. Can you please elaborate a little more.

> 
> If the only use to which this hook is put is to identify attributes
> that should be discarded it's unnecessary overhead to pass the
> parameters that are never used.

Ok, I will get rid of extra parameters. If somebody needs these, it can
be added later.

Vivek

Re: [PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Casey Schaufler]

On 7/5/2016 2:15 PM, Vivek Goyal wrote:
> On Tue, Jul 05, 2016 at 01:22:22PM -0700, Casey Schaufler wrote:
>> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
>>> Provide a security hook which is called when xattrs of a file are being
>>> copied up. This hook is called once for each xattr and one can either
>>> accept or reject xattr. If 0 is returned, xattr will be copied up, if 1
>>> is returned, xattr will not be copied up and if negative error code
>>> is returned, copy up will be aborted.
>>>
>>> In SELinux, label of lower file is not copied up. File already has been
>>> set with right label at the time of creation and we don't want to overwrite
>>> that label.
>>>
>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>> ---
>>>  fs/overlayfs/copy_up.c    |  8 ++++++++
>>>  include/linux/lsm_hooks.h | 13 +++++++++++++
>>>  include/linux/security.h  | 10 ++++++++++
>>>  security/security.c       |  9 +++++++++
>>>  security/selinux/hooks.c  | 14 ++++++++++++++
>>>  5 files changed, 54 insertions(+)
>>>
>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>>> index 90dc362..2c31938 100644
>>> --- a/fs/overlayfs/copy_up.c
>>> +++ b/fs/overlayfs/copy_up.c
>>> @@ -103,6 +103,14 @@ retry:
>>>  			goto retry;
>>>  		}
>>>  
>>> +		error = security_inode_copy_up_xattr(old, new,
>>> +						     name, value, size);
>>> +		if (error < 0)
>>> +			break;
>>> +		if (error == 1) {
>>> +			error = 0;
>>> +			continue; /* Discard */
>>> +		}
>>>  		error = vfs_setxattr(new, name, value, size, 0);
>>>  		if (error)
>>>  			break;
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index fcde9b9..2a8ee8c 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -412,6 +412,16 @@
>>>   *	@src indicates the union dentry of file that is being copied up.
>>>   *	@old indicates the pointer to old_cred returned to caller.
>>>   *	Returns 0 on success or a negative error code on error.
>>> + * @inode_copy_up_xattr:
>>> + *	Filter the xattrs being copied up when a unioned file is copied
>>> + *	up from a lower layer to the union/overlay layer.
>>> + *	@src indicates the file that is being copied up.
>>> + *	@dst indicates the file that has being created by the copy up.
>>> + *	@name indicates the name of the xattr.
>>> + *	@value, @size indicate the payload of the xattr.
>>> + *	Returns 0 to accept the xattr, 1 to discard the xattr or a negative
>>> + *	error code to abort the copy up. Note that the caller is responsible
>>> + *	for reading and writing the xattrs as this hook is merely a filter.
>> The return should be -EOPNOTSUPP from security modules that don't
>> support the attribute "name". This will make it possible to support
>> multiple modules that provide attributes. (patches pending)
> Hmm.., Sorry I did not understand this one. 
>
> So all modules will not understand all xattrs. So if they start returning
> -EOPNOTSUPP, then as per current implementation, copy up operation will
> be aborted. 

Yes, the infrastructure code will have to change to deal with the
tri-state returns. That's also true of several other hooks.

> Current implementation relies on that a security module, returns 0 if
> every thing is "name" xattr should be copied up or lsm does not care.
> Negative error code is returned only if something is wrong. Given every
> lsm will not understand/care about all the xattrs, we can't return 
> error code if lsm does not own/understand the "name". In fact
> call_int_hook() will bail out the very first time negative error code
> is returned. 
>
> IOW, current implementation will work with multiple modules providing
> implementation for same hook as long as module returns 0 for the xattrs
> it does not understand. 

There have to be four states. I own this attribute, and want you
to use it. I own this attribute and I want you to ignore it. I don't
own this attribute. I own this attribute and something went terribly
wrong, such as running out of memory.

>
> I guess I am missing something. Can you please elaborate a little more.
>
>> If the only use to which this hook is put is to identify attributes
>> that should be discarded it's unnecessary overhead to pass the
>> parameters that are never used.
> Ok, I will get rid of extra parameters. If somebody needs these, it can
> be added later.
>
> Vivek
>

Re: [PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Vivek Goyal]

On Tue, Jul 05, 2016 at 02:34:43PM -0700, Casey Schaufler wrote:
> On 7/5/2016 2:15 PM, Vivek Goyal wrote:
> > On Tue, Jul 05, 2016 at 01:22:22PM -0700, Casey Schaufler wrote:
> >> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> >>> Provide a security hook which is called when xattrs of a file are being
> >>> copied up. This hook is called once for each xattr and one can either
> >>> accept or reject xattr. If 0 is returned, xattr will be copied up, if 1
> >>> is returned, xattr will not be copied up and if negative error code
> >>> is returned, copy up will be aborted.
> >>>
> >>> In SELinux, label of lower file is not copied up. File already has been
> >>> set with right label at the time of creation and we don't want to overwrite
> >>> that label.
> >>>
> >>> Signed-off-by: David Howells <dhowells@redhat.com>
> >>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >>> ---
> >>>  fs/overlayfs/copy_up.c    |  8 ++++++++
> >>>  include/linux/lsm_hooks.h | 13 +++++++++++++
> >>>  include/linux/security.h  | 10 ++++++++++
> >>>  security/security.c       |  9 +++++++++
> >>>  security/selinux/hooks.c  | 14 ++++++++++++++
> >>>  5 files changed, 54 insertions(+)
> >>>
> >>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> >>> index 90dc362..2c31938 100644
> >>> --- a/fs/overlayfs/copy_up.c
> >>> +++ b/fs/overlayfs/copy_up.c
> >>> @@ -103,6 +103,14 @@ retry:
> >>>  			goto retry;
> >>>  		}
> >>>  
> >>> +		error = security_inode_copy_up_xattr(old, new,
> >>> +						     name, value, size);
> >>> +		if (error < 0)
> >>> +			break;
> >>> +		if (error == 1) {
> >>> +			error = 0;
> >>> +			continue; /* Discard */
> >>> +		}
> >>>  		error = vfs_setxattr(new, name, value, size, 0);
> >>>  		if (error)
> >>>  			break;
> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> >>> index fcde9b9..2a8ee8c 100644
> >>> --- a/include/linux/lsm_hooks.h
> >>> +++ b/include/linux/lsm_hooks.h
> >>> @@ -412,6 +412,16 @@
> >>>   *	@src indicates the union dentry of file that is being copied up.
> >>>   *	@old indicates the pointer to old_cred returned to caller.
> >>>   *	Returns 0 on success or a negative error code on error.
> >>> + * @inode_copy_up_xattr:
> >>> + *	Filter the xattrs being copied up when a unioned file is copied
> >>> + *	up from a lower layer to the union/overlay layer.
> >>> + *	@src indicates the file that is being copied up.
> >>> + *	@dst indicates the file that has being created by the copy up.
> >>> + *	@name indicates the name of the xattr.
> >>> + *	@value, @size indicate the payload of the xattr.
> >>> + *	Returns 0 to accept the xattr, 1 to discard the xattr or a negative
> >>> + *	error code to abort the copy up. Note that the caller is responsible
> >>> + *	for reading and writing the xattrs as this hook is merely a filter.
> >> The return should be -EOPNOTSUPP from security modules that don't
> >> support the attribute "name". This will make it possible to support
> >> multiple modules that provide attributes. (patches pending)
> > Hmm.., Sorry I did not understand this one. 
> >
> > So all modules will not understand all xattrs. So if they start returning
> > -EOPNOTSUPP, then as per current implementation, copy up operation will
> > be aborted. 
> 
> Yes, the infrastructure code will have to change to deal with the
> tri-state returns. That's also true of several other hooks.
> 
> > Current implementation relies on that a security module, returns 0 if
> > every thing is "name" xattr should be copied up or lsm does not care.
> > Negative error code is returned only if something is wrong. Given every
> > lsm will not understand/care about all the xattrs, we can't return 
> > error code if lsm does not own/understand the "name". In fact
> > call_int_hook() will bail out the very first time negative error code
> > is returned. 
> >
> > IOW, current implementation will work with multiple modules providing
> > implementation for same hook as long as module returns 0 for the xattrs
> > it does not understand. 
> 
> There have to be four states. I own this attribute, and want you
> to use it. I own this attribute and I want you to ignore it. I don't
> own this attribute. I own this attribute and something went terribly
> wrong, such as running out of memory.

Ok, so we have 3 states currently and we should have four.

I own this attribute and want you to use it ---> Return 0
I own this attribute and want you to ignore it --> Return 1
I don't own this attribute --> -EOPNOTSUPP
Something went terribly wrong --> Negative error code.

I can modify call_int_hook() to continue if -EOPNOTSUPP is returned. And
if none of the LSMs claimed xattr, caller will get -EOPNOTSUPP.

But what is caller supposed to do with it. There might be xattrs which
are just user data (user.foo) and aborting copying up will not make sense.
That means caller will continue to copy up anyway and treat -EOPNOTSUPP
as success.

IOW, What are we going to gain by introducing this extra state when none
of the LSMs claims to know about the xattr name passed in.

Vivek

Re: [PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Vivek Goyal]

On Wed, Jul 06, 2016 at 01:09:00PM -0400, Vivek Goyal wrote:
> On Tue, Jul 05, 2016 at 02:34:43PM -0700, Casey Schaufler wrote:
> > On 7/5/2016 2:15 PM, Vivek Goyal wrote:
> > > On Tue, Jul 05, 2016 at 01:22:22PM -0700, Casey Schaufler wrote:
> > >> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> > >>> Provide a security hook which is called when xattrs of a file are being
> > >>> copied up. This hook is called once for each xattr and one can either
> > >>> accept or reject xattr. If 0 is returned, xattr will be copied up, if 1
> > >>> is returned, xattr will not be copied up and if negative error code
> > >>> is returned, copy up will be aborted.
> > >>>
> > >>> In SELinux, label of lower file is not copied up. File already has been
> > >>> set with right label at the time of creation and we don't want to overwrite
> > >>> that label.
> > >>>
> > >>> Signed-off-by: David Howells <dhowells@redhat.com>
> > >>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > >>> ---
> > >>>  fs/overlayfs/copy_up.c    |  8 ++++++++
> > >>>  include/linux/lsm_hooks.h | 13 +++++++++++++
> > >>>  include/linux/security.h  | 10 ++++++++++
> > >>>  security/security.c       |  9 +++++++++
> > >>>  security/selinux/hooks.c  | 14 ++++++++++++++
> > >>>  5 files changed, 54 insertions(+)
> > >>>
> > >>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > >>> index 90dc362..2c31938 100644
> > >>> --- a/fs/overlayfs/copy_up.c
> > >>> +++ b/fs/overlayfs/copy_up.c
> > >>> @@ -103,6 +103,14 @@ retry:
> > >>>  			goto retry;
> > >>>  		}
> > >>>  
> > >>> +		error = security_inode_copy_up_xattr(old, new,
> > >>> +						     name, value, size);
> > >>> +		if (error < 0)
> > >>> +			break;
> > >>> +		if (error == 1) {
> > >>> +			error = 0;
> > >>> +			continue; /* Discard */
> > >>> +		}
> > >>>  		error = vfs_setxattr(new, name, value, size, 0);
> > >>>  		if (error)
> > >>>  			break;
> > >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > >>> index fcde9b9..2a8ee8c 100644
> > >>> --- a/include/linux/lsm_hooks.h
> > >>> +++ b/include/linux/lsm_hooks.h
> > >>> @@ -412,6 +412,16 @@
> > >>>   *	@src indicates the union dentry of file that is being copied up.
> > >>>   *	@old indicates the pointer to old_cred returned to caller.
> > >>>   *	Returns 0 on success or a negative error code on error.
> > >>> + * @inode_copy_up_xattr:
> > >>> + *	Filter the xattrs being copied up when a unioned file is copied
> > >>> + *	up from a lower layer to the union/overlay layer.
> > >>> + *	@src indicates the file that is being copied up.
> > >>> + *	@dst indicates the file that has being created by the copy up.
> > >>> + *	@name indicates the name of the xattr.
> > >>> + *	@value, @size indicate the payload of the xattr.
> > >>> + *	Returns 0 to accept the xattr, 1 to discard the xattr or a negative
> > >>> + *	error code to abort the copy up. Note that the caller is responsible
> > >>> + *	for reading and writing the xattrs as this hook is merely a filter.
> > >> The return should be -EOPNOTSUPP from security modules that don't
> > >> support the attribute "name". This will make it possible to support
> > >> multiple modules that provide attributes. (patches pending)
> > > Hmm.., Sorry I did not understand this one. 
> > >
> > > So all modules will not understand all xattrs. So if they start returning
> > > -EOPNOTSUPP, then as per current implementation, copy up operation will
> > > be aborted. 
> > 
> > Yes, the infrastructure code will have to change to deal with the
> > tri-state returns. That's also true of several other hooks.
> > 
> > > Current implementation relies on that a security module, returns 0 if
> > > every thing is "name" xattr should be copied up or lsm does not care.
> > > Negative error code is returned only if something is wrong. Given every
> > > lsm will not understand/care about all the xattrs, we can't return 
> > > error code if lsm does not own/understand the "name". In fact
> > > call_int_hook() will bail out the very first time negative error code
> > > is returned. 
> > >
> > > IOW, current implementation will work with multiple modules providing
> > > implementation for same hook as long as module returns 0 for the xattrs
> > > it does not understand. 
> > 
> > There have to be four states. I own this attribute, and want you
> > to use it. I own this attribute and I want you to ignore it. I don't
> > own this attribute. I own this attribute and something went terribly
> > wrong, such as running out of memory.
> 
> Ok, so we have 3 states currently and we should have four.
> 
> I own this attribute and want you to use it ---> Return 0
> I own this attribute and want you to ignore it --> Return 1
> I don't own this attribute --> -EOPNOTSUPP
> Something went terribly wrong --> Negative error code.
> 
> I can modify call_int_hook() to continue if -EOPNOTSUPP is returned. And
> if none of the LSMs claimed xattr, caller will get -EOPNOTSUPP.
> 
> But what is caller supposed to do with it. There might be xattrs which
> are just user data (user.foo) and aborting copying up will not make sense.
> That means caller will continue to copy up anyway and treat -EOPNOTSUPP
> as success.
> 
> IOW, What are we going to gain by introducing this extra state when none
> of the LSMs claims to know about the xattr name passed in.

Or you are looking for something where caller does not see -EOPNOTSUPP. It
is useful for call_int_hook_foo() where it will return after first LSM
has claimed the "name".

Vivek

Re: [PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Vivek Goyal]

On Wed, Jul 06, 2016 at 01:09:00PM -0400, Vivek Goyal wrote:

[..]
> > >> The return should be -EOPNOTSUPP from security modules that don't
> > >> support the attribute "name". This will make it possible to support
> > >> multiple modules that provide attributes. (patches pending)
> > > Hmm.., Sorry I did not understand this one. 
> > >
> > > So all modules will not understand all xattrs. So if they start returning
> > > -EOPNOTSUPP, then as per current implementation, copy up operation will
> > > be aborted. 
> > 
> > Yes, the infrastructure code will have to change to deal with the
> > tri-state returns. That's also true of several other hooks.
> > 
> > > Current implementation relies on that a security module, returns 0 if
> > > every thing is "name" xattr should be copied up or lsm does not care.
> > > Negative error code is returned only if something is wrong. Given every
> > > lsm will not understand/care about all the xattrs, we can't return 
> > > error code if lsm does not own/understand the "name". In fact
> > > call_int_hook() will bail out the very first time negative error code
> > > is returned. 
> > >
> > > IOW, current implementation will work with multiple modules providing
> > > implementation for same hook as long as module returns 0 for the xattrs
> > > it does not understand. 
> > 
> > There have to be four states. I own this attribute, and want you
> > to use it. I own this attribute and I want you to ignore it. I don't
> > own this attribute. I own this attribute and something went terribly
> > wrong, such as running out of memory.
> 
> Ok, so we have 3 states currently and we should have four.
> 
> I own this attribute and want you to use it ---> Return 0
> I own this attribute and want you to ignore it --> Return 1
> I don't own this attribute --> -EOPNOTSUPP
> Something went terribly wrong --> Negative error code.
> 
> I can modify call_int_hook() to continue if -EOPNOTSUPP is returned. And
> if none of the LSMs claimed xattr, caller will get -EOPNOTSUPP.
> 
> But what is caller supposed to do with it. There might be xattrs which
> are just user data (user.foo) and aborting copying up will not make sense.
> That means caller will continue to copy up anyway and treat -EOPNOTSUPP
> as success.
> 
> IOW, What are we going to gain by introducing this extra state when none
> of the LSMs claims to know about the xattr name passed in.

Looks like behavior of lsm hook inode_getsecurity() seems to be closest to
what you are looking for. If an LSM does not recognize the name, it
returns -EOPNOTSUPP. Also security_inode_getsecurity() returns -EOPNOTSUPP
if none of the lsms know about the "name". Only problem seems be what
if two lsms are stacked and first one does not know about the "name" but
second one does. In that case looks like we will return -EOPNOTSUPP
without calling into second lsm. And that sounds wrong. 

Please find attached the patch. I think this is the change you are looking
for. I have also changed call_int_hook() to continue calling into stacked
hooks if return code is -EOPNOTSUPP.

Does this look good?

Subject: security,overlayfs: Provide security hook for copy up of xattrs for overlay file

Provide a security hook which is called when xattrs of a file are being
copied up. This hook is called once for each xattr and LSM can return 0
to access the xattr, 1 to reject xattr, -EOPNOTSUPP if none of the lsms
claim to know xattr and a negative error code if something went terribly
wrong.

If 0 or -EOPNOTSUPP is returned, xattr will be copied up, if 1 is returned,
xattr will not be copied up and if negative error code is returned, copy up
will be aborted.

In SELinux, label of lower file is not copied up. File already has been
set with right label at the time of creation and we don't want to overwrite
that label. 

Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c    |    7 +++++++
 include/linux/lsm_hooks.h |   10 ++++++++++
 include/linux/security.h  |    6 ++++++
 security/security.c       |   10 +++++++++-
 security/selinux/hooks.c  |   16 ++++++++++++++++
 5 files changed, 48 insertions(+), 1 deletion(-)

Index: rhvgoyal-linux/include/linux/lsm_hooks.h
===================================================================
--- rhvgoyal-linux.orig/include/linux/lsm_hooks.h	2016-07-05 17:29:34.373064081 -0400
+++ rhvgoyal-linux/include/linux/lsm_hooks.h	2016-07-06 14:37:19.507202315 -0400
@@ -412,6 +412,14 @@
  *	@src indicates the union dentry of file that is being copied up.
  *	@old indicates the pointer to old_cred returned to caller.
  *	Returns 0 on success or a negative error code on error.
+ * @inode_copy_up_xattr:
+ *	Filter the xattrs being copied up when a unioned file is copied
+ *	up from a lower layer to the union/overlay layer.
+ *	@name indicates the name of the xattr.
+ *	Returns 0 to accept the xattr, 1 to discard the xattr, -EOPNOTSUPP if
+ *	security module does not know about attribute or a negative error code
+ *	to abort the copy up. Note that the caller is responsible for reading
+ *	and writing the xattrs as this hook is merely a filter.
  *
  * Security hooks for file operations
  *
@@ -1437,6 +1445,7 @@ union security_list_options {
 					size_t buffer_size);
 	void (*inode_getsecid)(struct inode *inode, u32 *secid);
 	int (*inode_copy_up) (struct dentry *src, const struct cred **old);
+	int (*inode_copy_up_xattr) (const char *name);
 
 	int (*file_permission)(struct file *file, int mask);
 	int (*file_alloc_security)(struct file *file);
@@ -1709,6 +1718,7 @@ struct security_hook_heads {
 	struct list_head inode_listsecurity;
 	struct list_head inode_getsecid;
 	struct list_head inode_copy_up;
+	struct list_head inode_copy_up_xattr;
 	struct list_head file_permission;
 	struct list_head file_alloc_security;
 	struct list_head file_free_security;
Index: rhvgoyal-linux/include/linux/security.h
===================================================================
--- rhvgoyal-linux.orig/include/linux/security.h	2016-07-05 17:29:34.375064081 -0400
+++ rhvgoyal-linux/include/linux/security.h	2016-07-06 14:37:19.507202315 -0400
@@ -283,6 +283,7 @@ int security_inode_setsecurity(struct in
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
 int security_inode_copy_up(struct dentry *src, const struct cred **old);
+int security_inode_copy_up_xattr(const char *name);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -765,6 +766,11 @@ static inline int security_inode_copy_up
 	return 0;
 }
 
+static inline int security_inode_copy_up_xattr(const char *name)
+{
+	-EOPNOTSUPP;
+}
+
 static inline int security_file_permission(struct file *file, int mask)
 {
 	return 0;
Index: rhvgoyal-linux/security/security.c
===================================================================
--- rhvgoyal-linux.orig/security/security.c	2016-07-05 17:29:34.376064081 -0400
+++ rhvgoyal-linux/security/security.c	2016-07-06 14:38:24.336202315 -0400
@@ -122,7 +122,7 @@ int __init security_module_enable(const
 								\
 		list_for_each_entry(P, &security_hook_heads.FUNC, list) { \
 			RC = P->hook.FUNC(__VA_ARGS__);		\
-			if (RC != 0)				\
+			if (RC != 0 && RC != IRC)		\
 				break;				\
 		}						\
 	} while (0);						\
@@ -733,6 +733,12 @@ int security_inode_copy_up(struct dentry
 }
 EXPORT_SYMBOL(security_inode_copy_up);
 
+int security_inode_copy_up_xattr(const char *name)
+{
+	return call_int_hook(inode_copy_up_xattr, -EOPNOTSUPP, name);
+}
+EXPORT_SYMBOL(security_inode_copy_up_xattr);
+
 int security_file_permission(struct file *file, int mask)
 {
 	int ret;
@@ -1671,6 +1677,8 @@ struct security_hook_heads security_hook
 		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
 	.inode_copy_up =
 		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
+	.inode_copy_up_xattr =
+		LIST_HEAD_INIT(security_hook_heads.inode_copy_up_xattr),
 	.file_permission =
 		LIST_HEAD_INIT(security_hook_heads.file_permission),
 	.file_alloc_security =
Index: rhvgoyal-linux/fs/overlayfs/copy_up.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/copy_up.c	2016-07-05 17:29:34.376064081 -0400
+++ rhvgoyal-linux/fs/overlayfs/copy_up.c	2016-07-06 13:41:35.565568095 -0400
@@ -103,6 +103,13 @@ retry:
 			goto retry;
 		}
 
+		error = security_inode_copy_up_xattr(name);
+		if (error < 0 && error != -EOPNOTSUPP)
+			break;
+		if (error == 1) {
+			error = 0;
+			continue; /* Discard */
+		}
 		error = vfs_setxattr(new, name, value, size, 0);
 		if (error)
 			break;
Index: rhvgoyal-linux/security/selinux/hooks.c
===================================================================
--- rhvgoyal-linux.orig/security/selinux/hooks.c	2016-07-05 17:29:34.378064081 -0400
+++ rhvgoyal-linux/security/selinux/hooks.c	2016-07-06 14:38:32.059202315 -0400
@@ -3296,6 +3296,21 @@ static int selinux_inode_copy_up(struct
 	return 0;
 }
 
+static int selinux_inode_copy_up_xattr(const char *name)
+{
+	/* The copy_up hook above sets the initial context on an inode, but we
+	 * don't then want to overwrite it by blindly copying all the lower
+	 * xattrs up.  Instead, we have to filter out SELinux-related xattrs.
+	 */
+	if (strcmp(name, XATTR_NAME_SELINUX) == 0)
+		return 1; /* Discard */
+	/*
+	 * Any other attribute apart from SELINUX is not claimed, supported
+	 * by selinux.
+	 */
+	return -EOPNOTSUPP;
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -6083,6 +6098,7 @@ static struct security_hook_list selinux
 	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
 	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
 	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
+	LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
 
 	LSM_HOOK_INIT(file_permission, selinux_file_permission),
 	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),

Re: [PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Casey Schaufler]

On 7/6/2016 12:01 PM, Vivek Goyal wrote:
> On Wed, Jul 06, 2016 at 01:09:00PM -0400, Vivek Goyal wrote:
>
> [..]
>>>>> The return should be -EOPNOTSUPP from security modules that don't
>>>>> support the attribute "name". This will make it possible to support
>>>>> multiple modules that provide attributes. (patches pending)
>>>> Hmm.., Sorry I did not understand this one. 
>>>>
>>>> So all modules will not understand all xattrs. So if they start returning
>>>> -EOPNOTSUPP, then as per current implementation, copy up operation will
>>>> be aborted. 
>>> Yes, the infrastructure code will have to change to deal with the
>>> tri-state returns. That's also true of several other hooks.
>>>
>>>> Current implementation relies on that a security module, returns 0 if
>>>> every thing is "name" xattr should be copied up or lsm does not care.
>>>> Negative error code is returned only if something is wrong. Given every
>>>> lsm will not understand/care about all the xattrs, we can't return 
>>>> error code if lsm does not own/understand the "name". In fact
>>>> call_int_hook() will bail out the very first time negative error code
>>>> is returned. 
>>>>
>>>> IOW, current implementation will work with multiple modules providing
>>>> implementation for same hook as long as module returns 0 for the xattrs
>>>> it does not understand. 
>>> There have to be four states. I own this attribute, and want you
>>> to use it. I own this attribute and I want you to ignore it. I don't
>>> own this attribute. I own this attribute and something went terribly
>>> wrong, such as running out of memory.
>> Ok, so we have 3 states currently and we should have four.
>>
>> I own this attribute and want you to use it ---> Return 0
>> I own this attribute and want you to ignore it --> Return 1
>> I don't own this attribute --> -EOPNOTSUPP
>> Something went terribly wrong --> Negative error code.
>>
>> I can modify call_int_hook() to continue if -EOPNOTSUPP is returned. And
>> if none of the LSMs claimed xattr, caller will get -EOPNOTSUPP.
>>
>> But what is caller supposed to do with it. There might be xattrs which
>> are just user data (user.foo) and aborting copying up will not make sense.
>> That means caller will continue to copy up anyway and treat -EOPNOTSUPP
>> as success.
>>
>> IOW, What are we going to gain by introducing this extra state when none
>> of the LSMs claims to know about the xattr name passed in.
> Looks like behavior of lsm hook inode_getsecurity() seems to be closest to
> what you are looking for. If an LSM does not recognize the name, it
> returns -EOPNOTSUPP. Also security_inode_getsecurity() returns -EOPNOTSUPP
> if none of the lsms know about the "name". Only problem seems be what
> if two lsms are stacked and first one does not know about the "name" but
> second one does. In that case looks like we will return -EOPNOTSUPP
> without calling into second lsm. And that sounds wrong. 

The current infrastructure code won't stack modules that
use the inode security blob, so don't worry about that.
Patches to make that work are coming "real soon". You don't
need to worry about the infrastructure. I'll take care of
that separately.

> Please find attached the patch. I think this is the change you are looking
> for. I have also changed call_int_hook() to continue calling into stacked
> hooks if return code is -EOPNOTSUPP.
>
> Does this look good?
>
> Subject: security,overlayfs: Provide security hook for copy up of xattrs for overlay file
>
> Provide a security hook which is called when xattrs of a file are being
> copied up. This hook is called once for each xattr and LSM can return 0
> to access the xattr, 1 to reject xattr, -EOPNOTSUPP if none of the lsms
> claim to know xattr and a negative error code if something went terribly
> wrong.
>
> If 0 or -EOPNOTSUPP is returned, xattr will be copied up, if 1 is returned,
> xattr will not be copied up and if negative error code is returned, copy up
> will be aborted.
>
> In SELinux, label of lower file is not copied up. File already has been
> set with right label at the time of creation and we don't want to overwrite
> that label. 
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c    |    7 +++++++
>  include/linux/lsm_hooks.h |   10 ++++++++++
>  include/linux/security.h  |    6 ++++++
>  security/security.c       |   10 +++++++++-
>  security/selinux/hooks.c  |   16 ++++++++++++++++
>  5 files changed, 48 insertions(+), 1 deletion(-)
>
> Index: rhvgoyal-linux/include/linux/lsm_hooks.h
> ===================================================================
> --- rhvgoyal-linux.orig/include/linux/lsm_hooks.h	2016-07-05 17:29:34.373064081 -0400
> +++ rhvgoyal-linux/include/linux/lsm_hooks.h	2016-07-06 14:37:19.507202315 -0400
> @@ -412,6 +412,14 @@
>   *	@src indicates the union dentry of file that is being copied up.
>   *	@old indicates the pointer to old_cred returned to caller.
>   *	Returns 0 on success or a negative error code on error.
> + * @inode_copy_up_xattr:
> + *	Filter the xattrs being copied up when a unioned file is copied
> + *	up from a lower layer to the union/overlay layer.
> + *	@name indicates the name of the xattr.
> + *	Returns 0 to accept the xattr, 1 to discard the xattr, -EOPNOTSUPP if
> + *	security module does not know about attribute or a negative error code
> + *	to abort the copy up. Note that the caller is responsible for reading
> + *	and writing the xattrs as this hook is merely a filter.
>   *
>   * Security hooks for file operations
>   *
> @@ -1437,6 +1445,7 @@ union security_list_options {
>  					size_t buffer_size);
>  	void (*inode_getsecid)(struct inode *inode, u32 *secid);
>  	int (*inode_copy_up) (struct dentry *src, const struct cred **old);
> +	int (*inode_copy_up_xattr) (const char *name);
>  
>  	int (*file_permission)(struct file *file, int mask);
>  	int (*file_alloc_security)(struct file *file);
> @@ -1709,6 +1718,7 @@ struct security_hook_heads {
>  	struct list_head inode_listsecurity;
>  	struct list_head inode_getsecid;
>  	struct list_head inode_copy_up;
> +	struct list_head inode_copy_up_xattr;
>  	struct list_head file_permission;
>  	struct list_head file_alloc_security;
>  	struct list_head file_free_security;
> Index: rhvgoyal-linux/include/linux/security.h
> ===================================================================
> --- rhvgoyal-linux.orig/include/linux/security.h	2016-07-05 17:29:34.375064081 -0400
> +++ rhvgoyal-linux/include/linux/security.h	2016-07-06 14:37:19.507202315 -0400
> @@ -283,6 +283,7 @@ int security_inode_setsecurity(struct in
>  int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
>  void security_inode_getsecid(struct inode *inode, u32 *secid);
>  int security_inode_copy_up(struct dentry *src, const struct cred **old);
> +int security_inode_copy_up_xattr(const char *name);
>  int security_file_permission(struct file *file, int mask);
>  int security_file_alloc(struct file *file);
>  void security_file_free(struct file *file);
> @@ -765,6 +766,11 @@ static inline int security_inode_copy_up
>  	return 0;
>  }
>  
> +static inline int security_inode_copy_up_xattr(const char *name)
> +{
> +	-EOPNOTSUPP;
> +}
> +
>  static inline int security_file_permission(struct file *file, int mask)
>  {
>  	return 0;
> Index: rhvgoyal-linux/security/security.c
> ===================================================================
> --- rhvgoyal-linux.orig/security/security.c	2016-07-05 17:29:34.376064081 -0400
> +++ rhvgoyal-linux/security/security.c	2016-07-06 14:38:24.336202315 -0400
> @@ -122,7 +122,7 @@ int __init security_module_enable(const
>  								\
>  		list_for_each_entry(P, &security_hook_heads.FUNC, list) { \
>  			RC = P->hook.FUNC(__VA_ARGS__);		\
> -			if (RC != 0)				\
> +			if (RC != 0 && RC != IRC)		\
>  				break;				\
>  		}						\
>  	} while (0);						\

I'm planning to deal with this case directly, so no, it shouldn't
go into the macro.

> @@ -733,6 +733,12 @@ int security_inode_copy_up(struct dentry
>  }
>  EXPORT_SYMBOL(security_inode_copy_up);
>  
> +int security_inode_copy_up_xattr(const char *name)
> +{
> +	return call_int_hook(inode_copy_up_xattr, -EOPNOTSUPP, name);
> +}
> +EXPORT_SYMBOL(security_inode_copy_up_xattr);
> +
>  int security_file_permission(struct file *file, int mask)
>  {
>  	int ret;
> @@ -1671,6 +1677,8 @@ struct security_hook_heads security_hook
>  		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
>  	.inode_copy_up =
>  		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
> +	.inode_copy_up_xattr =
> +		LIST_HEAD_INIT(security_hook_heads.inode_copy_up_xattr),
>  	.file_permission =
>  		LIST_HEAD_INIT(security_hook_heads.file_permission),
>  	.file_alloc_security =
> Index: rhvgoyal-linux/fs/overlayfs/copy_up.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/copy_up.c	2016-07-05 17:29:34.376064081 -0400
> +++ rhvgoyal-linux/fs/overlayfs/copy_up.c	2016-07-06 13:41:35.565568095 -0400
> @@ -103,6 +103,13 @@ retry:
>  			goto retry;
>  		}
>  
> +		error = security_inode_copy_up_xattr(name);
> +		if (error < 0 && error != -EOPNOTSUPP)
> +			break;
> +		if (error == 1) {
> +			error = 0;
> +			continue; /* Discard */
> +		}

Better. Thanks.

>  		error = vfs_setxattr(new, name, value, size, 0);
>  		if (error)
>  			break;
> Index: rhvgoyal-linux/security/selinux/hooks.c
> ===================================================================
> --- rhvgoyal-linux.orig/security/selinux/hooks.c	2016-07-05 17:29:34.378064081 -0400
> +++ rhvgoyal-linux/security/selinux/hooks.c	2016-07-06 14:38:32.059202315 -0400
> @@ -3296,6 +3296,21 @@ static int selinux_inode_copy_up(struct
>  	return 0;
>  }
>  
> +static int selinux_inode_copy_up_xattr(const char *name)
> +{
> +	/* The copy_up hook above sets the initial context on an inode, but we
> +	 * don't then want to overwrite it by blindly copying all the lower
> +	 * xattrs up.  Instead, we have to filter out SELinux-related xattrs.
> +	 */
> +	if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> +		return 1; /* Discard */
> +	/*
> +	 * Any other attribute apart from SELINUX is not claimed, supported
> +	 * by selinux.
> +	 */
> +	return -EOPNOTSUPP;
> +}
> +
>  /* file security operations */
>  
>  static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -6083,6 +6098,7 @@ static struct security_hook_list selinux
>  	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
>  	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
>  	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
> +	LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
>  
>  	LSM_HOOK_INIT(file_permission, selinux_file_permission),
>  	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
>

Re: [PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Paul Moore]

On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Provide a security hook which is called when xattrs of a file are being
> copied up. This hook is called once for each xattr and one can either
> accept or reject xattr. If 0 is returned, xattr will be copied up, if 1
> is returned, xattr will not be copied up and if negative error code
> is returned, copy up will be aborted.
>
> In SELinux, label of lower file is not copied up. File already has been
> set with right label at the time of creation and we don't want to overwrite
> that label.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c    |  8 ++++++++
>  include/linux/lsm_hooks.h | 13 +++++++++++++
>  include/linux/security.h  | 10 ++++++++++
>  security/security.c       |  9 +++++++++
>  security/selinux/hooks.c  | 14 ++++++++++++++
>  5 files changed, 54 insertions(+)

To continue the earlier feedback about mixing generic LSM hook
definitions with the SELinux specific hook implementations - I prefer
to see patchsets organized in the following manner:

[PATCH 1/X] - add new LSM hooks and the calls from the relevant
subsystems, e.g.
{security/security.c,include/linux/security.h,fs/overlayfs/*}
[PATCH 2/X] - LSM specific hook implementation, e.g. security/selinux/*
[PATCH n/X] - LSM specific hook implementation, e.g. security/smack/*

-- 
paul moore
security @ redhat

Re: [PATCH 2/5] security,overlayfs: Provide security hook for copy up of xattrs for overlay file

[Vivek Goyal]

On Tue, Jul 05, 2016 at 05:45:25PM -0400, Paul Moore wrote:
> On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Provide a security hook which is called when xattrs of a file are being
> > copied up. This hook is called once for each xattr and one can either
> > accept or reject xattr. If 0 is returned, xattr will be copied up, if 1
> > is returned, xattr will not be copied up and if negative error code
> > is returned, copy up will be aborted.
> >
> > In SELinux, label of lower file is not copied up. File already has been
> > set with right label at the time of creation and we don't want to overwrite
> > that label.
> >
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c    |  8 ++++++++
> >  include/linux/lsm_hooks.h | 13 +++++++++++++
> >  include/linux/security.h  | 10 ++++++++++
> >  security/security.c       |  9 +++++++++
> >  security/selinux/hooks.c  | 14 ++++++++++++++
> >  5 files changed, 54 insertions(+)
> 
> To continue the earlier feedback about mixing generic LSM hook
> definitions with the SELinux specific hook implementations - I prefer
> to see patchsets organized in the following manner:
> 
> [PATCH 1/X] - add new LSM hooks and the calls from the relevant
> subsystems, e.g.
> {security/security.c,include/linux/security.h,fs/overlayfs/*}
> [PATCH 2/X] - LSM specific hook implementation, e.g. security/selinux/*
> [PATCH n/X] - LSM specific hook implementation, e.g. security/smack/*

Ok, will do this way.

Vivek

[PATCH 3/5] selinux: Pass security pointer to determine_inode_label()

[Vivek Goyal]

Right now selinux_determine_inode_label() works on security pointer of
current task. Soon I need this to work on a security pointer retrieved
from a set of creds. So start passing in a pointer and caller can decide
where to fetch security pointer from.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 security/selinux/hooks.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c68223c..86a07ed 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1785,13 +1785,13 @@ out:
 /*
  * Determine the label for an inode that might be unioned.
  */
-static int selinux_determine_inode_label(struct inode *dir,
-					 const struct qstr *name,
-					 u16 tclass,
+static int selinux_determine_inode_label(const void *security,
+					 struct inode *dir,
+					 const struct qstr *name, u16 tclass,
 					 u32 *_new_isid)
 {
 	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
-	const struct task_security_struct *tsec = current_security();
+	const struct task_security_struct *tsec = security;
 
 	if ((sbsec->flags & SE_SBINITIALIZED) &&
 	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
@@ -1834,8 +1834,8 @@ static int may_create(struct inode *dir,
 	if (rc)
 		return rc;
 
-	rc = selinux_determine_inode_label(dir, &dentry->d_name, tclass,
-					   &newsid);
+	rc = selinux_determine_inode_label(current_security(), dir,
+					   &dentry->d_name, tclass, &newsid);
 	if (rc)
 		return rc;
 
@@ -2815,7 +2815,8 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
 	u32 newsid;
 	int rc;
 
-	rc = selinux_determine_inode_label(d_inode(dentry->d_parent), name,
+	rc = selinux_determine_inode_label(current_security(),
+					   d_inode(dentry->d_parent), name,
 					   inode_mode_to_security_class(mode),
 					   &newsid);
 	if (rc)
@@ -2840,7 +2841,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 	sid = tsec->sid;
 	newsid = tsec->create_sid;
 
-	rc = selinux_determine_inode_label(
+	rc = selinux_determine_inode_label(current_security(),
 		dir, qstr,
 		inode_mode_to_security_class(inode->i_mode),
 		&newsid);
-- 
2.7.4

Re: [PATCH 3/5] selinux: Pass security pointer to determine_inode_label()

[Casey Schaufler]

On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> Right now selinux_determine_inode_label() works on security pointer of
> current task. Soon I need this to work on a security pointer retrieved
> from a set of creds. So start passing in a pointer and caller can decide
> where to fetch security pointer from.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  security/selinux/hooks.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c68223c..86a07ed 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1785,13 +1785,13 @@ out:
>  /*
>   * Determine the label for an inode that might be unioned.
>   */
> -static int selinux_determine_inode_label(struct inode *dir,
> -					 const struct qstr *name,
> -					 u16 tclass,
> +static int selinux_determine_inode_label(const void *security,

You know the type. Why not use it?

	static int selinux_determine_inode_label(const struct task_security_struct *tsec,

> +					 struct inode *dir,
> +					 const struct qstr *name, u16 tclass,
>  					 u32 *_new_isid)
>  {
>  	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
> -	const struct task_security_struct *tsec = current_security();
> +	const struct task_security_struct *tsec = security;
>  
>  	if ((sbsec->flags & SE_SBINITIALIZED) &&
>  	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> @@ -1834,8 +1834,8 @@ static int may_create(struct inode *dir,
>  	if (rc)
>  		return rc;
>  
> -	rc = selinux_determine_inode_label(dir, &dentry->d_name, tclass,
> -					   &newsid);
> +	rc = selinux_determine_inode_label(current_security(), dir,
> +					   &dentry->d_name, tclass, &newsid);
>  	if (rc)
>  		return rc;
>  
> @@ -2815,7 +2815,8 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
>  	u32 newsid;
>  	int rc;
>  
> -	rc = selinux_determine_inode_label(d_inode(dentry->d_parent), name,
> +	rc = selinux_determine_inode_label(current_security(),
> +					   d_inode(dentry->d_parent), name,
>  					   inode_mode_to_security_class(mode),
>  					   &newsid);
>  	if (rc)
> @@ -2840,7 +2841,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>  	sid = tsec->sid;
>  	newsid = tsec->create_sid;
>  
> -	rc = selinux_determine_inode_label(
> +	rc = selinux_determine_inode_label(current_security(),
>  		dir, qstr,
>  		inode_mode_to_security_class(inode->i_mode),
>  		&newsid);

Re: [PATCH 3/5] selinux: Pass security pointer to determine_inode_label()

[Vivek Goyal]

On Tue, Jul 05, 2016 at 01:25:22PM -0700, Casey Schaufler wrote:
> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> > Right now selinux_determine_inode_label() works on security pointer of
> > current task. Soon I need this to work on a security pointer retrieved
> > from a set of creds. So start passing in a pointer and caller can decide
> > where to fetch security pointer from.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  security/selinux/hooks.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index c68223c..86a07ed 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1785,13 +1785,13 @@ out:
> >  /*
> >   * Determine the label for an inode that might be unioned.
> >   */
> > -static int selinux_determine_inode_label(struct inode *dir,
> > -					 const struct qstr *name,
> > -					 u16 tclass,
> > +static int selinux_determine_inode_label(const void *security,
> 
> You know the type. Why not use it?
> 
> 	static int selinux_determine_inode_label(const struct task_security_struct *tsec,

Will change it. All callers use current_security() to fetch this pointer
and it returns void * and I guess I assumed that compiler will complain
but it does not seem to complain. 

Vivek

[PATCH 4/5] overlayfs: Correctly label newly created file over whiteout

[Vivek Goyal]

During a new file creation, we have switched to mounter's creds for actual
file creation. Also if there is a whiteout present, then file will be
created in work/ dir and then renamed in upper. In none of the cases file
will be labeled as we want it to be.

Newly created file's labels should be such that as if task had created file
in upper/.

This patch introduces a new hook which determines the label dentry will
get if it had been created by task in upper and sets the secid of label
in passed set of creds. Caller makes use of these new creds for file
creation.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/dir.c        | 10 ++++++++++
 include/linux/lsm_hooks.h | 15 +++++++++++++++
 include/linux/security.h  | 12 ++++++++++++
 security/security.c       | 11 +++++++++++
 security/selinux/hooks.c  | 22 ++++++++++++++++++++++
 5 files changed, 70 insertions(+)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 4cdeb74..f94872f 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -433,6 +433,15 @@ static int ovl_create_or_link(struct dentry *dentry, int mode, dev_t rdev,
 	if (override_cred) {
 		override_cred->fsuid = inode->i_uid;
 		override_cred->fsgid = inode->i_gid;
+		if (!hardlink) {
+			err = security_dentry_create_files_as(dentry,
+					mode, &dentry->d_name, old_cred,
+					override_cred);
+			if (err) {
+				put_cred(override_cred);
+				goto out_revert_creds;
+			}
+		}
 		put_cred(override_creds(override_cred));
 		put_cred(override_cred);
 
@@ -443,6 +452,7 @@ static int ovl_create_or_link(struct dentry *dentry, int mode, dev_t rdev,
 			err = ovl_create_over_whiteout(dentry, inode, &stat,
 							link, hardlink);
 	}
+out_revert_creds:
 	revert_creds(old_cred);
 	if (!err) {
 		struct inode *realinode = d_inode(ovl_dentry_upper(dentry));
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 2a8ee8c..cc5099f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -151,6 +151,16 @@
  *	@name name of the last path component used to create file
  *	@ctx pointer to place the pointer to the resulting context in.
  *	@ctxlen point to place the length of the resulting context.
+ * @dentry_create_files_as:
+ *	Compute a context for a dentry as the inode is not yet available
+ *	and set that context in passed in creds so that new files are
+ *	created using that context. Context is calculated using the
+ *	passed in creds and not the creds of the caller.
+ *	@dentry dentry to use in calculating the context.
+ *	@mode mode used to determine resource type.
+ *	@name name of the last path component used to create file
+ * 	@old creds which should be used for context calculation
+ * 	@new creds to modify
  *
  *
  * Security hooks for inode operations.
@@ -1379,6 +1389,10 @@ union security_list_options {
 	int (*dentry_init_security)(struct dentry *dentry, int mode,
 					struct qstr *name, void **ctx,
 					u32 *ctxlen);
+	int (*dentry_create_files_as)(struct dentry *dentry, int mode,
+					struct qstr *name,
+					const struct cred *old,
+					struct cred *new);
 
 
 #ifdef CONFIG_SECURITY_PATH
@@ -1680,6 +1694,7 @@ struct security_hook_heads {
 	struct list_head sb_clone_mnt_opts;
 	struct list_head sb_parse_opts_str;
 	struct list_head dentry_init_security;
+	struct list_head dentry_create_files_as;
 #ifdef CONFIG_SECURITY_PATH
 	struct list_head path_unlink;
 	struct list_head path_mkdir;
diff --git a/include/linux/security.h b/include/linux/security.h
index 663ca15..90713e5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -242,6 +242,10 @@ int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts);
 int security_dentry_init_security(struct dentry *dentry, int mode,
 					struct qstr *name, void **ctx,
 					u32 *ctxlen);
+int security_dentry_create_files_as(struct dentry *dentry, int mode,
+					struct qstr *name,
+					const struct cred *old,
+					struct cred *new);
 
 int security_inode_alloc(struct inode *inode);
 void security_inode_free(struct inode *inode);
@@ -601,6 +605,14 @@ static inline int security_dentry_init_security(struct dentry *dentry,
 	return -EOPNOTSUPP;
 }
 
+static inline int security_dentry_create_files_as(struct dentry *dentry,
+						  int mode, struct qstr *name,
+						  const struct cred *old,
+						  struct cred *new)
+{
+	return 0;
+}
+
 
 static inline int security_inode_init_security(struct inode *inode,
 						struct inode *dir,
diff --git a/security/security.c b/security/security.c
index 87712c6..810642a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -364,6 +364,15 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
 }
 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)
+{
+	return call_int_hook(dentry_create_files_as, 0, dentry, mode,
+				name, old, new);
+}
+EXPORT_SYMBOL(security_dentry_create_files_as);
+
 int security_inode_init_security(struct inode *inode, struct inode *dir,
 				 const struct qstr *qstr,
 				 const initxattrs initxattrs, void *fs_data)
@@ -1615,6 +1624,8 @@ struct security_hook_heads security_hook_heads = {
 		LIST_HEAD_INIT(security_hook_heads.sb_parse_opts_str),
 	.dentry_init_security =
 		LIST_HEAD_INIT(security_hook_heads.dentry_init_security),
+	.dentry_create_files_as =
+		LIST_HEAD_INIT(security_hook_heads.dentry_create_files_as),
 #ifdef CONFIG_SECURITY_PATH
 	.path_unlink =	LIST_HEAD_INIT(security_hook_heads.path_unlink),
 	.path_mkdir =	LIST_HEAD_INIT(security_hook_heads.path_mkdir),
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 86a07ed..7f83ea4 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2825,6 +2825,27 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
 	return security_sid_to_context(newsid, (char **)ctx, ctxlen);
 }
 
+static int selinux_dentry_create_files_as(struct dentry *dentry, int mode,
+					  struct qstr *name,
+					  const struct cred *old,
+					  struct cred *new)
+{
+	u32 newsid;
+	int rc;
+	struct task_security_struct *tsec;
+
+	rc = selinux_determine_inode_label(old->security,
+					   d_inode(dentry->d_parent), name,
+					   inode_mode_to_security_class(mode),
+					   &newsid);
+	if (rc)
+		return rc;
+
+	tsec = new->security;
+	tsec->create_sid = newsid;
+	return 0;
+}
+
 static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 				       const struct qstr *qstr,
 				       const char **name,
@@ -6070,6 +6091,7 @@ static struct security_hook_list selinux_hooks[] = {
 	LSM_HOOK_INIT(sb_parse_opts_str, selinux_parse_opts_str),
 
 	LSM_HOOK_INIT(dentry_init_security, selinux_dentry_init_security),
+	LSM_HOOK_INIT(dentry_create_files_as, selinux_dentry_create_files_as),
 
 	LSM_HOOK_INIT(inode_alloc_security, selinux_inode_alloc_security),
 	LSM_HOOK_INIT(inode_free_security, selinux_inode_free_security),
-- 
2.7.4

[PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

[Vivek Goyal]

ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
if mounter does not have DAC/MAC permission to access getxattr.

Specifically this becomes a problem when selinux is trying to initialize
overlay inode and does ->getxattr(overlay_inode). A task might trigger
initialization of overlay inode and we will access real inode xattr in the
context of mounter and if mounter does not have permissions, then inode
selinux context initialization fails and inode is labeled as unlabeled_t.

One way to deal with it is to let SELinux do getxattr checks both on
overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
to make sure when selinux is trying to initialize label on inode, it does
not go through checks on lower levels and initialization is successful.
And after inode initialization, SELinux will make sure task has getatttr
permission.

One issue with this approach is that it does not work for directories as
d_real() returns the overlay dentry for directories and not the underlying
directory dentry.

Another way to deal with it to introduce another function pointer in
inode_operations, say getxattr_noperm(), which is responsible to get
xattr without any checks. SELinux initialization code will call this
first if it is available on inode. So user space code path will call
->getxattr() and that will go through checks and SELinux internal
initialization will call ->getxattr_noperm() and that will not
go through checks.

For now, I am just converting ovl_getxattr() to get xattr without
any checks on underlying inode. That means it is possible for
a task to get xattr of a file/dir on lower/upper through overlay mount
while it is not possible outside overlay mount.

If this is a major concern, I can look into implementing getxattr_noperm().

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/inode.c  |  7 +------
 fs/xattr.c            | 28 +++++++++++++++++++---------
 include/linux/xattr.h |  1 +
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 36dfd86..a5d3320 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -233,16 +233,11 @@ ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode,
 		     const char *name, void *value, size_t size)
 {
 	struct dentry *realdentry = ovl_dentry_real(dentry);
-	ssize_t sz;
-	const struct cred *old_cred;
 
 	if (ovl_is_private_xattr(name))
 		return -ENODATA;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
-	sz = vfs_getxattr(realdentry, name, value, size);
-	revert_creds(old_cred);
-	return size;
+	return vfs_getxattr_noperm(realdentry, name, value, size);
 }
 
 ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
diff --git a/fs/xattr.c b/fs/xattr.c
index 4beafc4..973e18c 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -209,19 +209,11 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
 }
 
 ssize_t
-vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
+vfs_getxattr_noperm(struct dentry *dentry, const char *name, void *value, size_t size)
 {
 	struct inode *inode = dentry->d_inode;
 	int error;
 
-	error = xattr_permission(inode, name, MAY_READ);
-	if (error)
-		return error;
-
-	error = security_inode_getxattr(dentry, name);
-	if (error)
-		return error;
-
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
 				XATTR_SECURITY_PREFIX_LEN)) {
 		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
@@ -242,6 +234,24 @@ nolsm:
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(vfs_getxattr_noperm);
+
+ssize_t
+vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
+{
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	error = xattr_permission(inode, name, MAY_READ);
+	if (error)
+		return error;
+
+	error = security_inode_getxattr(dentry, name);
+	if (error)
+		return error;
+
+	return vfs_getxattr_noperm(dentry, name, value, size);
+}
 EXPORT_SYMBOL_GPL(vfs_getxattr);
 
 ssize_t
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 94079ba..832a6b6 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -47,6 +47,7 @@ struct xattr {
 
 ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
 ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t);
+ssize_t vfs_getxattr_noperm(struct dentry *, const char *, void *, size_t);
 ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
 int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, size_t, int);
 int vfs_setxattr(struct dentry *, const char *, const void *, size_t, int);
-- 
2.7.4

Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

[Casey Schaufler]

On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
> if mounter does not have DAC/MAC permission to access getxattr.
>
> Specifically this becomes a problem when selinux is trying to initialize
> overlay inode and does ->getxattr(overlay_inode). A task might trigger
> initialization of overlay inode and we will access real inode xattr in the
> context of mounter and if mounter does not have permissions, then inode
> selinux context initialization fails and inode is labeled as unlabeled_t.
>
> One way to deal with it is to let SELinux do getxattr checks both on
> overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
> to make sure when selinux is trying to initialize label on inode, it does
> not go through checks on lower levels and initialization is successful.
> And after inode initialization, SELinux will make sure task has getatttr
> permission.
>
> One issue with this approach is that it does not work for directories as
> d_real() returns the overlay dentry for directories and not the underlying
> directory dentry.
>
> Another way to deal with it to introduce another function pointer in
> inode_operations, say getxattr_noperm(), which is responsible to get
> xattr without any checks. SELinux initialization code will call this
> first if it is available on inode. So user space code path will call
> ->getxattr() and that will go through checks and SELinux internal
> initialization will call ->getxattr_noperm() and that will not
> go through checks.
>
> For now, I am just converting ovl_getxattr() to get xattr without
> any checks on underlying inode. That means it is possible for
> a task to get xattr of a file/dir on lower/upper through overlay mount
> while it is not possible outside overlay mount.
>
> If this is a major concern, I can look into implementing getxattr_noperm().

This is a major concern.

>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/inode.c  |  7 +------
>  fs/xattr.c            | 28 +++++++++++++++++++---------
>  include/linux/xattr.h |  1 +
>  3 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 36dfd86..a5d3320 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -233,16 +233,11 @@ ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode,
>  		     const char *name, void *value, size_t size)
>  {
>  	struct dentry *realdentry = ovl_dentry_real(dentry);
> -	ssize_t sz;
> -	const struct cred *old_cred;
>  
>  	if (ovl_is_private_xattr(name))
>  		return -ENODATA;
>  
> -	old_cred = ovl_override_creds(dentry->d_sb);
> -	sz = vfs_getxattr(realdentry, name, value, size);
> -	revert_creds(old_cred);
> -	return size;
> +	return vfs_getxattr_noperm(realdentry, name, value, size);
>  }
>  
>  ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 4beafc4..973e18c 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -209,19 +209,11 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
>  }
>  
>  ssize_t
> -vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> +vfs_getxattr_noperm(struct dentry *dentry, const char *name, void *value, size_t size)
>  {
>  	struct inode *inode = dentry->d_inode;
>  	int error;
>  
> -	error = xattr_permission(inode, name, MAY_READ);
> -	if (error)
> -		return error;
> -
> -	error = security_inode_getxattr(dentry, name);
> -	if (error)
> -		return error;
> -
>  	if (!strncmp(name, XATTR_SECURITY_PREFIX,
>  				XATTR_SECURITY_PREFIX_LEN)) {
>  		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> @@ -242,6 +234,24 @@ nolsm:
>  
>  	return error;
>  }
> +EXPORT_SYMBOL_GPL(vfs_getxattr_noperm);
> +
> +ssize_t
> +vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	int error;
> +
> +	error = xattr_permission(inode, name, MAY_READ);
> +	if (error)
> +		return error;
> +
> +	error = security_inode_getxattr(dentry, name);
> +	if (error)
> +		return error;
> +
> +	return vfs_getxattr_noperm(dentry, name, value, size);
> +}
>  EXPORT_SYMBOL_GPL(vfs_getxattr);
>  
>  ssize_t
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index 94079ba..832a6b6 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -47,6 +47,7 @@ struct xattr {
>  
>  ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
>  ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t);
> +ssize_t vfs_getxattr_noperm(struct dentry *, const char *, void *, size_t);
>  ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
>  int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, size_t, int);
>  int vfs_setxattr(struct dentry *, const char *, const void *, size_t, int);

Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

[Vivek Goyal]

On Tue, Jul 05, 2016 at 01:29:39PM -0700, Casey Schaufler wrote:
> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> > ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
> > if mounter does not have DAC/MAC permission to access getxattr.
> >
> > Specifically this becomes a problem when selinux is trying to initialize
> > overlay inode and does ->getxattr(overlay_inode). A task might trigger
> > initialization of overlay inode and we will access real inode xattr in the
> > context of mounter and if mounter does not have permissions, then inode
> > selinux context initialization fails and inode is labeled as unlabeled_t.
> >
> > One way to deal with it is to let SELinux do getxattr checks both on
> > overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
> > to make sure when selinux is trying to initialize label on inode, it does
> > not go through checks on lower levels and initialization is successful.
> > And after inode initialization, SELinux will make sure task has getatttr
> > permission.
> >
> > One issue with this approach is that it does not work for directories as
> > d_real() returns the overlay dentry for directories and not the underlying
> > directory dentry.
> >
> > Another way to deal with it to introduce another function pointer in
> > inode_operations, say getxattr_noperm(), which is responsible to get
> > xattr without any checks. SELinux initialization code will call this
> > first if it is available on inode. So user space code path will call
> > ->getxattr() and that will go through checks and SELinux internal
> > initialization will call ->getxattr_noperm() and that will not
> > go through checks.
> >
> > For now, I am just converting ovl_getxattr() to get xattr without
> > any checks on underlying inode. That means it is possible for
> > a task to get xattr of a file/dir on lower/upper through overlay mount
> > while it is not possible outside overlay mount.
> >
> > If this is a major concern, I can look into implementing getxattr_noperm().
> 
> This is a major concern.

Hmm.., In that case I will write patch to provide another inode operation
getxattr_noperm() and a wrapper which falls back to getxattr() if noperm
variant is not defined. That should take care of this issue.

Thanks
Vivek
> 
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/inode.c  |  7 +------
> >  fs/xattr.c            | 28 +++++++++++++++++++---------
> >  include/linux/xattr.h |  1 +
> >  3 files changed, 21 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 36dfd86..a5d3320 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -233,16 +233,11 @@ ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode,
> >  		     const char *name, void *value, size_t size)
> >  {
> >  	struct dentry *realdentry = ovl_dentry_real(dentry);
> > -	ssize_t sz;
> > -	const struct cred *old_cred;
> >  
> >  	if (ovl_is_private_xattr(name))
> >  		return -ENODATA;
> >  
> > -	old_cred = ovl_override_creds(dentry->d_sb);
> > -	sz = vfs_getxattr(realdentry, name, value, size);
> > -	revert_creds(old_cred);
> > -	return size;
> > +	return vfs_getxattr_noperm(realdentry, name, value, size);
> >  }
> >  
> >  ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index 4beafc4..973e18c 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -209,19 +209,11 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
> >  }
> >  
> >  ssize_t
> > -vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> > +vfs_getxattr_noperm(struct dentry *dentry, const char *name, void *value, size_t size)
> >  {
> >  	struct inode *inode = dentry->d_inode;
> >  	int error;
> >  
> > -	error = xattr_permission(inode, name, MAY_READ);
> > -	if (error)
> > -		return error;
> > -
> > -	error = security_inode_getxattr(dentry, name);
> > -	if (error)
> > -		return error;
> > -
> >  	if (!strncmp(name, XATTR_SECURITY_PREFIX,
> >  				XATTR_SECURITY_PREFIX_LEN)) {
> >  		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> > @@ -242,6 +234,24 @@ nolsm:
> >  
> >  	return error;
> >  }
> > +EXPORT_SYMBOL_GPL(vfs_getxattr_noperm);
> > +
> > +ssize_t
> > +vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> > +{
> > +	struct inode *inode = dentry->d_inode;
> > +	int error;
> > +
> > +	error = xattr_permission(inode, name, MAY_READ);
> > +	if (error)
> > +		return error;
> > +
> > +	error = security_inode_getxattr(dentry, name);
> > +	if (error)
> > +		return error;
> > +
> > +	return vfs_getxattr_noperm(dentry, name, value, size);
> > +}
> >  EXPORT_SYMBOL_GPL(vfs_getxattr);
> >  
> >  ssize_t
> > diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> > index 94079ba..832a6b6 100644
> > --- a/include/linux/xattr.h
> > +++ b/include/linux/xattr.h
> > @@ -47,6 +47,7 @@ struct xattr {
> >  
> >  ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
> >  ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t);
> > +ssize_t vfs_getxattr_noperm(struct dentry *, const char *, void *, size_t);
> >  ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
> >  int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, size_t, int);
> >  int vfs_setxattr(struct dentry *, const char *, const void *, size_t, int);

Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

[Miklos Szeredi]

On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Jul 05, 2016 at 01:29:39PM -0700, Casey Schaufler wrote:
>> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
>> > ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
>> > if mounter does not have DAC/MAC permission to access getxattr.
>> >
>> > Specifically this becomes a problem when selinux is trying to initialize
>> > overlay inode and does ->getxattr(overlay_inode). A task might trigger
>> > initialization of overlay inode and we will access real inode xattr in the
>> > context of mounter and if mounter does not have permissions, then inode
>> > selinux context initialization fails and inode is labeled as unlabeled_t.
>> >
>> > One way to deal with it is to let SELinux do getxattr checks both on
>> > overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
>> > to make sure when selinux is trying to initialize label on inode, it does
>> > not go through checks on lower levels and initialization is successful.
>> > And after inode initialization, SELinux will make sure task has getatttr
>> > permission.
>> >
>> > One issue with this approach is that it does not work for directories as
>> > d_real() returns the overlay dentry for directories and not the underlying
>> > directory dentry.
>> >
>> > Another way to deal with it to introduce another function pointer in
>> > inode_operations, say getxattr_noperm(), which is responsible to get
>> > xattr without any checks. SELinux initialization code will call this
>> > first if it is available on inode. So user space code path will call
>> > ->getxattr() and that will go through checks and SELinux internal
>> > initialization will call ->getxattr_noperm() and that will not
>> > go through checks.
>> >
>> > For now, I am just converting ovl_getxattr() to get xattr without
>> > any checks on underlying inode. That means it is possible for
>> > a task to get xattr of a file/dir on lower/upper through overlay mount
>> > while it is not possible outside overlay mount.
>> >
>> > If this is a major concern, I can look into implementing getxattr_noperm().
>>
>> This is a major concern.
>
> Hmm.., In that case I will write patch to provide another inode operation
> getxattr_noperm() and a wrapper which falls back to getxattr() if noperm
> variant is not defined. That should take care of this issue.

That's not going to fly.  A slighly better, but still quite ugly
solution would be to add a "flags" arg to the current ->getxattr()
callback indicating whether the caller wants permission checking
inside the call or not.

But we already have the current->creds.  Can't that be used to control
the permission checking done by the callback?

Thanks,
Miklos

Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

[Vivek Goyal]

On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote:
> On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Tue, Jul 05, 2016 at 01:29:39PM -0700, Casey Schaufler wrote:
> >> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> >> > ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
> >> > if mounter does not have DAC/MAC permission to access getxattr.
> >> >
> >> > Specifically this becomes a problem when selinux is trying to initialize
> >> > overlay inode and does ->getxattr(overlay_inode). A task might trigger
> >> > initialization of overlay inode and we will access real inode xattr in the
> >> > context of mounter and if mounter does not have permissions, then inode
> >> > selinux context initialization fails and inode is labeled as unlabeled_t.
> >> >
> >> > One way to deal with it is to let SELinux do getxattr checks both on
> >> > overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
> >> > to make sure when selinux is trying to initialize label on inode, it does
> >> > not go through checks on lower levels and initialization is successful.
> >> > And after inode initialization, SELinux will make sure task has getatttr
> >> > permission.
> >> >
> >> > One issue with this approach is that it does not work for directories as
> >> > d_real() returns the overlay dentry for directories and not the underlying
> >> > directory dentry.
> >> >
> >> > Another way to deal with it to introduce another function pointer in
> >> > inode_operations, say getxattr_noperm(), which is responsible to get
> >> > xattr without any checks. SELinux initialization code will call this
> >> > first if it is available on inode. So user space code path will call
> >> > ->getxattr() and that will go through checks and SELinux internal
> >> > initialization will call ->getxattr_noperm() and that will not
> >> > go through checks.
> >> >
> >> > For now, I am just converting ovl_getxattr() to get xattr without
> >> > any checks on underlying inode. That means it is possible for
> >> > a task to get xattr of a file/dir on lower/upper through overlay mount
> >> > while it is not possible outside overlay mount.
> >> >
> >> > If this is a major concern, I can look into implementing getxattr_noperm().
> >>
> >> This is a major concern.
> >
> > Hmm.., In that case I will write patch to provide another inode operation
> > getxattr_noperm() and a wrapper which falls back to getxattr() if noperm
> > variant is not defined. That should take care of this issue.
> 
> That's not going to fly.  A slighly better, but still quite ugly
> solution would be to add a "flags" arg to the current ->getxattr()
> callback indicating whether the caller wants permission checking
> inside the call or not.
> 

Ok, will try that.

> But we already have the current->creds.  Can't that be used to control
> the permission checking done by the callback?

Sorry, did not get how to use current->creds to control permission
checking.

Vivek

Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

[Miklos Szeredi]

On Wed, Jul 6, 2016 at 12:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote:
>> On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Tue, Jul 05, 2016 at 01:29:39PM -0700, Casey Schaufler wrote:
>> >> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
>> >> > ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
>> >> > if mounter does not have DAC/MAC permission to access getxattr.
>> >> >
>> >> > Specifically this becomes a problem when selinux is trying to initialize
>> >> > overlay inode and does ->getxattr(overlay_inode). A task might trigger
>> >> > initialization of overlay inode and we will access real inode xattr in the
>> >> > context of mounter and if mounter does not have permissions, then inode
>> >> > selinux context initialization fails and inode is labeled as unlabeled_t.
>> >> >
>> >> > One way to deal with it is to let SELinux do getxattr checks both on
>> >> > overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
>> >> > to make sure when selinux is trying to initialize label on inode, it does
>> >> > not go through checks on lower levels and initialization is successful.
>> >> > And after inode initialization, SELinux will make sure task has getatttr
>> >> > permission.
>> >> >
>> >> > One issue with this approach is that it does not work for directories as
>> >> > d_real() returns the overlay dentry for directories and not the underlying
>> >> > directory dentry.
>> >> >
>> >> > Another way to deal with it to introduce another function pointer in
>> >> > inode_operations, say getxattr_noperm(), which is responsible to get
>> >> > xattr without any checks. SELinux initialization code will call this
>> >> > first if it is available on inode. So user space code path will call
>> >> > ->getxattr() and that will go through checks and SELinux internal
>> >> > initialization will call ->getxattr_noperm() and that will not
>> >> > go through checks.
>> >> >
>> >> > For now, I am just converting ovl_getxattr() to get xattr without
>> >> > any checks on underlying inode. That means it is possible for
>> >> > a task to get xattr of a file/dir on lower/upper through overlay mount
>> >> > while it is not possible outside overlay mount.
>> >> >
>> >> > If this is a major concern, I can look into implementing getxattr_noperm().
>> >>
>> >> This is a major concern.
>> >
>> > Hmm.., In that case I will write patch to provide another inode operation
>> > getxattr_noperm() and a wrapper which falls back to getxattr() if noperm
>> > variant is not defined. That should take care of this issue.
>>
>> That's not going to fly.  A slighly better, but still quite ugly
>> solution would be to add a "flags" arg to the current ->getxattr()
>> callback indicating whether the caller wants permission checking
>> inside the call or not.
>>
>
> Ok, will try that.
>
>> But we already have the current->creds.  Can't that be used to control
>> the permission checking done by the callback?
>
> Sorry, did not get how to use current->creds to control permission
> checking.

I'm not sure about the details either.  But current->creds *is* the
context provided for the VFS and filesystems to check permissions.  It
might make sense to use that to indicate to overlayfs that permission
should not be checked.

Thanks,
Miklos

Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

[Vivek Goyal]

On Wed, Jul 06, 2016 at 04:58:37PM +0200, Miklos Szeredi wrote:
> On Wed, Jul 6, 2016 at 12:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote:
> >> On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Tue, Jul 05, 2016 at 01:29:39PM -0700, Casey Schaufler wrote:
> >> >> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> >> >> > ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
> >> >> > if mounter does not have DAC/MAC permission to access getxattr.
> >> >> >
> >> >> > Specifically this becomes a problem when selinux is trying to initialize
> >> >> > overlay inode and does ->getxattr(overlay_inode). A task might trigger
> >> >> > initialization of overlay inode and we will access real inode xattr in the
> >> >> > context of mounter and if mounter does not have permissions, then inode
> >> >> > selinux context initialization fails and inode is labeled as unlabeled_t.
> >> >> >
> >> >> > One way to deal with it is to let SELinux do getxattr checks both on
> >> >> > overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
> >> >> > to make sure when selinux is trying to initialize label on inode, it does
> >> >> > not go through checks on lower levels and initialization is successful.
> >> >> > And after inode initialization, SELinux will make sure task has getatttr
> >> >> > permission.
> >> >> >
> >> >> > One issue with this approach is that it does not work for directories as
> >> >> > d_real() returns the overlay dentry for directories and not the underlying
> >> >> > directory dentry.
> >> >> >
> >> >> > Another way to deal with it to introduce another function pointer in
> >> >> > inode_operations, say getxattr_noperm(), which is responsible to get
> >> >> > xattr without any checks. SELinux initialization code will call this
> >> >> > first if it is available on inode. So user space code path will call
> >> >> > ->getxattr() and that will go through checks and SELinux internal
> >> >> > initialization will call ->getxattr_noperm() and that will not
> >> >> > go through checks.
> >> >> >
> >> >> > For now, I am just converting ovl_getxattr() to get xattr without
> >> >> > any checks on underlying inode. That means it is possible for
> >> >> > a task to get xattr of a file/dir on lower/upper through overlay mount
> >> >> > while it is not possible outside overlay mount.
> >> >> >
> >> >> > If this is a major concern, I can look into implementing getxattr_noperm().
> >> >>
> >> >> This is a major concern.
> >> >
> >> > Hmm.., In that case I will write patch to provide another inode operation
> >> > getxattr_noperm() and a wrapper which falls back to getxattr() if noperm
> >> > variant is not defined. That should take care of this issue.
> >>
> >> That's not going to fly.  A slighly better, but still quite ugly
> >> solution would be to add a "flags" arg to the current ->getxattr()
> >> callback indicating whether the caller wants permission checking
> >> inside the call or not.
> >>
> >
> > Ok, will try that.
> >
> >> But we already have the current->creds.  Can't that be used to control
> >> the permission checking done by the callback?
> >
> > Sorry, did not get how to use current->creds to control permission
> > checking.
> 
> I'm not sure about the details either.  But current->creds *is* the
> context provided for the VFS and filesystems to check permissions.  It
> might make sense to use that to indicate to overlayfs that permission
> should not be checked.

That sounds like raising capabilities of task temporarily to do
getxattr(). But AFAIK, there is no cap which will override SELinux checks.

I am taking a step back re-thinking about the problem.

- For context mounts this is not a problem at all as overlay inode will
  get its label from context= mount option and we will not call into
  ovl_getxattr().

- For non-context mounts this is a problem only if mounter is not
  privileged enough to do getattr. And that's not going to be a common
  case either.

IOW, this does not look like a common case. And if getxattr() fails,
SELinux already seems to mark inode as unlabeled_t. And my understanding
is that task can't access unlabeled_t anyway, so there is no information
leak.

So for now, why not leave it as it is. Only side affect I seem to see 
is following warnings on console.

SELinux: inode_doinit_with_dentry:  getxattr returned 13 for dev=overlay ino=29147

This is for information purposes only and given getxattr() can fail in
stacked configuration, I think we can change this to KERN_DEBUG instead
of KERN_WARNING.

Thanks
Vivek

Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

[Miklos Szeredi]

On Thu, Jul 7, 2016 at 8:35 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Jul 06, 2016 at 04:58:37PM +0200, Miklos Szeredi wrote:
>> On Wed, Jul 6, 2016 at 12:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote:
>> >> On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > On Tue, Jul 05, 2016 at 01:29:39PM -0700, Casey Schaufler wrote:
>> >> >> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
>> >> >> > ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
>> >> >> > if mounter does not have DAC/MAC permission to access getxattr.
>> >> >> >
>> >> >> > Specifically this becomes a problem when selinux is trying to initialize
>> >> >> > overlay inode and does ->getxattr(overlay_inode). A task might trigger
>> >> >> > initialization of overlay inode and we will access real inode xattr in the
>> >> >> > context of mounter and if mounter does not have permissions, then inode
>> >> >> > selinux context initialization fails and inode is labeled as unlabeled_t.
>> >> >> >
>> >> >> > One way to deal with it is to let SELinux do getxattr checks both on
>> >> >> > overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
>> >> >> > to make sure when selinux is trying to initialize label on inode, it does
>> >> >> > not go through checks on lower levels and initialization is successful.
>> >> >> > And after inode initialization, SELinux will make sure task has getatttr
>> >> >> > permission.
>> >> >> >
>> >> >> > One issue with this approach is that it does not work for directories as
>> >> >> > d_real() returns the overlay dentry for directories and not the underlying
>> >> >> > directory dentry.
>> >> >> >
>> >> >> > Another way to deal with it to introduce another function pointer in
>> >> >> > inode_operations, say getxattr_noperm(), which is responsible to get
>> >> >> > xattr without any checks. SELinux initialization code will call this
>> >> >> > first if it is available on inode. So user space code path will call
>> >> >> > ->getxattr() and that will go through checks and SELinux internal
>> >> >> > initialization will call ->getxattr_noperm() and that will not
>> >> >> > go through checks.
>> >> >> >
>> >> >> > For now, I am just converting ovl_getxattr() to get xattr without
>> >> >> > any checks on underlying inode. That means it is possible for
>> >> >> > a task to get xattr of a file/dir on lower/upper through overlay mount
>> >> >> > while it is not possible outside overlay mount.
>> >> >> >
>> >> >> > If this is a major concern, I can look into implementing getxattr_noperm().
>> >> >>
>> >> >> This is a major concern.
>> >> >
>> >> > Hmm.., In that case I will write patch to provide another inode operation
>> >> > getxattr_noperm() and a wrapper which falls back to getxattr() if noperm
>> >> > variant is not defined. That should take care of this issue.
>> >>
>> >> That's not going to fly.  A slighly better, but still quite ugly
>> >> solution would be to add a "flags" arg to the current ->getxattr()
>> >> callback indicating whether the caller wants permission checking
>> >> inside the call or not.
>> >>
>> >
>> > Ok, will try that.
>> >
>> >> But we already have the current->creds.  Can't that be used to control
>> >> the permission checking done by the callback?
>> >
>> > Sorry, did not get how to use current->creds to control permission
>> > checking.
>>
>> I'm not sure about the details either.  But current->creds *is* the
>> context provided for the VFS and filesystems to check permissions.  It
>> might make sense to use that to indicate to overlayfs that permission
>> should not be checked.
>
> That sounds like raising capabilities of task temporarily to do
> getxattr(). But AFAIK, there is no cap which will override SELinux checks.

So a new capability can be invented for this purpose?

> I am taking a step back re-thinking about the problem.
>
> - For context mounts this is not a problem at all as overlay inode will
>   get its label from context= mount option and we will not call into
>   ovl_getxattr().
>
> - For non-context mounts this is a problem only if mounter is not
>   privileged enough to do getattr. And that's not going to be a common
>   case either.
>
> IOW, this does not look like a common case. And if getxattr() fails,
> SELinux already seems to mark inode as unlabeled_t. And my understanding
> is that task can't access unlabeled_t anyway, so there is no information
> leak.
>
> So for now, why not leave it as it is. Only side affect I seem to see
> is following warnings on console.
>
> SELinux: inode_doinit_with_dentry:  getxattr returned 13 for dev=overlay ino=29147
>
> This is for information purposes only and given getxattr() can fail in
> stacked configuration, I think we can change this to KERN_DEBUG instead
> of KERN_WARNING.

I'm fine with this as well.

Thanks,
Miklos

Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

[Casey Schaufler]

On 7/8/2016 12:06 AM, Miklos Szeredi wrote:
> On Thu, Jul 7, 2016 at 8:35 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Wed, Jul 06, 2016 at 04:58:37PM +0200, Miklos Szeredi wrote:
>>> On Wed, Jul 6, 2016 at 12:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>> On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote:
>>>>> On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>>>> On Tue, Jul 05, 2016 at 01:29:39PM -0700, Casey Schaufler wrote:
>>>>>>> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
>>>>>>>> ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
>>>>>>>> if mounter does not have DAC/MAC permission to access getxattr.
>>>>>>>>
>>>>>>>> Specifically this becomes a problem when selinux is trying to initialize
>>>>>>>> overlay inode and does ->getxattr(overlay_inode). A task might trigger
>>>>>>>> initialization of overlay inode and we will access real inode xattr in the
>>>>>>>> context of mounter and if mounter does not have permissions, then inode
>>>>>>>> selinux context initialization fails and inode is labeled as unlabeled_t.
>>>>>>>>
>>>>>>>> One way to deal with it is to let SELinux do getxattr checks both on
>>>>>>>> overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
>>>>>>>> to make sure when selinux is trying to initialize label on inode, it does
>>>>>>>> not go through checks on lower levels and initialization is successful.
>>>>>>>> And after inode initialization, SELinux will make sure task has getatttr
>>>>>>>> permission.
>>>>>>>>
>>>>>>>> One issue with this approach is that it does not work for directories as
>>>>>>>> d_real() returns the overlay dentry for directories and not the underlying
>>>>>>>> directory dentry.
>>>>>>>>
>>>>>>>> Another way to deal with it to introduce another function pointer in
>>>>>>>> inode_operations, say getxattr_noperm(), which is responsible to get
>>>>>>>> xattr without any checks. SELinux initialization code will call this
>>>>>>>> first if it is available on inode. So user space code path will call
>>>>>>>> ->getxattr() and that will go through checks and SELinux internal
>>>>>>>> initialization will call ->getxattr_noperm() and that will not
>>>>>>>> go through checks.
>>>>>>>>
>>>>>>>> For now, I am just converting ovl_getxattr() to get xattr without
>>>>>>>> any checks on underlying inode. That means it is possible for
>>>>>>>> a task to get xattr of a file/dir on lower/upper through overlay mount
>>>>>>>> while it is not possible outside overlay mount.
>>>>>>>>
>>>>>>>> If this is a major concern, I can look into implementing getxattr_noperm().
>>>>>>> This is a major concern.
>>>>>> Hmm.., In that case I will write patch to provide another inode operation
>>>>>> getxattr_noperm() and a wrapper which falls back to getxattr() if noperm
>>>>>> variant is not defined. That should take care of this issue.
>>>>> That's not going to fly.  A slighly better, but still quite ugly
>>>>> solution would be to add a "flags" arg to the current ->getxattr()
>>>>> callback indicating whether the caller wants permission checking
>>>>> inside the call or not.
>>>>>
>>>> Ok, will try that.
>>>>
>>>>> But we already have the current->creds.  Can't that be used to control
>>>>> the permission checking done by the callback?
>>>> Sorry, did not get how to use current->creds to control permission
>>>> checking.
>>> I'm not sure about the details either.  But current->creds *is* the
>>> context provided for the VFS and filesystems to check permissions.  It
>>> might make sense to use that to indicate to overlayfs that permission
>>> should not be checked.
>> That sounds like raising capabilities of task temporarily to do
>> getxattr(). But AFAIK, there is no cap which will override SELinux checks.
> So a new capability can be invented for this purpose?

SELinux does not use capabilities as an override mechanism.
The capability you would want if it did is CAP_MAC_OVERRIDE,
which is used by Smack. 

>
>> I am taking a step back re-thinking about the problem.
>>
>> - For context mounts this is not a problem at all as overlay inode will
>>   get its label from context= mount option and we will not call into
>>   ovl_getxattr().
>>
>> - For non-context mounts this is a problem only if mounter is not
>>   privileged enough to do getattr. And that's not going to be a common
>>   case either.
>>
>> IOW, this does not look like a common case. And if getxattr() fails,
>> SELinux already seems to mark inode as unlabeled_t. And my understanding
>> is that task can't access unlabeled_t anyway, so there is no information
>> leak.
>>
>> So for now, why not leave it as it is. Only side affect I seem to see
>> is following warnings on console.
>>
>> SELinux: inode_doinit_with_dentry:  getxattr returned 13 for dev=overlay ino=29147
>>
>> This is for information purposes only and given getxattr() can fail in
>> stacked configuration, I think we can change this to KERN_DEBUG instead
>> of KERN_WARNING.
> I'm fine with this as well.
>
> Thanks,
> Miklos
>