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]

Date:	Tue, 26 Feb 2013 14:32:08 -0500
From:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
To:	Al Viro <viro@...IV.linux.org.uk>
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, 2013-02-26 at 16:20 +0000, Al Viro wrote:
> On Wed, Feb 20, 2013 at 04:27:51PM -0500, Mimi Zohar wrote:
> > Hi Al,
> > 
> > Are there any negative repercussions to temporarily removing the
> > o_direct flag in order to calculate the file hash?
> > 
> > thanks,
> > 
> > Mimi
> > -----
> > 
> > Files are measured or appraised based on the IMA policy.  When a file
> > in policy is opened for read with the O_DIRECT flag set, a deadlock
> > occurs due to do_blockdev_direct_IO() taking i_mutex before calling
> > filemap_write_and_wait_range(). The i_mutex was previously taken in
> > process_measurement().  This patch temporarily removes the O_DIRECT
> > flag in order to calculate the hash and restores it once completed.
> 
> 	Why does process_measurement() hold ->i_mutex across that?

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.

> It really sounds like "we kinda hope no ->read() will take ->i_mutex,
> oops, at least one case does, umm... let's kludge around a bit and
> hope no other case shows up".  
> 
> Locking rules should be documented and they should make sense.  You are
> introducing a new one and it's really convoluted - "no ->read() instance
> for a regular file shall take ->i_mutex unless it's an O_DIRECT open".

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.

Other than fiddling with the O_DIRECT flag, do you have any other
suggestions?

thanks,

Mimi

--
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/