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

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

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