linux-kernel - Re: [PATCH] ima: prevent dead lock when a file is opened for direct io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130226203408.GJ4503@ZenIV.linux.org.uk>
Date:	Tue, 26 Feb 2013 20:34:08 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:	linux-security-module <linux-security-module@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ima: prevent dead lock when a file is opened for direct
 io

On Tue, Feb 26, 2013 at 02:32:08PM -0500, Mimi Zohar wrote:
> Before anything gets access to the file, the file needs to be measured,
> appraised, and/or audited, based on policy.  If IMA-appraisal is enabled
> and the file is in policy, we enforce local file integrity and return
> -EINVAL on failure, similar to LSMs.
> 
> Appraising the file is a two step process.  Before appraising the file
> data's integrity, we verify the integrity of the file metadata. Included
> in the 'security.evm' calculation is the ino, generation, uid, gid,
> mode, uuid, and the security xattrs.  'security.ima' contains the file
> data hash or a signature based on the hash.
> 
> The i_mutex is held when making file metadata changes (eg. xattrs,
> chmod, ...).  We hold the i_mutex through the entire verification,
> preventing the file data/metadata from changing.

->i_mutex is *not* guaranteed to prevent file data changes.  It does
cover metadata, but that's it.  ->write() is not required to take it.
Note, BTW, that as soon as you've dropped ->i_mutex, the metadata can
be changed by somebody else.

What do you achieve by holding it over the vfs_read() call?

> I guess I wasn't clear here.  IMA always takes the i_mutex, regardless
> of the O_DIRECT flag.  When a file is opened for read,
> process_measurement() takes the i_mutex and then, if the file was opened
> with the O_DIRECT flag, do_blockdev_direct_IO() attempts to take the
> i_mutex again, causing the lockdep.

*sigh*
Do you actually disagree with my description of the locking rules you
implicitly rely upon?  Suppose wankfs_file_read() happens to grab
->i_mutex for some reason; without IMA it used to be perfectly legitimate.
With IMA it will deadlock as soon as IMA decides that such file is worth
its attention.  So these days the rule has (silently) become
	* ->read() on a regular file is not allowed to touch ->i_mutex
and with your proposed change it becomes (still undocumented)
	* ->read() on a regular file is not allowed to touch ->i_mutex unless
O_DIRECT is present in file flags at the moment of ->read()
Correct?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/