On Wed, 2011-11-16 at 12:27 -0500, Eric Paris wrote:
> 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.
np, neither am I. I was hoping that there was a better overall
approach. :-( If not, then I'll clean up this patch.
> > 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.
Either way is painful.
> [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?
No, with the right refactoring it's probably unnecessary. In fact, we
could land up with policy consistency errors.
> > 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.
Right
> > +
> > 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?
Ok, thanks for the review.
Mimi
--
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
