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

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

On Tue, 2011-11-15 at 19:05 +0200, Kasatkin, Dmitry wrote:
> On Tue, Nov 15, 2011 at 4:44 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > 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.
> >
> 
> As you are asking for possible alternative solution,
> I think I might have one.
> 
> It could possibly done in such away:
> 
> When binaries or executables are opened for mmap or bprm,
> kernel sets open_flag |= __FMODE_EXEC;
> 
> ima_file_check() could have additional parameter: op->open_flag
> and implementation could selection a function as:
> int function = (flag & __FMODE_EXEC) ? BPRM_CHECK : FILE_CHECK;
> 
> IMA policy has the same entries for BPRM_CHECK or FILE_MMAP.
> 
> This can possibly make mmap and bprm hooks redundant.
> 
> - Dmitry

As a file can be opened read only and then mmapped executable, it is
impossible to know on open, whether that file will be mmapped
executable.

Mimi