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?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
