From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751916AbdJBDZh convert rfc822-to-8bit (ORCPT ); Sun, 1 Oct 2017 23:25:37 -0400 Received: from out03.mta.xmission.com ([166.70.13.233]:59333 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750943AbdJBDZe (ORCPT ); Sun, 1 Oct 2017 23:25:34 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Mimi Zohar Cc: Dave Chinner , Linus Torvalds , LSM List , linux-fsdevel , Christoph Hellwig , "Theodore Ts'o" , Jan Kara , Linux Kernel Mailing List , linux-integrity@vger.kernel.org, Sascha Hauer References: <20170928220215.GC15067@dastard> <1506643967.5691.46.camel@linux.vnet.ibm.com> <1506649980.5691.100.camel@linux.vnet.ibm.com> <87mv5blki7.fsf@xmission.com> <1506859691.5691.211.camel@linux.vnet.ibm.com> <20171001223402.GG15067@dastard> <1506901362.5691.247.camel@linux.vnet.ibm.com> Date: Sun, 01 Oct 2017 22:25:18 -0500 In-Reply-To: <1506901362.5691.247.camel@linux.vnet.ibm.com> (Mimi Zohar's message of "Sun, 01 Oct 2017 19:42:42 -0400") Message-ID: <87zi9ai63l.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT X-XM-SPF: eid=1dyrMF-0000fN-SK;;;mid=<87zi9ai63l.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=67.3.200.44;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18w0FDV3HQrGdEBDXSDh2qdBYzpja9/fbc= X-SA-Exim-Connect-IP: 67.3.200.44 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.7 XMSubLong Long Subject * 1.5 XMNoVowels Alpha-numberic number with no vowels * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa01 1397; Body=1 Fuz1=1 Fuz2=1] * 0.2 T_XMDrugObfuBody_14 obfuscated drug references * 0.0 T_TooManySym_01 4+ unique symbols in subject X-Spam-DCC: XMission; sa01 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: **;Mimi Zohar X-Spam-Relay-Country: X-Spam-Timing: total 887 ms - load_scoreonly_sql: 0.05 (0.0%), signal_user_changed: 3.2 (0.4%), b_tie_ro: 2.1 (0.2%), parse: 1.58 (0.2%), extract_message_metadata: 7 (0.8%), get_uri_detail_list: 4.5 (0.5%), tests_pri_-1000: 7 (0.8%), tests_pri_-950: 2.1 (0.2%), tests_pri_-900: 1.70 (0.2%), tests_pri_-400: 44 (5.0%), check_bayes: 42 (4.7%), b_tokenize: 17 (1.9%), b_tok_get_all: 11 (1.2%), b_comp_prob: 6 (0.7%), b_tok_touch_all: 3.4 (0.4%), b_finish: 0.83 (0.1%), tests_pri_0: 790 (89.1%), check_dkim_signature: 0.95 (0.1%), check_dkim_adsp: 4.7 (0.5%), tests_pri_500: 7 (0.8%), rewrite_mail: 0.00 (0.0%) Subject: Re: [RFC PATCH 3/3] fs: detect that the i_rwsem has already been taken exclusively X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Mimi Zohar writes: > On Mon, 2017-10-02 at 09:34 +1100, Dave Chinner wrote: >> On Sun, Oct 01, 2017 at 11:41:48AM -0700, Linus Torvalds wrote: >> > On Sun, Oct 1, 2017 at 5:08 AM, Mimi Zohar wrote: >> > > >> > > Right, re-introducing the iint->mutex and a new i_generation field in >> > > the iint struct with a separate set of locks should work. It will be >> > > reset if the file metadata changes (eg. setxattr, chown, chmod). >> > >> > Note that the "inner lock" could possibly be omitted if the >> > invalidation can be just a single atomic instruction. >> > >> > So particularly if invalidation could be just an atomic_inc() on the >> > generation count, there might not need to be any inner lock at all. >> > >> > You'd have to serialize the actual measurement with the "read >> > generation count", but that should be as simple as just doing a >> > smp_rmb() between the "read generation count" and "do measurement on >> > file contents". >> >> We already have a change counter on the inode, which is modified on >> any data or metadata write (i_version) under filesystem locks. The >> i_version counter has well defined semantics - it's required by >> NFSv4 to increment on any metadata or data change - so we should be >> able to rely on it's behaviour to implement IMA as well. Filesystems >> that support i_version are marked with [SB|MS]_I_VERSION in the >> superblock (IS_I_VERSION(inode)) so it should be easy to tell if IMA >> can be supported on a specific filesystem (btrfs, ext4, fuse and xfs >> ATM). > > Recently I received a patch to replace i_version with mtime/atime. >  Now, even more recently, I received a patch that claims that > i_version is just a performance improvement.  For file systems that > don't support i_version, assume that the file has changed. > > For file systems that don't support i_version, instead of assuming > that the file has changed, we can at least use i_generation. > > With Linus' suggested changes, I think this will work nicely. > >> The IMA code should be able to sample that at measurement time and >> either fail or be retried if i_version changes during measurement. >> We can then simply make the IMA xattr write conditional on the >> i_version value being unchanged from the sample the IMA code passes >> into the filesystem once the filesystem holds all the locks it needs >> to write the xattr... > >> I note that IMA already grabs the i_version in >> ima_collect_measurement(), so this shouldn't be too hard to do. >> Perhaps we don't need any new locks or counterst all, maybe just >> the ability to feed a version cookie to the set_xattr method? > > The security.ima xattr is normally written out in > ima_check_last_writer(), not in ima_collect_measurement(). >  ima_collect_measurement() calculates the file hash for storing in the > measurement list (IMA-measurement), verifying the hash/signature (IMA- > appraisal) already stored in the xattr, and auditing (IMA-audit). > > The only time that ima_collect_measurement() writes the file xattr is > in "fix" mode.  Writing the xattr will need to be deferred until after > the iint->mutex is released. > > There should be no open writers in ima_check_last_writer(), so the > file shouldn't be changing. This is slightly tangential but I think important to consider. What do you do about distributed filesystems fuse, nfs, etc that can change the data behind the kernels back. Do you not support such systems or do you have a sufficient way to detect changes? Eric