integrity: nfsd imbalance bug fix

integrity: nfsd imbalance bug fix

[Mimi Zohar]

The number of calls to ima_path_check()/ima_file_free()
should be balanced.  An extra call to fput(), indicates
the file could have been accessed without first being
measured.

An nfsd exported file is opened/closed by the kernel
causing an integrity imbalance message.

- rename and export opencount_get to ima_opencount_get
- replace ima_shm_check calls with ima_opencount_get
- add call to increment opencount for files opened by nfsd.
- add call to measure exported files in nfsd_permission().
- export ima_path_check

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>

Index: security-testing-2.6/fs/nfsd/vfs.c
===================================================================
--- security-testing-2.6.orig/fs/nfsd/vfs.c
+++ security-testing-2.6/fs/nfsd/vfs.c
@@ -55,6 +55,7 @@
 #include <linux/security.h>
 #endif /* CONFIG_NFSD_V4 */
 #include <linux/jhash.h>
+#include <linux/ima.h>
 
 #include <asm/uaccess.h>
 
@@ -735,6 +736,8 @@ nfsd_open(struct svc_rqst *rqstp, struct
 			    flags, cred);
 	if (IS_ERR(*filp))
 		host_err = PTR_ERR(*filp);
+	else
+		ima_opencount_get(*filp);
 out_nfserr:
 	err = nfserrno(host_err);
 out:
@@ -2096,7 +2099,13 @@ nfsd_permission(struct svc_rqst *rqstp, 
 	if (err == -EACCES && S_ISREG(inode->i_mode) &&
 	    acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE))
 		err = inode_permission(inode, MAY_EXEC);
+	if (err)
+		goto nfsd_out;
 
+	err = ima_path_check(&exp->ex_path,
+			     acc & (MAY_READ | MAY_WRITE | MAY_EXEC));
+	return err;
+nfsd_out:
 	return err? nfserrno(err) : 0;
 }
 
Index: security-testing-2.6/security/integrity/ima/ima_main.c
===================================================================
--- security-testing-2.6.orig/security/integrity/ima/ima_main.c
+++ security-testing-2.6/security/integrity/ima/ima_main.c
@@ -206,6 +206,7 @@ out:
 	kref_put(&iint->refcount, iint_free);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ima_path_check);
 
 static int process_measurement(struct file *file, const unsigned char *filename,
 			       int mask, int function)
@@ -234,7 +235,16 @@ out:
 	return rc;
 }
 
