Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Eric Paris <eparis@redhat.com>]

On Tue, 2011-11-15 at 07:31 -0500, Mimi Zohar wrote:
> The circular lockdep is caused by allocating the 'iint' for mmapped
> files.  Originally when an 'iint' was allocated for every inode
> in inode_alloc_security(), before the inode was accessible, no
> locking was necessary.  Commits bc7d2a3e and 196f518 changed this
> behavior and allocated the 'iint' on a per need basis, resulting in
> the mmap_sem being taken before the i_mutex for mmapped files.
> 
> Possible unsafe locking scenario:
>        CPU0                    CPU1
>        ----                    ----
> lock(&mm->mmap_sem);
>                               lock(&sb->s_type->i_mutex_key);
>                               lock(&mm->mmap_sem);
> lock(&sb->s_type->i_mutex_key);
> 
> This patch adds a new hook ima_file_premmap() to pre-allocate the
> iint, preventing the i_mutex being taken after the mmap_sem, and
> defines a do_mmap() helper function do_mmap_with_sem().
> 
> Before making this sort of change throughout, perhaps someone sees
> a better option?

The idea is ok, but I'm not a fan of the patch itself.

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 3dc3a8c..bf8da47 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1417,6 +1417,11 @@ out:
>  	return ret;
>  }
>  
> +extern unsigned long do_mmap_with_sem(struct file *file, unsigned long addr,
> +	unsigned long len, unsigned long prot,
> +	unsigned long flag, unsigned long offset,
> +	struct rw_semaphore *mmap_sem);
> +
>  extern int do_munmap(struct mm_struct *, unsigned long, size_t);
>  
>  extern unsigned long do_brk(unsigned long, unsigned long);

I don't like the new helper.  I'd much rather just sprinkle
ima_file_premmap() all over the place.  Anything that hides locking
deeper makes me sad.

[snip]
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 3ccf7ac..80819aa 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -114,7 +114,7 @@ struct integrity_iint_cache *integrity_iint_insert(struct inode *inode);
>  struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>  
>  /* IMA policy related functions */
> -enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
> +enum ima_hooks { FILE_CHECK = 1, FILE_PREMMAP, FILE_MMAP, BPRM_CHECK };

Really don't like this.  Do we really need to extend the language rules
to support this?

>  int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask);
>  void ima_init_policy(void);
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 1eff5cb..1df7ede 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -140,6 +140,9 @@ retry:
>  		return rc;
>  	}
>  
> +	if (function == FILE_PREMMAP)	/* defer to FILE_MMAP */
> +		return 0;

Lets just break the beginning of this function off into its own helper
function which you use in ima_pre_mmap as well.

> +
>  	mutex_lock(&iint->mutex);
>  
>  	rc = iint->flags & IMA_MEASURED ? 1 : 0;
> @@ -153,6 +156,30 @@ out:
>  	mutex_unlock(&iint->mutex);
>  	return rc;
>  }
> + 
> +/**
> + * ima_file_premmap - based on policy allocate the 'iint'
> + * @file: pointer to the file to be measured (May be NULL)
> + * @prot: contains the protection that will be applied by the kernel.
> + *
> + * Based on the measurement policy, pre-allocate the iint before the
> + * mmap_sem is taken, but defer the actual measurement until
> + * security_file_mmap().
> + *
> + * (Pre-allocating the iint, prevents the i_mutex being taken after the
> + * mmap_sem.)
> + */
> +int ima_file_premmap(struct file *file, unsigned long prot)
> +{
> +	int rc;
> +
> +	if (!file)
> +		return 0;
> +	if (prot & PROT_EXEC)
> +		rc = process_measurement(file, file->f_dentry->d_name.name,
> +					 MAY_EXEC, FILE_PREMMAP);
> +	return 0;
> +}

Here lets call the helper above, but instead of FILE_PREMMAP, lets use
the correct FILE_MMAP or FILE_BPRM, which is going to have to come as a
third argument, right?