linux-kernel - Re: Locking rules for fscrypt_operations->set

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] [day] [month] [year] [list]

Date:   Wed, 21 Sep 2016 17:14:02 -0700
From:   Eric Biggers <ebiggers@...gle.com>
To:     Richard Weinberger <richard@....at>
Cc:     linux-ext4@...r.kernel.org,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Theodore Ts'o <tytso@....edu>,
        David Gstir <david@...ma-star.at>
Subject: Re: Locking rules for fscrypt_operations->set_context()

On Tue, Sep 20, 2016 at 04:30:06PM +0200, Richard Weinberger wrote:
> Hi!
> 
> To my understanding ->setxattr() is always being called with i_mutex held.
> ->set_context() in ext4 stores the security context using ext4_xattr_set(),
> but the fs crypto framework does not lock the inode itself.
> So, depending on the call path, ext4_xattr_set() is sometimes being
> called with i_mutex held and some times not.
> 
> What are the locking rules for fscrypt_operations and especially ->set_context()?

Hi Richard, this is a great question.  I would like to document somewhere the
semantics of each of the fscrypt_operations, but I am still figuring them out
myself.

With regards to ->set_context(), it is called in two distinct situtations:

  (1) when a user process uses FS_IOC_SET_ENCRYPTION_POLICY to set the
      encryption policy on an empty directory

  (2) when an encryption policy is inherited by a newly created file in an
      encrypted directory

In case (1), I think there needs to be an inode_lock() added.  For ext4 and
f2fs, it looks like setting an xattr without inode_lock() isn't problematic by
itself.  Instead, the problem I see is that fscrypt_process_policy() does
several operations, including the ->empty_dir() check, which aren't guaranteed
to be atomic if the directory inode is not locked with inode_lock().

In case (2), I don't think it matters whether inode_lock() is held, since the
inode is still being initialized and is still "locked" in a different way, in
the I_NEW state.  There are also other xattrs being set in __ext4_new_inode(),
seemingly without inode_lock(), which I *think* is fine.

So I am currently thinking that fscrypt_process_policy() should be fixed to do
inode_lock(), and ->set_context() should be documented as a filesystem internal
operation (not necessarily related to ->setxattr()) that is called on either an
inode_lock()-ed inode or on an I_NEW inode.

Eric