On Wed, Nov 16, 2011 at 1:04 AM, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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... 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 > 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 > -- 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
