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

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

On Tue, 2011-11-15 at 16:17 +0200, Kasatkin, Dmitry wrote:
> On Tue, Nov 15, 2011 at 2:31 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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?
> >
> 
> Hi,
> 
> After a bit of thinking I remembered that I have seen ima hooks are
> called for the same file...
> i have done call tracing again and found out that.
> 
> FILE_CHECK is ALWAYS called before FILE_MMAP or BPRM_CHECK.
> 
> So when 2 above are called, file is already verified..
> Indeed, in both cases before mmap or exec,  the file is opened with
> do_filp_open().
> 
> Are these completely useless then?
> FILE_MMAP or BPRM_CHECK
> 
> - Dmitry

There are a couple of reasons for deferring IMA processing until
BPRM_CHECK/FILE_MMAP:
- Defer processing until the file has been locked and won't be modified
- Different policies can be associated with the different hooks

For example, with the ima_tcb policy, only files opened for read by root
are measured at file_check, but all files mmapped executable are
measured at file_mmap.  So although a file is opened before it is
mmapped, we don't know apriori if it will be mmapped.  We could allocate
the iint for all inodes opened for read, but that would kind of defeat
the purpose of dynamically allocating the iint as needed.

thanks,

Mimi