Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1437138 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files Vivek Goyal <vgoyal@redhat.com> - 2016-07-05 18:00 +0200
    Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files kbuild test robot <lkp@intel.com> - 2016-07-05 19:00 +0200
      Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Vivek Goyal <vgoyal@redhat.com> - 2016-07-05 19:50 +0200
    Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files kbuild test robot <lkp@intel.com> - 2016-07-05 19:30 +0200
    Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Casey Schaufler <casey@schaufler-ca.com> - 2016-07-05 21:40 +0200
      Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Vivek Goyal <vgoyal@redhat.com> - 2016-07-05 22:50 +0200
      Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Vivek Goyal <vgoyal@redhat.com> - 2016-07-07 22:50 +0200
        Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Casey Schaufler <casey@schaufler-ca.com> - 2016-07-07 23:50 +0200
          Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Miklos Szeredi <miklos@szeredi.hu> - 2016-07-08 09:30 +0200
            Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Vivek Goyal <vgoyal@redhat.com> - 2016-07-08 14:50 +0200
              Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Vivek Goyal <vgoyal@redhat.com> - 2016-07-08 15:50 +0200
                Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Casey Schaufler <casey@schaufler-ca.com> - 2016-07-08 17:40 +0200
    Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Paul Moore <pmoore@redhat.com> - 2016-07-05 23:40 +0200
      Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Vivek Goyal <vgoyal@redhat.com> - 2016-07-06 00:00 +0200
        Re: [PATCH 1/5] security, overlayfs: provide copy up security hook  for unioned files Paul Moore <pmoore@redhat.com> - 2016-07-06 00:10 +0200

#1437138 — [PATCH 1/5] 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  |  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

[toc] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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
>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web