Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Eric Biggers <ebiggers@kernel.org>
Cc: linux-fscrypt@vger.kernel.org, linux-ext4@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-mtd@lists.infradead.org, linux-fsdevel@vger.kernel.org,
	linux-crypto@vger.kernel.org, linux-api@vger.kernel.org,
	keyrings@vger.kernel.org, Satya Tangirala <satyat@google.com>,
	Paul Crowley <paulcrowley@google.com>
Subject: Re: [RFC PATCH v2 11/20] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl
Date: Tue, 12 Feb 2019 09:12:49 +1100
Message-ID: <20190211221249.GH20493@dastard> (raw)
In-Reply-To: <20190211172738.4633-12-ebiggers@kernel.org>

On Mon, Feb 11, 2019 at 09:27:29AM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Add a new fscrypt ioctl, FS_IOC_REMOVE_ENCRYPTION_KEY.  This ioctl
> removes an encryption key that was added by FS_IOC_ADD_ENCRYPTION_KEY.
> It wipes the secret key itself, then "locks" the encrypted files and
> directories that had been unlocked using that key -- implemented by
> evicting the relevant dentries and inodes from the VFS caches.
> 
> The problem this solves is that many fscrypt users want the ability to
> remove encryption keys, causing the corresponding encrypted directories
> to appear "locked" (presented in ciphertext form) again.  Moreover,
> users want removing an encryption key to *really* remove it, in the
> sense that the removed keys cannot be recovered even if kernel memory is
> compromised, e.g. by the exploit of a kernel security vulnerability or
> by a physical attack.  This is desirable after a user logs out of the
> system, for example.  In many cases users even already assume this to be
> the case and are surprised to hear when it's not.
> 
> It is not sufficient to simply unlink the master key from the keyring
> (or to revoke or invalidate it), since the actual encryption transform
> objects are still pinned in memory by their inodes.  Therefore, to
> really remove a key we must also evict the relevant inodes.
> 
> Currently one workaround is to run 'sync && echo 2 >
> /proc/sys/vm/drop_caches'.  But, that evicts all unused inodes in the
> system rather than just the inodes associated with the key being
> removed, causing severe performance problems.  Moreover, it requires
> root privileges, so regular users can't "lock" their encrypted files.
> 
> Another workaround, used in Chromium OS kernels, is to add a new
> VFS-level ioctl FS_IOC_DROP_CACHE which is a more restricted version of
> drop_caches that operates on a single super_block.  It does:
> 
>         shrink_dcache_sb(sb);
>         invalidate_inodes(sb, false);
> 
> But it's still a hack.  Yet, the major users of filesystem encryption
> want this feature badly enough that they are actually using these hacks.
> 
> To properly solve the problem, start maintaining a list of the inodes
> which have been "unlocked" using each master key.  Originally this
> wasn't possible because the kernel didn't keep track of in-use master
> keys at all.  But, with the ->s_master_keys keyring it is now possible.
> 
> Then, add an ioctl FS_IOC_REMOVE_ENCRYPTION_KEY.  It finds the specified
> master key in ->s_master_keys, then wipes the secret key itself, which
> prevents any additional inodes from being unlocked with the key.  Then,
> it syncs the filesystem and evicts the inodes in the key's list.  The
> normal inode eviction code will free and wipe the per-file keys (in
> ->i_crypt_info).  Note that freeing ->i_crypt_info without evicting the
> inodes was also considered, but would have been racy.

The solution is still so gross. Exporting all the inode cache
internal functions so you can invalidate an external list of inodes
is, IMO, not an appropriate solution for anything.

Indeed, this is exactly what ->drop_inode() is for.

Take this function:

> +static void evict_dentries_for_decrypted_inodes(struct fscrypt_master_key *mk)
> +{
> +	struct fscrypt_info *ci;
> +	struct inode *inode;
> +	struct inode *toput_inode = NULL;
> +
> +	spin_lock(&mk->mk_decrypted_inodes_lock);
> +
> +	list_for_each_entry(ci, &mk->mk_decrypted_inodes, ci_master_key_link) {
> +		inode = ci->ci_inode;
> +		spin_lock(&inode->i_lock);
> +		if (inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW)) {
> +			spin_unlock(&inode->i_lock);
> +			continue;
> +		}
> +		__iget(inode);
> +		spin_unlock(&inode->i_lock);
> +		spin_unlock(&mk->mk_decrypted_inodes_lock);
> +
> +		shrink_dcache_inode(inode);
> +		iput(toput_inode);
> +		toput_inode = inode;
> +
> +		spin_lock(&mk->mk_decrypted_inodes_lock);
> +	}
> +
> +	spin_unlock(&mk->mk_decrypted_inodes_lock);
> +	iput(toput_inode);
> +}

It takes a new reference to each decrypted inode, and then drops it
again after all the dentry cache references have been killed and
we've got a reference to the next inode in the list.  Killing the
dentry references to the inode means it should only have in-use
references and the reference this function holds on it.

If the inode is not in use then there will be only one, and so it
will fall into iput_final() and the ->drop_inode() function
determines if the inode should be evicted from the cache and
destroyed immediately.  IOWs, implement fscrypt_drop_inode() to do
the right thing when the key has been destroyed, and you can get rid
of all this crazy inode cache walk-and-invalidate hackery.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-11 17:27 [RFC PATCH v2 00/20] fscrypt: key management improvements Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 01/20] fs, fscrypt: move uapi definitions to new header <linux/fscrypt.h> Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 02/20] fscrypt: use FSCRYPT_ prefix for uapi constants Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 03/20] fscrypt: use FSCRYPT_* definitions, not FS_* Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 04/20] fs: add ->s_master_keys to struct super_block Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 05/20] fscrypt: add ->ci_inode to fscrypt_info Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 06/20] fscrypt: refactor v1 policy key setup into keysetup_legacy.c Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 07/20] fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 08/20] fs/inode.c: export inode_lru_list_del() Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 09/20] fs/inode.c: rename and export dispose_list() Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 10/20] fs/dcache.c: add shrink_dcache_inode() Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 11/20] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl Eric Biggers
2019-02-11 22:12   ` Dave Chinner [this message]
2019-02-11 23:31     ` Eric Biggers
2019-02-12  0:03       ` Dave Chinner
2019-02-11 17:27 ` [RFC PATCH v2 12/20] fscrypt: add FS_IOC_GET_ENCRYPTION_KEY_STATUS ioctl Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 13/20] fscrypt: add an HKDF-SHA512 implementation Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 14/20] fscrypt: v2 encryption policy support Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 15/20] fscrypt: allow unprivileged users to add/remove keys for v2 policies Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 16/20] fscrypt: require that key be added when setting a v2 encryption policy Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 17/20] ext4: wire up new fscrypt ioctls Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 18/20] f2fs: " Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 19/20] ubifs: " Eric Biggers
2019-02-11 17:27 ` [RFC PATCH v2 20/20] fscrypt: document the new ioctls and policy version Eric Biggers

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190211221249.GH20493@dastard \
    --to=david@fromorbit.com \
    --cc=ebiggers@kernel.org \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=paulcrowley@google.com \
    --cc=satyat@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org linux-fsdevel@archiver.kernel.org
	public-inbox-index linux-fsdevel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox