linux-next: manual merge of the security tree with the vfs tree

linux-next: manual merge of the security tree with the vfs tree

[Stephen Rothwell]

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

Hi James,

Today's linux-next merge of the security tree got a conflict in
init/main.c between commit 87fb64a6c1f7 ("take the targets
of /proc/*/ns/* symlinks to separate fs") from the vfs tree and commit
c9cd2ce2bc63 ("integrity: provide a hook to load keys when rootfs is
ready") from the security tree.

I fixed it up (see below) and can carry the fix as necessary (no action
is required).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc init/main.c
index 40240c8e31e8,9fc795fe4b3b..000000000000
--- a/init/main.c
+++ b/init/main.c
@@@ -78,7 -78,7 +78,8 @@@
  #include <linux/context_tracking.h>
  #include <linux/random.h>
  #include <linux/list.h>
 +#include <linux/proc_ns.h>
+ #include <linux/integrity.h>
  
  #include <asm/io.h>
  #include <asm/bugs.h>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

linux-next: manual merge of the security tree with the vfs tree

[Stephen Rothwell]

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

Hi James,

Today's linux-next merge of the security tree got a conflict in
security/capability.c between commits 37882db0546c ("SECURITY: remove
nameidata arg from inode_follow_link") and bda0be7ad994 ("security:
make inode_follow_link RCU-walk aware") from the vfs tree and commit
1ddd3b4e07a4 ("LSM: Remove unused capability.c") from the security tree.

I fixed it up (the latter removed the file, so I did that) and can
carry the fix as necessary (no action is required).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

linux-next: manual merge of the security tree with the vfs tree

[Stephen Rothwell]

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

Hi James,

Today's linux-next merge of the security tree got a conflict in
include/linux/security.h and security/security.c between commits
37882db0546c ("SECURITY: remove nameidata arg from inode_follow_link")
and bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
from the vfs tree and commits 3c4ed7bdf599 ("LSM: Split security.h")
and 346033a28fb1 ("LSM: Remove a comment from security.h") from the
security tree.

I fixed it up (see below, plus I added the following merge fix patch)
and can carry the fix as necessary (no action is required).

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Wed, 13 May 2015 14:29:17 +1000
Subject: [PATCH] LSM: merge fix for follow_link API changes

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 include/linux/lsm_hooks.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index f014f2596e22..9429f054c323 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -315,7 +315,8 @@
  * @inode_follow_link:
  *	Check permission to follow a symbolic link when looking up a pathname.
  *	@dentry contains the dentry structure for the link.
- *	@nd contains the nameidata structure for the parent directory.
+ *	@inode contains the inode, which itself is not stable in RCU-walk
+ *	@rcu indicates whether we are in RCU-walk mode.
  *	Return 0 if permission is granted.
  * @inode_permission:
  *	Check permission before accessing an inode.  This hook is called by the
@@ -1397,7 +1398,8 @@ union security_list_options {
 				struct inode *new_dir,
 				struct dentry *new_dentry);
 	int (*inode_readlink)(struct dentry *dentry);
-	int (*inode_follow_link)(struct dentry *dentry, struct nameidata *nd);
+	int (*inode_follow_link)(struct dentry *dentry, struct inode *inode,
+				 bool rcu);
 	int (*inode_permission)(struct inode *inode, int mask);
 	int (*inode_setattr)(struct dentry *dentry, struct iattr *attr);
 	int (*inode_getattr)(const struct path *path);
-- 
2.1.4

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc security/security.c
index 04c8feca081a,bd4c5f6a5b78..000000000000
--- a/security/security.c
+++ b/security/security.c
@@@ -578,15 -586,14 +586,15 @@@ int security_inode_readlink(struct dent
  {
  	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
  		return 0;
- 	return security_ops->inode_readlink(dentry);
+ 	return call_int_hook(inode_readlink, 0, dentry);
  }
  
 -int security_inode_follow_link(struct dentry *dentry, struct nameidata *nd)
 +int security_inode_follow_link(struct dentry *dentry, struct inode *inode,
 +			       bool rcu)
  {
 -	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 +	if (unlikely(IS_PRIVATE(inode)))
  		return 0;
- 	return security_ops->inode_follow_link(dentry, inode, rcu);
 -	return call_int_hook(inode_follow_link, 0, dentry, nd);
++	return call_int_hook(inode_follow_link, 0, dentry, inode, rcu);
  }
  
  int security_inode_permission(struct inode *inode, int mask)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

linux-next: manual merge of the security tree with the vfs tree

[Stephen Rothwell]

Hi James,

Today's linux-next merge of the security tree got a conflict in:

  security/integrity/ima/ima_fs.c

between commit:

  3bc8f29b149e ("new helper: memdup_user_nul()")

from the vfs tree and commit:

  38d859f991f3 ("IMA: policy can now be updated multiple times")

from the security tree.

I fixed it up (hopefully, see below) and can carry the fix as necessary
(no action is required).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc security/integrity/ima/ima_fs.c
index 71aa60b8d257,3caed6de610c..000000000000
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@@ -259,21 -261,35 +261,30 @@@ static const struct file_operations ima
  static ssize_t ima_write_policy(struct file *file, const char __user *buf,
  				size_t datalen, loff_t *ppos)
  {
 -	char *data = NULL;
  	ssize_t result;
- 	char *data;
++	char *data = NULL;
+ 	int res;
+ 
+ 	res = mutex_lock_interruptible(&ima_write_mutex);
+ 	if (res)
+ 		return res;
  
  	if (datalen >= PAGE_SIZE)
  		datalen = PAGE_SIZE - 1;
  
  	/* No partial writes. */
+ 	result = -EINVAL;
  	if (*ppos != 0)
- 		return -EINVAL;
+ 		goto out;
  
 -	result = -ENOMEM;
 -	data = kmalloc(datalen + 1, GFP_KERNEL);
 -	if (!data)
 -		goto out;
 -
 -	*(data + datalen) = '\0';
 -
 -	result = -EFAULT;
 -	if (copy_from_user(data, buf, datalen))
 +	data = memdup_user_nul(buf, datalen);
- 	if (IS_ERR(data))
- 		return PTR_ERR(data);
++	if (IS_ERR(data)) {
++		result = PTR_ERR(data);
+ 		goto out;
++	}
  
  	result = ima_parse_add_rule(data);
+ out:
  	if (result < 0)
  		valid_policy = 0;
  	kfree(data);

Re: linux-next: manual merge of the security tree with the vfs tree

[Al Viro]

On Thu, Dec 31, 2015 at 03:24:53PM +1100, Stephen Rothwell wrote:
> Hi James,
> 
> Today's linux-next merge of the security tree got a conflict in:
> 
>   security/integrity/ima/ima_fs.c
> 
> between commit:
> 
>   3bc8f29b149e ("new helper: memdup_user_nul()")
> 
> from the vfs tree and commit:
> 
>   38d859f991f3 ("IMA: policy can now be updated multiple times")
> 
> from the security tree.
> 
> I fixed it up (hopefully, see below) and can carry the fix as necessary
> (no action is required).
 
> + 	res = mutex_lock_interruptible(&ima_write_mutex);
> + 	if (res)
> + 		return res;
>   
>   	if (datalen >= PAGE_SIZE)
>   		datalen = PAGE_SIZE - 1;
>   
>   	/* No partial writes. */
> + 	result = -EINVAL;
>   	if (*ppos != 0)
> - 		return -EINVAL;
> + 		goto out;
>   
>  -	result = -ENOMEM;
>  -	data = kmalloc(datalen + 1, GFP_KERNEL);
>  -	if (!data)
>  -		goto out;
>  -
>  -	*(data + datalen) = '\0';
>  -
>  -	result = -EFAULT;
>  -	if (copy_from_user(data, buf, datalen))
>  +	data = memdup_user_nul(buf, datalen);
> - 	if (IS_ERR(data))
> - 		return PTR_ERR(data);
> ++	if (IS_ERR(data)) {
> ++		result = PTR_ERR(data);
> + 		goto out;
> ++	}

Why do it in this order?  With or without opencoding memdup_user_nul(),
what's the point of taking the mutex before copying the data from
userland?  All it achieves is holding it longer, over the area that
needs no exclusion whatsoever.

Re: linux-next: manual merge of the security tree with the vfs tree

[Petko Manolov]

On 15-12-31 04:30:19, Al Viro wrote:
> On Thu, Dec 31, 2015 at 03:24:53PM +1100, Stephen Rothwell wrote:
> > Hi James,
> > 
> > Today's linux-next merge of the security tree got a conflict in:
> > 
> >   security/integrity/ima/ima_fs.c
> > 
> > between commit:
> > 
> >   3bc8f29b149e ("new helper: memdup_user_nul()")
> > 
> > from the vfs tree and commit:
> > 
> >   38d859f991f3 ("IMA: policy can now be updated multiple times")
> > 
> > from the security tree.
> > 
> > I fixed it up (hopefully, see below) and can carry the fix as necessary
> > (no action is required).
>  
> > + 	res = mutex_lock_interruptible(&ima_write_mutex);
> > + 	if (res)
> > + 		return res;
> >   
> >   	if (datalen >= PAGE_SIZE)
> >   		datalen = PAGE_SIZE - 1;
> >   
> >   	/* No partial writes. */
> > + 	result = -EINVAL;
> >   	if (*ppos != 0)
> > - 		return -EINVAL;
> > + 		goto out;
> >   
> >  -	result = -ENOMEM;
> >  -	data = kmalloc(datalen + 1, GFP_KERNEL);
> >  -	if (!data)
> >  -		goto out;
> >  -
> >  -	*(data + datalen) = '\0';
> >  -
> >  -	result = -EFAULT;
> >  -	if (copy_from_user(data, buf, datalen))
> >  +	data = memdup_user_nul(buf, datalen);
> > - 	if (IS_ERR(data))
> > - 		return PTR_ERR(data);
> > ++	if (IS_ERR(data)) {
> > ++		result = PTR_ERR(data);
> > + 		goto out;
> > ++	}
> 
> Why do it in this order?  With or without opencoding memdup_user_nul(),
> what's the point of taking the mutex before copying the data from
> userland?  All it achieves is holding it longer, over the area that
> needs no exclusion whatsoever.

I introduced the write mutex when ima_write_policy() stopped being serialized by 
other means.  Come to think about it the semaphore could be taken right before 
copy_from_user() so it is my fault, not Stephen's.

The patch, however, leaves out a bug where free without allocation can occur.  
Look at *ppos evaluation.  Instead of "goto out" it should be "return -EINVAL;".  
This requires the mutex lock to be moved down, though.


cheers,
Petko

Re: linux-next: manual merge of the security tree with the vfs tree

[Al Viro]

On Thu, Dec 31, 2015 at 12:45:35PM +0200, Petko Manolov wrote:

> I introduced the write mutex when ima_write_policy() stopped being serialized by 
> other means.  Come to think about it the semaphore could be taken right before 
> copy_from_user() so it is my fault, not Stephen's.

s/before/after/, surely?

Re: linux-next: manual merge of the security tree with the vfs tree

[Petko Manolov]

On 16-01-01 04:34:16, Al Viro wrote:
> On Thu, Dec 31, 2015 at 12:45:35PM +0200, Petko Manolov wrote:
> 
> > I introduced the write mutex when ima_write_policy() stopped being serialized by 
> > other means.  Come to think about it the semaphore could be taken right before 
> > copy_from_user() so it is my fault, not Stephen's.
> 
> s/before/after/, surely?

Right.  This is a quick patch which i hope solves most issues...


		Petko


>From 6c9058009c59fda5b8e98a3fc09497ce3efdb3e9 Mon Sep 17 00:00:00 2001
From: Petko Manolov <petkan@mip-labs.com>
Date: Fri, 1 Jan 2016 19:10:43 +0200
Subject: [PATCH] ima_write_policy() optimizations;

There is no need to hold the write semaphore for so long.  We only need it
around ima_parse_add_rule();

The return path now takes into account failed kmalloc() call.

Signed-off-by: Petko Manolov <petkan@mip-labs.com>
---
 security/integrity/ima/ima_fs.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 3caed6d..d2c0d55 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -261,13 +261,7 @@ static const struct file_operations ima_ascii_measurements_ops = {
 static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 				size_t datalen, loff_t *ppos)
 {
-	char *data = NULL;
 	ssize_t result;
-	int res;
-
-	res = mutex_lock_interruptible(&ima_write_mutex);
-	if (res)
-		return res;
 
 	if (datalen >= PAGE_SIZE)
 		datalen = PAGE_SIZE - 1;
@@ -286,15 +280,21 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 
 	result = -EFAULT;
 	if (copy_from_user(data, buf, datalen))
-		goto out;
+		goto out_free;
+
+	result = mutex_lock_interruptible(&ima_write_mutex);
+	if (result)
+		goto out_free;
 
 	result = ima_parse_add_rule(data);
-out:
+
+	mutex_unlock(&ima_write_mutex);
+
 	if (result < 0)
 		valid_policy = 0;
+out_free:
 	kfree(data);
-	mutex_unlock(&ima_write_mutex);
-
+out:
 	return result;
 }
 
-- 
2.6.4

Re: linux-next: manual merge of the security tree with the vfs tree

[Mimi Zohar]

On Thu, 2015-12-31 at 15:24 +1100, Stephen Rothwell wrote:
> Hi James,
> 
> Today's linux-next merge of the security tree got a conflict in:
> 
>   security/integrity/ima/ima_fs.c
> 
> between commit:
> 
>   3bc8f29b149e ("new helper: memdup_user_nul()")
> 
> from the vfs tree and commit:
> 
>   38d859f991f3 ("IMA: policy can now be updated multiple times")
> 
> from the security tree.
> 
> I fixed it up (hopefully, see below) and can carry the fix as necessary
> (no action is required).

Hi Stephen,

FYI, I pushed out Petko's patch to linux-integrity/next earlier today.
His patch moves taking the ima_write_mutex to after the the call to
copy_from_user(), as discussed.  This obviously won't fix the conflict
with Al's patch.  How do you want to handle it?  Do I need to do
anything?

Thanks!

Mimi

Re: linux-next: manual merge of the security tree with the vfs tree

[Stephen Rothwell]

Hi Mimi,

On Sun, 03 Jan 2016 20:37:20 -0500 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
> FYI, I pushed out Petko's patch to linux-integrity/next earlier today.
> His patch moves taking the ima_write_mutex to after the the call to
> copy_from_user(), as discussed.  This obviously won't fix the conflict
> with Al's patch.  How do you want to handle it?  Do I need to do
> anything?

No, I will just do a different conflict resolution.  I will send the
usual email when I get to it.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

linux-next: manual merge of the security tree with the vfs tree

[Stephen Rothwell]

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

Hi all,

Today's linux-next merge of the security tree got a conflict in:

  fs/tracefs/inode.c

between commit:

  67782f8ae148 ("vfs: Convert tracefs to use the new mount API")

from the vfs tree and commit:

  757ff7244358 ("tracefs: Restrict tracefs when the kernel is locked down")

from the security tree.

I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging.  You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc fs/tracefs/inode.c
index 1387bcd96a79,34da48036e08..000000000000
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@@ -19,8 -17,10 +19,9 @@@
  #include <linux/tracefs.h>
  #include <linux/fsnotify.h>
  #include <linux/seq_file.h>
 -#include <linux/parser.h>
  #include <linux/magic.h>
  #include <linux/slab.h>
+ #include <linux/security.h>
  
  #define TRACEFS_DEFAULT_MODE	0700
  
@@@ -210,12 -239,26 +228,18 @@@ static int tracefs_apply_options(struc
  	return 0;
  }
  
+ static void tracefs_destroy_inode(struct inode *inode)
+ {
+ 	if (S_ISREG(inode->i_mode))
+ 		kfree(inode->i_fop);
+ }
+ 
 -static int tracefs_remount(struct super_block *sb, int *flags, char *data)
 +static int tracefs_reconfigure(struct fs_context *fc)
  {
 -	int err;
 -	struct tracefs_fs_info *fsi = sb->s_fs_info;
 +	struct super_block *sb = fc->root->d_sb;
  
  	sync_filesystem(sb);
 -	err = tracefs_parse_options(data, &fsi->mount_opts);
 -	if (err)
 -		goto fail;
 -
 -	tracefs_apply_options(sb);
 -
 -fail:
 -	return err;
 +	return tracefs_apply_options(sb);
  }
  
  static int tracefs_show_options(struct seq_file *m, struct dentry *root)
@@@ -236,6 -280,8 +260,7 @@@
  
  static const struct super_operations tracefs_super_operations = {
  	.statfs		= simple_statfs,
 -	.remount_fs	= tracefs_remount,
+ 	.destroy_inode  = tracefs_destroy_inode,
  	.show_options	= tracefs_show_options,
  };
  

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]