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

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

On Wed, 2011-11-16 at 11:35 +0200, Kasatkin, Dmitry wrote:
> On Wed, Nov 16, 2011 at 1:04 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > 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.
> >
> 
> If the file has been already opened, it has been already verified...

That would imply that everything that is opened for 'read' would have an
iint allocated.  At which point, we might as well go back to using the
inode_alloc hook.

> But the point is probably if that while file is opened read-only, it
> can be opened "write" by some other process.
> It will invalidate verification result and mmap hook would provide
> re-verification.
> 
> Right?
> 
> - Dmitry

Right, so the patch I posted, here, doesn't measure on FILE_PREMMAP, but
only allocates the 'iint' and defers measurement to FILE_MMAP.

Mimi