-static void opencount_get(struct file *file)
+/*
+ * ima_opencount_get - incr opencount for files opened by the kernel
+ *
+ * - IPC shm and shmat create/fput a file.
+ * - nfsd opens/closes exported files.
+ *
+ * Increment the opencount for these files to prevent unnecessary
+ * imbalance messages.
+ */
+void ima_opencount_get(struct file *file)
 {
 	struct inode *inode = file->f_dentry->d_inode;
 	struct ima_iint_cache *iint;
@@ -248,6 +258,7 @@ static void opencount_get(struct file *f
 	iint->opencount++;
 	mutex_unlock(&iint->mutex);
 }
+EXPORT_SYMBOL_GPL(ima_opencount_get);
 
 /**
  * ima_file_mmap - based on policy, collect/store measurement.
@@ -272,18 +283,6 @@ int ima_file_mmap(struct file *file, uns
 	return 0;
 }
 
-/*
- * ima_shm_check - IPC shm and shmat create/fput a file
- *
- * Maintain the opencount for these files to prevent unnecessary
- * imbalance messages.
- */
-void ima_shm_check(struct file *file)
-{
-	opencount_get(file);
-	return;
-}
-
 /**
  * ima_bprm_check - based on policy, collect/store measurement.
  * @bprm: contains the linux_binprm structure
Index: security-testing-2.6/include/linux/ima.h
===================================================================
--- security-testing-2.6.orig/include/linux/ima.h
+++ security-testing-2.6/include/linux/ima.h
@@ -20,7 +20,7 @@ extern void ima_inode_free(struct inode 
 extern int ima_path_check(struct path *path, int mask);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
-extern void ima_shm_check(struct file *file);
+extern void ima_opencount_get(struct file *file);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -53,7 +53,7 @@ static inline int ima_file_mmap(struct f
 	return 0;
 }
 
-static inline void ima_shm_check(struct file *file)
+static inline void ima_opencount_get(struct file *file)
 {
 	return;
 }
Index: security-testing-2.6/ipc/shm.c
===================================================================
--- security-testing-2.6.orig/ipc/shm.c
+++ security-testing-2.6/ipc/shm.c
@@ -384,7 +384,7 @@ static int newseg(struct ipc_namespace *
 	error = PTR_ERR(file);
 	if (IS_ERR(file))
 		goto no_file;
-	ima_shm_check(file);
+	ima_opencount_get(file);
 
 	id = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni);
 	if (id < 0) {
@@ -891,7 +891,7 @@ long do_shmat(int shmid, char __user *sh
 	file = alloc_file(path.mnt, path.dentry, f_mode, &shm_file_operations);
 	if (!file)
 		goto out_free;
-	ima_shm_check(file);
+	ima_opencount_get(file);
 
 	file->private_data = sfd;
 	file->f_mapping = shp->shm_file->f_mapping;
Index: security-testing-2.6/mm/shmem.c
===================================================================
--- security-testing-2.6.orig/mm/shmem.c
+++ security-testing-2.6/mm/shmem.c
@@ -2680,7 +2680,7 @@ int shmem_zero_setup(struct vm_area_stru
 	if (IS_ERR(file))
 		return PTR_ERR(file);
 
-	ima_shm_check(file);
+	ima_opencount_get(file);
 	if (vma->vm_file)
 		fput(vma->vm_file);
 	vma->vm_file = file;

Re: integrity: nfsd imbalance bug fix

[James Morris]

On Tue, 28 Apr 2009, Mimi Zohar wrote:

> The number of calls to ima_path_check()/ima_file_free()
> should be balanced.  An extra call to fput(), indicates
> the file could have been accessed without first being
> measured.
> 
> An nfsd exported file is opened/closed by the kernel
> causing an integrity imbalance message.

[Adding NFS folk to the CC]

> 
> - rename and export opencount_get to ima_opencount_get
> - replace ima_shm_check calls with ima_opencount_get
> - add call to increment opencount for files opened by nfsd.
> - add call to measure exported files in nfsd_permission().
> - export ima_path_check
> 
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> 
> Index: security-testing-2.6/fs/nfsd/vfs.c
> ===================================================================
> --- security-testing-2.6.orig/fs/nfsd/vfs.c
> +++ security-testing-2.6/fs/nfsd/vfs.c
> @@ -55,6 +55,7 @@
>  #include <linux/security.h>
>  #endif /* CONFIG_NFSD_V4 */
>  #include <linux/jhash.h>
> +#include <linux/ima.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -735,6 +736,8 @@ nfsd_open(struct svc_rqst *rqstp, struct
>  			    flags, cred);
>  	if (IS_ERR(*filp))
>  		host_err = PTR_ERR(*filp);
> +	else
> +		ima_opencount_get(*filp);
>  out_nfserr:
>  	err = nfserrno(host_err);
>  out:
> @@ -2096,7 +2099,13 @@ nfsd_permission(struct svc_rqst *rqstp, 
>  	if (err == -EACCES && S_ISREG(inode->i_mode) &&
>  	    acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE))
>  		err = inode_permission(inode, MAY_EXEC);
> +	if (err)
> +		goto nfsd_out;
>  
> +	err = ima_path_check(&exp->ex_path,
> +			     acc & (MAY_READ | MAY_WRITE | MAY_EXEC));
> +	return err;
> +nfsd_out:
>  	return err? nfserrno(err) : 0;
>  }
>  
> Index: security-testing-2.6/security/integrity/ima/ima_main.c
> ===================================================================
> --- security-testing-2.6.orig/security/integrity/ima/ima_main.c
> +++ security-testing-2.6/security/integrity/ima/ima_main.c
> @@ -206,6 +206,7 @@ out:
>  	kref_put(&iint->refcount, iint_free);
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(ima_path_check);
>  
>  static int process_measurement(struct file *file, const unsigned char *filename,
>  			       int mask, int function)
> @@ -234,7 +235,16 @@ out:
>  	return rc;
>  }
>  
> -static void opencount_get(struct file *file)
> +/*
> + * ima_opencount_get - incr opencount for files opened by the kernel
> + *
> + * - IPC shm and shmat create/fput a file.
> + * - nfsd opens/closes exported files.
> + *
> + * Increment the opencount for these files to prevent unnecessary
> + * imbalance messages.
> + */
> +void ima_opencount_get(struct file *file)
>  {
>  	struct inode *inode = file->f_dentry->d_inode;
>  	struct ima_iint_cache *iint;
> @@ -248,6 +258,7 @@ static void opencount_get(struct file *f
>  	iint->opencount++;
>  	mutex_unlock(&iint->mutex);
>  }
> +EXPORT_SYMBOL_GPL(ima_opencount_get);
>  
>  /**
>   * ima_file_mmap - based on policy, collect/store measurement.
> @@ -272,18 +283,6 @@ int ima_file_mmap(struct file *file, uns
>  	return 0;
>  }
>  
> -/*
> - * ima_shm_check - IPC shm and shmat create/fput a file
> - *
> - * Maintain the opencount for these files to prevent unnecessary
> - * imbalance messages.
> - */
> -void ima_shm_check(struct file *file)
> -{
> -	opencount_get(file);
> -	return;
> -}
> -
>  /**
>   * ima_bprm_check - based on policy, collect/store measurement.
>   * @bprm: contains the linux_binprm structure
> Index: security-testing-2.6/include/linux/ima.h
> ===================================================================
> --- security-testing-2.6.orig/include/linux/ima.h
> +++ security-testing-2.6/include/linux/ima.h
> @@ -20,7 +20,7 @@ extern void ima_inode_free(struct inode 
>  extern int ima_path_check(struct path *path, int mask);
>  extern void ima_file_free(struct file *file);
>  extern int ima_file_mmap(struct file *file, unsigned long prot);
> -extern void ima_shm_check(struct file *file);
> +extern void ima_opencount_get(struct file *file);
>  
>  #else
>  static inline int ima_bprm_check(struct linux_binprm *bprm)
> @@ -53,7 +53,7 @@ static inline int ima_file_mmap(struct f
>  	return 0;
>  }
>  
> -static inline void ima_shm_check(struct file *file)
> +static inline void ima_opencount_get(struct file *file)
>  {
>  	return;
>  }
> Index: security-testing-2.6/ipc/shm.c
> ===================================================================
> --- security-testing-2.6.orig/ipc/shm.c
> +++ security-testing-2.6/ipc/shm.c
> @@ -384,7 +384,7 @@ static int newseg(struct ipc_namespace *
>  	error = PTR_ERR(file);
>  	if (IS_ERR(file))
>  		goto no_file;
> -	ima_shm_check(file);
> +	ima_opencount_get(file);
>  
>  	id = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni);
>  	if (id < 0) {
> @@ -891,7 +891,7 @@ long do_shmat(int shmid, char __user *sh
>  	file = alloc_file(path.mnt, path.dentry, f_mode, &shm_file_operations);
>  	if (!file)
>  		goto out_free;
> -	ima_shm_check(file);
> +	ima_opencount_get(file);
>  
>  	file->private_data = sfd;
>  	file->f_mapping = shp->shm_file->f_mapping;
> Index: security-testing-2.6/mm/shmem.c
> ===================================================================
> --- security-testing-2.6.orig/mm/shmem.c
> +++ security-testing-2.6/mm/shmem.c
> @@ -2680,7 +2680,7 @@ int shmem_zero_setup(struct vm_area_stru
>  	if (IS_ERR(file))
>  		return PTR_ERR(file);
>  
> -	ima_shm_check(file);
> +	ima_opencount_get(file);
>  	if (vma->vm_file)
>  		fput(vma->vm_file);
>  	vma->vm_file = file;
> 
> 

-- 
James Morris
<jmorris@namei.org>

Re: integrity: nfsd imbalance bug fix

[Andrew Morton]

On Tue, 28 Apr 2009 14:32:14 -0400
Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> The number of calls to ima_path_check()/ima_file_free()
> should be balanced.  An extra call to fput(), indicates
> the file could have been accessed without first being
> measured.
> 
> An nfsd exported file is opened/closed by the kernel
> causing an integrity imbalance message.
> 
> - rename and export opencount_get to ima_opencount_get
> - replace ima_shm_check calls with ima_opencount_get
> - add call to increment opencount for files opened by nfsd.
> - add call to measure exported files in nfsd_permission().
> - export ima_path_check
> 

The patch looks fragile to me.  It sprinkles IMA-specific operations
over random unrelated subsystems.  There is hence a decent chance of
breakage in the future.

Suppose some other new piece of kernel code opens/closes a file.  The
developer didn't think to add the IMA hooks and whoops, we have a bug.

It would be really really better if we could add the IMA hooks in a
single place.  That might require the addition of a new function, and
that's fine.  d_instantiate_kernel() or init_file_kernel() or whatever
- that's fine.  It still has the risk that new code will forget to use
the in-kernel variant, but we have a better chance of detecting it.

> 
> Index: security-testing-2.6/fs/nfsd/vfs.c
> ===================================================================
> --- security-testing-2.6.orig/fs/nfsd/vfs.c
> +++ security-testing-2.6/fs/nfsd/vfs.c
> @@ -55,6 +55,7 @@
>  #include <linux/security.h>
>  #endif /* CONFIG_NFSD_V4 */
>  #include <linux/jhash.h>
> +#include <linux/ima.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -735,6 +736,8 @@ nfsd_open(struct svc_rqst *rqstp, struct
>  			    flags, cred);
>  	if (IS_ERR(*filp))
>  		host_err = PTR_ERR(*filp);
> +	else
> +		ima_opencount_get(*filp);

