From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mimi Zohar Subject: Re: [RFC][PATCH] ima: fix lockdep circular locking dependency Date: Tue, 15 Nov 2011 18:04:47 -0500 Message-ID: <1321398287.2002.14.camel@falcor> References: <1321360287-6345-1-git-send-email-zohar@linux.vnet.ibm.com> <1321368273.3780.41.camel@falcor> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: linux-security-module@vger.kernel.org, Eric Paris , linux-fsdevel@vger.kernel.org To: "Kasatkin, Dmitry" Return-path: Received: from e31.co.us.ibm.com ([32.97.110.149]:38280 "EHLO e31.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753897Ab1KOXHm (ORCPT ); Tue, 15 Nov 2011 18:07:42 -0500 Received: from /spool/local by e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 15 Nov 2011 16:07:41 -0700 In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, 2011-11-15 at 19:05 +0200, Kasatkin, Dmitry wrote: > On Tue, Nov 15, 2011 at 4:44 PM, Mimi Zohar wrote: > > On Tue, 2011-11-15 at 16:17 +0200, Kasatkin, Dmitry wrote: > >> On Tue, Nov 15, 2011 at 2:31 PM, 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? > >> > > >> > >> 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