This suggests dentry_open_kernel().

>  out_nfserr:
>  	err = nfserrno(host_err);
>  out:
> @@ -2096,7 +2099,13 @@ nfsd_permission(struct svc_rqst *rqstp, 
>  	if (err == -EACCES && S_ISREG(inode->i_mode) &&
>  	    acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE))
>  		err = inode_permission(inode, MAY_EXEC);
> +	if (err)
> +		goto nfsd_out;
>  
> +	err = ima_path_check(&exp->ex_path,
> +			     acc & (MAY_READ | MAY_WRITE | MAY_EXEC));

hm, dunno what to do about that.

> +	return err;
> +nfsd_out:
>  	return err? nfserrno(err) : 0;
>  }
>  
> Index: security-testing-2.6/security/integrity/ima/ima_main.c
> ===================================================================
> --- security-testing-2.6.orig/security/integrity/ima/ima_main.c
> +++ security-testing-2.6/security/integrity/ima/ima_main.c
> @@ -206,6 +206,7 @@ out:
>  	kref_put(&iint->refcount, iint_free);
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(ima_path_check);
>  
>  static int process_measurement(struct file *file, const unsigned char *filename,
>  			       int mask, int function)
> @@ -234,7 +235,16 @@ out:
>  	return rc;
>  }
>  
> -static void opencount_get(struct file *file)
> +/*
> + * ima_opencount_get - incr opencount for files opened by the kernel
> + *
> + * - IPC shm and shmat create/fput a file.
> + * - nfsd opens/closes exported files.
> + *
> + * Increment the opencount for these files to prevent unnecessary
> + * imbalance messages.
> + */
> +void ima_opencount_get(struct file *file)
>  {
>  	struct inode *inode = file->f_dentry->d_inode;
>  	struct ima_iint_cache *iint;
> @@ -248,6 +258,7 @@ static void opencount_get(struct file *f
>  	iint->opencount++;
>  	mutex_unlock(&iint->mutex);
>  }
> +EXPORT_SYMBOL_GPL(ima_opencount_get);
>  
>  /**
>   * ima_file_mmap - based on policy, collect/store measurement.
> @@ -272,18 +283,6 @@ int ima_file_mmap(struct file *file, uns
>  	return 0;
>  }
>  
> -/*
> - * ima_shm_check - IPC shm and shmat create/fput a file
> - *
> - * Maintain the opencount for these files to prevent unnecessary
> - * imbalance messages.
> - */
> -void ima_shm_check(struct file *file)
> -{
> -	opencount_get(file);
> -	return;
> -}
> -
>  /**
>   * ima_bprm_check - based on policy, collect/store measurement.
>   * @bprm: contains the linux_binprm structure
> Index: security-testing-2.6/include/linux/ima.h
> ===================================================================
> --- security-testing-2.6.orig/include/linux/ima.h
> +++ security-testing-2.6/include/linux/ima.h
> @@ -20,7 +20,7 @@ extern void ima_inode_free(struct inode 
>  extern int ima_path_check(struct path *path, int mask);
>  extern void ima_file_free(struct file *file);
>  extern int ima_file_mmap(struct file *file, unsigned long prot);
> -extern void ima_shm_check(struct file *file);
> +extern void ima_opencount_get(struct file *file);
>  
>  #else
>  static inline int ima_bprm_check(struct linux_binprm *bprm)
> @@ -53,7 +53,7 @@ static inline int ima_file_mmap(struct f
>  	return 0;
>  }
>  
> -static inline void ima_shm_check(struct file *file)
> +static inline void ima_opencount_get(struct file *file)
>  {
>  	return;
>  }
> Index: security-testing-2.6/ipc/shm.c
> ===================================================================
> --- security-testing-2.6.orig/ipc/shm.c
> +++ security-testing-2.6/ipc/shm.c
> @@ -384,7 +384,7 @@ static int newseg(struct ipc_namespace *
>  	error = PTR_ERR(file);
>  	if (IS_ERR(file))
>  		goto no_file;
> -	ima_shm_check(file);
> +	ima_opencount_get(file);
>  
>  	id = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni);
>  	if (id < 0) {
> @@ -891,7 +891,7 @@ long do_shmat(int shmid, char __user *sh
>  	file = alloc_file(path.mnt, path.dentry, f_mode, &shm_file_operations);
>  	if (!file)
>  		goto out_free;
> -	ima_shm_check(file);
> +	ima_opencount_get(file);
>  
>  	file->private_data = sfd;
>  	file->f_mapping = shp->shm_file->f_mapping;
> Index: security-testing-2.6/mm/shmem.c
> ===================================================================
> --- security-testing-2.6.orig/mm/shmem.c
> +++ security-testing-2.6/mm/shmem.c
> @@ -2680,7 +2680,7 @@ int shmem_zero_setup(struct vm_area_stru
>  	if (IS_ERR(file))
>  		return PTR_ERR(file);
>  
> -	ima_shm_check(file);
> +	ima_opencount_get(file);
>  	if (vma->vm_file)
>  		fput(vma->vm_file);
>  	vma->vm_file = file;

Maybe do the IMA operations in (or under) shmem_file_setup() and
hugetlb_file_setup()?

Re: integrity: nfsd imbalance bug fix

[Mimi Zohar]

On Wed, 2009-04-29 at 14:18 -0700, Andrew Morton wrote: 
> On Tue, 28 Apr 2009 14:32:14 -0400
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > The number of calls to ima_path_check()/ima_file_free()
> > should be balanced.  An extra call to fput(), indicates
> > the file could have been accessed without first being
> > measured.
> > 
> > An nfsd exported file is opened/closed by the kernel
> > causing an integrity imbalance message.
> > 
> > - rename and export opencount_get to ima_opencount_get
> > - replace ima_shm_check calls with ima_opencount_get
> > - add call to increment opencount for files opened by nfsd.
> > - add call to measure exported files in nfsd_permission().
> > - export ima_path_check
> > 
> 
> The patch looks fragile to me.  It sprinkles IMA-specific operations
> over random unrelated subsystems.  There is hence a decent chance of
> breakage in the future.
> 
> Suppose some other new piece of kernel code opens/closes a file.  The
> developer didn't think to add the IMA hooks and whoops, we have a bug.

Agreed.  Exactly for that reason we have the imbalance message. The
imbalance message is not a bug per se, but an indication that the
file hasn't been integrity (permission) checked.

> It would be really really better if we could add the IMA hooks in a
> single place.  That might require the addition of a new function, and
> that's fine.  d_instantiate_kernel() or init_file_kernel() or whatever
> - that's fine.  It still has the risk that new code will forget to use
> the in-kernel variant, but we have a better chance of detecting it.

Definitely. Originally the integrity checking was in inode_permission().
But measuring a file requires a dentry and mount point.

I've updated the patch to fix a 'counts' error and divided the patch
into two, separating the IMA code changes from the IMA calls in nfsd.

> > 
> > Index: security-testing-2.6/fs/nfsd/vfs.c
> > ===================================================================
> > --- security-testing-2.6.orig/fs/nfsd/vfs.c
> > +++ security-testing-2.6/fs/nfsd/vfs.c
> > @@ -55,6 +55,7 @@
> >  #include <linux/security.h>
> >  #endif /* CONFIG_NFSD_V4 */
> >  #include <linux/jhash.h>
> > +#include <linux/ima.h>
> >  
> >  #include <asm/uaccess.h>
> >  
> > @@ -735,6 +736,8 @@ nfsd_open(struct svc_rqst *rqstp, struct
> >  			    flags, cred);
> >  	if (IS_ERR(*filp))
> >  		host_err = PTR_ERR(*filp);
> > +	else
> > +		ima_opencount_get(*filp);
> 
> This suggests dentry_open_kernel().

Good name, but making dentry_open_kernel() the default behavior,
would mask the real bug - not doing integrity (permission) checking.

> 
> >  out_nfserr:
> >  	err = nfserrno(host_err);
> >  out:
> > @@ -2096,7 +2099,13 @@ nfsd_permission(struct svc_rqst *rqstp, 
> >  	if (err == -EACCES && S_ISREG(inode->i_mode) &&
> >  	    acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE))
> >  		err = inode_permission(inode, MAY_EXEC);
> > +	if (err)
> > +		goto nfsd_out;
> >  
> > +	err = ima_path_check(&exp->ex_path,
> > +			     acc & (MAY_READ | MAY_WRITE | MAY_EXEC));
> 
> hm, dunno what to do about that.

The real problem is that this belongs in inode_permission(), but an
inode is not enough.  It requires a dentry and mount point to
measure the file.

> > +	return err;
> > +nfsd_out:
> >  	return err? nfserrno(err) : 0;
> >  }
> >  
> > Index: security-testing-2.6/security/integrity/ima/ima_main.c
> > ===================================================================
> > --- security-testing-2.6.orig/security/integrity/ima/ima_main.c
> > +++ security-testing-2.6/security/integrity/ima/ima_main.c
> > @@ -206,6 +206,7 @@ out:
> >  	kref_put(&iint->refcount, iint_free);
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(ima_path_check);
> >  
> >  static int process_measurement(struct file *file, const unsigned char *filename,
> >  			       int mask, int function)
> > @@ -234,7 +235,16 @@ out:
> >  	return rc;
> >  }
> >  
> > -static void opencount_get(struct file *file)
> > +/*
> > + * ima_opencount_get - incr opencount for files opened by the kernel
> > + *
> > + * - IPC shm and shmat create/fput a file.
> > + * - nfsd opens/closes exported files.
> > + *
> > + * Increment the opencount for these files to prevent unnecessary
> > + * imbalance messages.
> > + */
> > +void ima_opencount_get(struct file *file)
> >  {
> >  	struct inode *inode = file->f_dentry->d_inode;
> >  	struct ima_iint_cache *iint;
> > @@ -248,6 +258,7 @@ static void opencount_get(struct file *f
> >  	iint->opencount++;
> >  	mutex_unlock(&iint->mutex);
> >  }
> > +EXPORT_SYMBOL_GPL(ima_opencount_get);
> >  
> >  /**
> >   * ima_file_mmap - based on policy, collect/store measurement.
> > @@ -272,18 +283,6 @@ int ima_file_mmap(struct file *file, uns
> >  	return 0;
> >  }
> >  
> > -/*
> > - * ima_shm_check - IPC shm and shmat create/fput a file
> > - *
> > - * Maintain the opencount for these files to prevent unnecessary
> > - * imbalance messages.
> > - */
> > -void ima_shm_check(struct file *file)
> > -{
> > -	opencount_get(file);
> > -	return;
> > -}
> > -
> >  /**
> >   * ima_bprm_check - based on policy, collect/store measurement.
> >   * @bprm: contains the linux_binprm structure
> > Index: security-testing-2.6/include/linux/ima.h
> > ===================================================================
> > --- security-testing-2.6.orig/include/linux/ima.h
> > +++ security-testing-2.6/include/linux/ima.h
> > @@ -20,7 +20,7 @@ extern void ima_inode_free(struct inode 
> >  extern int ima_path_check(struct path *path, int mask);
> >  extern void ima_file_free(struct file *file);
> >  extern int ima_file_mmap(struct file *file, unsigned long prot);
> > -extern void ima_shm_check(struct file *file);
> > +extern void ima_opencount_get(struct file *file);
> >  
> >  #else
> >  static inline int ima_bprm_check(struct linux_binprm *bprm)
> > @@ -53,7 +53,7 @@ static inline int ima_file_mmap(struct f
> >  	return 0;
> >  }
> >  
> > -static inline void ima_shm_check(struct file *file)
> > +static inline void ima_opencount_get(struct file *file)
> >  {
> >  	return;
> >  }
> > Index: security-testing-2.6/ipc/shm.c
> > ===================================================================
> > --- security-testing-2.6.orig/ipc/shm.c
> > +++ security-testing-2.6/ipc/shm.c
> > @@ -384,7 +384,7 @@ static int newseg(struct ipc_namespace *
> >  	error = PTR_ERR(file);
> >  	if (IS_ERR(file))
> >  		goto no_file;
> > -	ima_shm_check(file);
> > +	ima_opencount_get(file);
> >  
> >  	id = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni);
> >  	if (id < 0) {
> > @@ -891,7 +891,7 @@ long do_shmat(int shmid, char __user *sh
> >  	file = alloc_file(path.mnt, path.dentry, f_mode, &shm_file_operations);
> >  	if (!file)
> >  		goto out_free;
> > -	ima_shm_check(file);
> > +	ima_opencount_get(file);
> >  
> >  	file->private_data = sfd;
> >  	file->f_mapping = shp->shm_file->f_mapping;
> > Index: security-testing-2.6/mm/shmem.c
> > ===================================================================
> > --- security-testing-2.6.orig/mm/shmem.c
> > +++ security-testing-2.6/mm/shmem.c
> > @@ -2680,7 +2680,7 @@ int shmem_zero_setup(struct vm_area_stru
> >  	if (IS_ERR(file))
> >  		return PTR_ERR(file);
> >  
> > -	ima_shm_check(file);
> > +	ima_opencount_get(file);
> >  	if (vma->vm_file)
> >  		fput(vma->vm_file);
> >  	vma->vm_file = file;
> 
> Maybe do the IMA operations in (or under) shmem_file_setup() and
> hugetlb_file_setup()?

Again, that would hide the real issue of not having done integrity
(permission) checking.

Mimi

Re: integrity: nfsd imbalance bug fix

[J. Bruce Fields]

On Fri, May 08, 2009 at 01:35:20PM -0400, Mimi Zohar wrote:
> On Wed, 2009-04-29 at 14:18 -0700, Andrew Morton wrote: 
> > On Tue, 28 Apr 2009 14:32:14 -0400
> > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > 
> > > The number of calls to ima_path_check()/ima_file_free()
> > > should be balanced.  An extra call to fput(), indicates
> > > the file could have been accessed without first being
> > > measured.
> > > 
> > > An nfsd exported file is opened/closed by the kernel
> > > causing an integrity imbalance message.
> > > 
> > > - rename and export opencount_get to ima_opencount_get
> > > - replace ima_shm_check calls with ima_opencount_get
> > > - add call to increment opencount for files opened by nfsd.
> > > - add call to measure exported files in nfsd_permission().
> > > - export ima_path_check
> > > 
> > 
> > The patch looks fragile to me.  It sprinkles IMA-specific operations
> > over random unrelated subsystems.  There is hence a decent chance of
> > breakage in the future.
> > 
> > Suppose some other new piece of kernel code opens/closes a file.  The
> > developer didn't think to add the IMA hooks and whoops, we have a bug.
> 
> Agreed.  Exactly for that reason we have the imbalance message. The
> imbalance message is not a bug per se, but an indication that the
> file hasn't been integrity (permission) checked.

Where do I need to go to find out what this integrity checking is?  I
don't know the first thing about it....

--b.

> 
> > It would be really really better if we could add the IMA hooks in a
> > single place.  That might require the addition of a new function, and
> > that's fine.  d_instantiate_kernel() or init_file_kernel() or whatever
> > - that's fine.  It still has the risk that new code will forget to use
> > the in-kernel variant, but we have a better chance of detecting it.
> 
> Definitely. Originally the integrity checking was in inode_permission().
> But measuring a file requires a dentry and mount point.
> 
> I've updated the patch to fix a 'counts' error and divided the patch
> into two, separating the IMA code changes from the IMA calls in nfsd.
> 
> > > 
> > > Index: security-testing-2.6/fs/nfsd/vfs.c
> > > ===================================================================
> > > --- security-testing-2.6.orig/fs/nfsd/vfs.c
> > > +++ security-testing-2.6/fs/nfsd/vfs.c
> > > @@ -55,6 +55,7 @@
> > >  #include <linux/security.h>
> > >  #endif /* CONFIG_NFSD_V4 */
> > >  #include <linux/jhash.h>
> > > +#include <linux/ima.h>
> > >  
> > >  #include <asm/uaccess.h>
> > >  
> > > @@ -735,6 +736,8 @@ nfsd_open(struct svc_rqst *rqstp, struct
> > >  			    flags, cred);
> > >  	if (IS_ERR(*filp))
> > >  		host_err = PTR_ERR(*filp);
> > > +	else
> > > +		ima_opencount_get(*filp);
> > 
> > This suggests dentry_open_kernel().
> 
> Good name, but making dentry_open_kernel() the default behavior,
> would mask the real bug - not doing integrity (permission) checking.
> 
> > 
> > >  out_nfserr:
> > >  	err = nfserrno(host_err);
> > >  out:
> > > @@ -2096,7 +2099,13 @@ nfsd_permission(struct svc_rqst *rqstp, 
> > >  	if (err == -EACCES && S_ISREG(inode->i_mode) &&
> > >  	    acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE))
> > >  		err = inode_permission(inode, MAY_EXEC);
> > > +	if (err)
> > > +		goto nfsd_out;
> > >  
> > > +	err = ima_path_check(&exp->ex_path,
> > > +			     acc & (MAY_READ | MAY_WRITE | MAY_EXEC));
> > 
> > hm, dunno what to do about that.
> 
> The real problem is that this belongs in inode_permission(), but an
> inode is not enough.  It requires a dentry and mount point to
> measure the file.
> 
> > > +	return err;
> > > +nfsd_out:
> > >  	return err? nfserrno(err) : 0;
> > >  }
> > >  
> > > Index: security-testing-2.6/security/integrity/ima/ima_main.c
> > > ===================================================================
> > > --- security-testing-2.6.orig/security/integrity/ima/ima_main.c
> > > +++ security-testing-2.6/security/integrity/ima/ima_main.c
> > > @@ -206,6 +206,7 @@ out:
> > >  	kref_put(&iint->refcount, iint_free);
> > >  	return 0;
> > >  }
> > > +EXPORT_SYMBOL_GPL(ima_path_check);
> > >  
> > >  static int process_measurement(struct file *file, const unsigned char *filename,
> > >  			       int mask, int function)
> > > @@ -234,7 +235,16 @@ out:
> > >  	return rc;
> > >  }
> > >  
> > > -static void opencount_get(struct file *file)
> > > +/*
> > > + * ima_opencount_get - incr opencount for files opened by the kernel
> > > + *
> > > + * - IPC shm and shmat create/fput a file.
> > > + * - nfsd opens/closes exported files.
> > > + *
> > > + * Increment the opencount for these files to prevent unnecessary
> > > + * imbalance messages.
> > > + */
> > > +void ima_opencount_get(struct file *file)
> > >  {
> > >  	struct inode *inode = file->f_dentry->d_inode;
> > >  	struct ima_iint_cache *iint;
> > > @@ -248,6 +258,7 @@ static void opencount_get(struct file *f
> > >  	iint->opencount++;
> > >  	mutex_unlock(&iint->mutex);
> > >  }
> > > +EXPORT_SYMBOL_GPL(ima_opencount_get);
> > >  
> > >  /**
> > >   * ima_file_mmap - based on policy, collect/store measurement.
> > > @@ -272,18 +283,6 @@ int ima_file_mmap(struct file *file, uns
> > >  	return 0;
> > >  }
> > >  
> > > -/*
> > > - * ima_shm_check - IPC shm and shmat create/fput a file
> > > - *
> > > - * Maintain the opencount for these files to prevent unnecessary
> > > - * imbalance messages.
> > > - */
> > > -void ima_shm_check(struct file *file)
> > > -{
> > > -	opencount_get(file);
> > > -	return;
> > > -}
> > > -
> > >  /**
> > >   * ima_bprm_check - based on policy, collect/store measurement.
> > >   * @bprm: contains the linux_binprm structure
> > > Index: security-testing-2.6/include/linux/ima.h
> > > ===================================================================
> > > --- security-testing-2.6.orig/include/linux/ima.h
> > > +++ security-testing-2.6/include/linux/ima.h
> > > @@ -20,7 +20,7 @@ extern void ima_inode_free(struct inode 
> > >  extern int ima_path_check(struct path *path, int mask);
> > >  extern void ima_file_free(struct file *file);
> > >  extern int ima_file_mmap(struct file *file, unsigned long prot);
> > > -extern void ima_shm_check(struct file *file);
> > > +extern void ima_opencount_get(struct file *file);
> > >  
> > >  #else
> > >  static inline int ima_bprm_check(struct linux_binprm *bprm)
> > > @@ -53,7 +53,7 @@ static inline int ima_file_mmap(struct f
> > >  	return 0;
> > >  }
> > >  
> > > -static inline void ima_shm_check(struct file *file)
> > > +static inline void ima_opencount_get(struct file *file)
> > >  {
> > >  	return;
> > >  }
> > > Index: security-testing-2.6/ipc/shm.c
> > > ===================================================================
> > > --- security-testing-2.6.orig/ipc/shm.c
> > > +++ security-testing-2.6/ipc/shm.c
> > > @@ -384,7 +384,7 @@ static int newseg(struct ipc_namespace *
> > >  	error = PTR_ERR(file);
> > >  	if (IS_ERR(file))
> > >  		goto no_file;
> > > -	ima_shm_check(file);
> > > +	ima_opencount_get(file);
> > >  
> > >  	id = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni);
> > >  	if (id < 0) {
> > > @@ -891,7 +891,7 @@ long do_shmat(int shmid, char __user *sh
> > >  	file = alloc_file(path.mnt, path.dentry, f_mode, &shm_file_operations);
> > >  	if (!file)
> > >  		goto out_free;
> > > -	ima_shm_check(file);
> > > +	ima_opencount_get(file);
> > >  
> > >  	file->private_data = sfd;
> > >  	file->f_mapping = shp->shm_file->f_mapping;
> > > Index: security-testing-2.6/mm/shmem.c
> > > ===================================================================
> > > --- security-testing-2.6.orig/mm/shmem.c
> > > +++ security-testing-2.6/mm/shmem.c
> > > @@ -2680,7 +2680,7 @@ int shmem_zero_setup(struct vm_area_stru
> > >  	if (IS_ERR(file))
> > >  		return PTR_ERR(file);
> > >  
> > > -	ima_shm_check(file);
> > > +	ima_opencount_get(file);
> > >  	if (vma->vm_file)
> > >  		fput(vma->vm_file);
> > >  	vma->vm_file = file;
> > 
> > Maybe do the IMA operations in (or under) shmem_file_setup() and
> > hugetlb_file_setup()?
> 
> Again, that would hide the real issue of not having done integrity
> (permission) checking.
> 
> Mimi
> 

Re: integrity: nfsd imbalance bug fix

[Mimi Zohar]

On Fri, 2009-05-08 at 14:07 -0400, J. Bruce Fields wrote:
> On Fri, May 08, 2009 at 01:35:20PM -0400, Mimi Zohar wrote:
> > On Wed, 2009-04-29 at 14:18 -0700, Andrew Morton wrote: 
> > > On Tue, 28 Apr 2009 14:32:14 -0400
> > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > 
> > > > The number of calls to ima_path_check()/ima_file_free()
> > > > should be balanced.  An extra call to fput(), indicates
> > > > the file could have been accessed without first being
> > > > measured.
> > > > 
> > > > An nfsd exported file is opened/closed by the kernel
> > > > causing an integrity imbalance message.
> > > > 
> > > > - rename and export opencount_get to ima_opencount_get
> > > > - replace ima_shm_check calls with ima_opencount_get
> > > > - add call to increment opencount for files opened by nfsd.
> > > > - add call to measure exported files in nfsd_permission().
> > > > - export ima_path_check
> > > > 
> > > 
> > > The patch looks fragile to me.  It sprinkles IMA-specific operations
> > > over random unrelated subsystems.  There is hence a decent chance of
> > > breakage in the future.
> > > 
> > > Suppose some other new piece of kernel code opens/closes a file.  The
> > > developer didn't think to add the IMA hooks and whoops, we have a bug.
> > 
> > Agreed.  Exactly for that reason we have the imbalance message. The
> > imbalance message is not a bug per se, but an indication that the
> > file hasn't been integrity (permission) checked.
> 
> Where do I need to go to find out what this integrity checking is?  I
> don't know the first thing about it....
> 
> --b.

I'd start with the patch descriptions http://lkml.org/lkml/2009/2/4/251
and security/integrity/ima/Kconfig, which has a pointer to the initial
paper on IMA. Documentation/ABI/testing/ima_policy contains information
on defining a config file.

Mimi

> > 
> > > It would be really really better if we could add the IMA hooks in a
> > > single place.  That might require the addition of a new function, and
> > > that's fine.  d_instantiate_kernel() or init_file_kernel() or whatever
> > > - that's fine.  It still has the risk that new code will forget to use
> > > the in-kernel variant, but we have a better chance of detecting it.
> > 
> > Definitely. Originally the integrity checking was in inode_permission().
> > But measuring a file requires a dentry and mount point.
> > 
> > I've updated the patch to fix a 'counts' error and divided the patch
> > into two, separating the IMA code changes from the IMA calls in nfsd.
> > 
> > > > 
> > > > Index: security-testing-2.6/fs/nfsd/vfs.c
> > > > ===================================================================
> > > > --- security-testing-2.6.orig/fs/nfsd/vfs.c
> > > > +++ security-testing-2.6/fs/nfsd/vfs.c
> > > > @@ -55,6 +55,7 @@
> > > >  #include <linux/security.h>
> > > >  #endif /* CONFIG_NFSD_V4 */
> > > >  #include <linux/jhash.h>
> > > > +#include <linux/ima.h>
> > > >  
> > > >  #include <asm/uaccess.h>
> > > >  
> > > > @@ -735,6 +736,8 @@ nfsd_open(struct svc_rqst *rqstp, struct
> > > >  			    flags, cred);
> > > >  	if (IS_ERR(*filp))
> > > >  		host_err = PTR_ERR(*filp);
> > > > +	else
> > > > +		ima_opencount_get(*filp);
> > > 
> > > This suggests dentry_open_kernel().
> > 
> > Good name, but making dentry_open_kernel() the default behavior,
> > would mask the real bug - not doing integrity (permission) checking.
> > 
> > > 
> > > >  out_nfserr:
> > > >  	err = nfserrno(host_err);
> > > >  out:
> > > > @@ -2096,7 +2099,13 @@ nfsd_permission(struct svc_rqst *rqstp, 
> > > >  	if (err == -EACCES && S_ISREG(inode->i_mode) &&
> > > >  	    acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE))
> > > >  		err = inode_permission(inode, MAY_EXEC);
> > > > +	if (err)
> > > > +		goto nfsd_out;
> > > >  
> > > > +	err = ima_path_check(&exp->ex_path,
> > > > +			     acc & (MAY_READ | MAY_WRITE | MAY_EXEC));
> > > 
> > > hm, dunno what to do about that.
> > 
> > The real problem is that this belongs in inode_permission(), but an
> > inode is not enough.  It requires a dentry and mount point to
> > measure the file.
> > 
> > > > +	return err;
> > > > +nfsd_out:
> > > >  	return err? nfserrno(err) : 0;
> > > >  }
> > > >  
> > > > Index: security-testing-2.6/security/integrity/ima/ima_main.c
> > > > ===================================================================
> > > > --- security-testing-2.6.orig/security/integrity/ima/ima_main.c
> > > > +++ security-testing-2.6/security/integrity/ima/ima_main.c
> > > > @@ -206,6 +206,7 @@ out:
> > > >  	kref_put(&iint->refcount, iint_free);
> > > >  	return 0;
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(ima_path_check);
> > > >  
> > > >  static int process_measurement(struct file *file, const unsigned char *filename,
> > > >  			       int mask, int function)
> > > > @@ -234,7 +235,16 @@ out:
> > > >  	return rc;
> > > >  }
> > > >  
> > > > -static void opencount_get(struct file *file)
> > > > +/*
> > > > + * ima_opencount_get - incr opencount for files opened by the kernel
> > > > + *
> > > > + * - IPC shm and shmat create/fput a file.
> > > > + * - nfsd opens/closes exported files.
> > > > + *
> > > > + * Increment the opencount for these files to prevent unnecessary
> > > > + * imbalance messages.
> > > > + */
> > > > +void ima_opencount_get(struct file *file)
> > > >  {
> > > >  	struct inode *inode = file->f_dentry->d_inode;
> > > >  	struct ima_iint_cache *iint;
> > > > @@ -248,6 +258,7 @@ static void opencount_get(struct file *f
> > > >  	iint->opencount++;
> > > >  	mutex_unlock(&iint->mutex);
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(ima_opencount_get);
> > > >  
> > > >  /**
> > > >   * ima_file_mmap - based on policy, collect/store measurement.
> > > > @@ -272,18 +283,6 @@ int ima_file_mmap(struct file *file, uns
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -/*
> > > > - * ima_shm_check - IPC shm and shmat create/fput a file
> > > > - *
> > > > - * Maintain the opencount for these files to prevent unnecessary
> > > > - * imbalance messages.
> > > > - */
> > > > -void ima_shm_check(struct file *file)
> > > > -{
> > > > -	opencount_get(file);
> > > > -	return;
> > > > -}
> > > > -
> > > >  /**
> > > >   * ima_bprm_check - based on policy, collect/store measurement.
> > > >   * @bprm: contains the linux_binprm structure
> > > > Index: security-testing-2.6/include/linux/ima.h
> > > > ===================================================================
> > > > --- security-testing-2.6.orig/include/linux/ima.h
> > > > +++ security-testing-2.6/include/linux/ima.h
> > > > @@ -20,7 +20,7 @@ extern void ima_inode_free(struct inode 
> > > >  extern int ima_path_check(struct path *path, int mask);
> > > >  extern void ima_file_free(struct file *file);
> > > >  extern int ima_file_mmap(struct file *file, unsigned long prot);
> > > > -extern void ima_shm_check(struct file *file);
> > > > +extern void ima_opencount_get(struct file *file);
> > > >  
> > > >  #else
> > > >  static inline int ima_bprm_check(struct linux_binprm *bprm)
> > > > @@ -53,7 +53,7 @@ static inline int ima_file_mmap(struct f
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static inline void ima_shm_check(struct file *file)
> > > > +static inline void ima_opencount_get(struct file *file)
> > > >  {
> > > >  	return;
> > > >  }
> > > > Index: security-testing-2.6/ipc/shm.c
> > > > ===================================================================
> > > > --- security-testing-2.6.orig/ipc/shm.c
> > > > +++ security-testing-2.6/ipc/shm.c
> > > > @@ -384,7 +384,7 @@ static int newseg(struct ipc_namespace *
> > > >  	error = PTR_ERR(file);
> > > >  	if (IS_ERR(file))
> > > >  		goto no_file;
> > > > -	ima_shm_check(file);
> > > > +	ima_opencount_get(file);
> > > >  
> > > >  	id = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni);
> > > >  	if (id < 0) {
> > > > @@ -891,7 +891,7 @@ long do_shmat(int shmid, char __user *sh
> > > >  	file = alloc_file(path.mnt, path.dentry, f_mode, &shm_file_operations);
> > > >  	if (!file)
> > > >  		goto out_free;
> > > > -	ima_shm_check(file);
> > > > +	ima_opencount_get(file);
> > > >  
> > > >  	file->private_data = sfd;
> > > >  	file->f_mapping = shp->shm_file->f_mapping;
> > > > Index: security-testing-2.6/mm/shmem.c
> > > > ===================================================================
> > > > --- security-testing-2.6.orig/mm/shmem.c
> > > > +++ security-testing-2.6/mm/shmem.c
> > > > @@ -2680,7 +2680,7 @@ int shmem_zero_setup(struct vm_area_stru
> > > >  	if (IS_ERR(file))
> > > >  		return PTR_ERR(file);
> > > >  
> > > > -	ima_shm_check(file);
> > > > +	ima_opencount_get(file);
> > > >  	if (vma->vm_file)
> > > >  		fput(vma->vm_file);
> > > >  	vma->vm_file = file;
> > > 
> > > Maybe do the IMA operations in (or under) shmem_file_setup() and
> > > hugetlb_file_setup()?
> > 
> > Again, that would hide the real issue of not having done integrity
> > (permission) checking.
> > 
> > Mimi
> > 

Re: integrity: nfsd imbalance bug fix

[Eric Paris]

On Fri, May 8, 2009 at 1:35 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Wed, 2009-04-29 at 14:18 -0700, Andrew Morton wrote:
>> On Tue, 28 Apr 2009 14:32:14 -0400
>> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

>> > Index: security-testing-2.6/mm/shmem.c
>> > ===================================================================
>> > --- security-testing-2.6.orig/mm/shmem.c
>> > +++ security-testing-2.6/mm/shmem.c
>> > @@ -2680,7 +2680,7 @@ int shmem_zero_setup(struct vm_area_stru
>> >     if (IS_ERR(file))
>> >             return PTR_ERR(file);
>> >
>> > -   ima_shm_check(file);
>> > +   ima_opencount_get(file);
>> >     if (vma->vm_file)
>> >             fput(vma->vm_file);
>> >     vma->vm_file = file;
>>
>> Maybe do the IMA operations in (or under) shmem_file_setup() and
>> hugetlb_file_setup()?
>
> Again, that would hide the real issue of not having done integrity
> (permission) checking.

Where are we doing integrity or permission checks on hugetlb or shm
files?   Andrew's right, another example of this problem would be
drivers/gpu/drm/drm_gem.c::drm_get_object_alloc() (at least in
linux-next) which uses shmem_file_setup().  If we aren't doing
integrity checks on shmem already, why do we care about kernel vs.
non-kernel users?

moving the ima_opencount_get() from newseg down a layer into
*_file_setup() would, I think, have taken care of the drm case as
well.....

-Eric