diff mbox series

[FOR,STABLE,4.1] ext4 crypto: don't regenerate the per-inode encryption key unnecessarily

Message ID 20171008180953.rpnxvpggqggc334m@thunk.org
State Not Applicable
Headers show
Series [FOR,STABLE,4.1] ext4 crypto: don't regenerate the per-inode encryption key unnecessarily | expand

Commit Message

Theodore Ts'o Oct. 8, 2017, 6:09 p.m. UTC 
This isn't a backport of the upstream commit; rather, it's a complete
rewrite.  An explanation of why is included in the patch description.


From e421d2269a66540cddddfd8d85902e7ecf4605df Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Sun, 8 Oct 2017 13:32:21 -0400
Subject: [PATCH] ext4 crypto: don't regenerate the per-inode encryption key unnecessarily

[ Relevant upstream commit: 1b53cf9815bb4744958d41f3795d5d5a1d365e2d ]

This fixes the same problem as upstream commit 1b53cf9815bb: "fscrypt:
remove broken support for detecting keyring key revocation".
Specifically, key revocations racing with readpage operations will
cause the kernel to crash and burn with a BUG_ON or a NULL pointer
dereference in a block I/O callback stemming from an ext4_readpage()
operation.

This fix is needed to fix prevent xfstests test runs from crashing
while running the generic/421 test.

The root cause is different in the 4.1 kernel, however, since the
4.1's encryption handling is so _primitive_ compared to later kernels.
The code isn't actually explicitly checking for revoked keys.
Instead, the code is neededly regenerating the per-file encryption key
on every mmap() or open() or directory operation (in the case of a
directory inode).  Yelch!

If the file is already opened and actively being read, and there is a
racing open() after the user's master key has been revoked, there will
be the same net effect as the problem fixed by upstream commit
1b53cf9815bb --- the per-file key will be marked as invalid and this
will cause a BUG_ON.

In the AOSP 3.18 and 4.4 android-common kernels, the more modern
version of ext4 encryption have been backported, including a backport
of upstream commit 1b53cf9815bb.  This is a dozen plus commits, and
isn't really suitable for the Upstream LTS kernel.  So instead, this
is the simplest bug which fixes the same high-level issue as the
upstream commit, without dragging in all of the other non-bug fixes
improvements to the ext4 encryption code found in newer kernels.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/crypto_key.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Eric Biggers Oct. 8, 2017, 7:17 p.m. UTC | #1 
Hi Ted,

On Sun, Oct 08, 2017 at 02:09:53PM -0400, Theodore Ts'o wrote:
> diff --git a/fs/ext4/crypto_key.c b/fs/ext4/crypto_key.c
> index 52170d0b7c40..c8c3bf0286be 100644
> --- a/fs/ext4/crypto_key.c
> +++ b/fs/ext4/crypto_key.c
> @@ -99,9 +99,14 @@ int ext4_generate_encryption_key(struct inode *inode)
>  	struct ext4_encryption_context ctx;
>  	struct user_key_payload *ukp;
>  	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> -	int res = ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION,
> -				 EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
> -				 &ctx, sizeof(ctx));
> +	int res;
> +
> +	if (ext4_has_encryption_key(inode))
> +		return 0;
> +
> +	res = ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION,
> +			     EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
> +			     &ctx, sizeof(ctx));
>  
>  	if (res != sizeof(ctx)) {
>  		if (res > 0)

This is an improvement, but it's still broken because there is no locking.  If
two processes ->open() the same inode at the same time, one can observe
ext4_has_encryption_key() as true before the key has been fully initialized.  It
may be a good idea to add a really simple
DEFINE_MUTEX(ext4_key_generation_mutex), then do the ext4_has_encryption_key()
check and key generation under the mutex...

Eric
Theodore Ts'o Oct. 8, 2017, 8:34 p.m. UTC | #2 
On Sun, Oct 08, 2017 at 12:17:49PM -0700, Eric Biggers wrote:
g> 
> This is an improvement, but it's still broken because there is no locking.  If
> two processes ->open() the same inode at the same time, one can observe
> ext4_has_encryption_key() as true before the key has been fully initialized.  It
> may be a good idea to add a really simple
> DEFINE_MUTEX(ext4_key_generation_mutex), then do the ext4_has_encryption_key()
> check and key generation under the mutex...

Thanks for the review.  You're right, we need to add a mutex because
otherwise this patch would cause a regression.  Previously, a racing
open would "only" leak kernel memory.  Now, it could cause a kernel
crash.  I'll respin a patch with the mutex added.

At this point, looking at 4.1, I can't really recommend that _anyone_
try to use ext4 encryption with the 4.1 LTS kernel.  Fortunately, I
don't think there are that many users of 4.1, and those that do
probably aren't using ext4 encryption.  (The Android ecosystem seems
to have largely settled on 3.18 and 4.4, and there hasn't been much
use of ext4 encryption outside of Android yet.)  I just want to make
sure keep the latest upstream LTS kernels mostly clean with respect to
xfstests runs, so I more easily detect regressions.

Speaking of which, over the weekend I've sent a dozen or so stable
kernel backport/cherry pick requests to stable@vger.kernel.org.  Most
of them I didn't bother to cc linux-ext4 since they were minor changes
to allow the patches to apply and I decided not to spam the list with
the backports.  I cc'ed the patch in this thread because it was new
code, and I'm glad I did, since your review was very helpful.  If
folks are interested the backports, I can either send them all to the
list, or I can publish them on git.kernel.org.  Hopefully they will
all show up in the LTS stable review process and then into a LTS
kernel in the next month or so.

Also, there are a number of the backports which are needed if you want
to be able to run xfstests without tests causing older kernels
(especialy 3.18 and 4.1) to crash or hang in the middle of the test
run.  So if anyone else is interested in running xfstests against
older kernels before these patches land in the LTS kernels, let me
know.  (I still have one known problem which still needs to be fixed,
which is that generic/269 will cause 4.4 and earlier kernels to soft
lockup.)

Cheers,

					- Ted
Eric Biggers Oct. 8, 2017, 9:52 p.m. UTC | #3 
On Sun, Oct 08, 2017 at 04:34:50PM -0400, Theodore Ts'o wrote:
> 
> At this point, looking at 4.1, I can't really recommend that _anyone_
> try to use ext4 encryption with the 4.1 LTS kernel.  Fortunately, I
> don't think there are that many users of 4.1, and those that do
> probably aren't using ext4 encryption.  (The Android ecosystem seems
> to have largely settled on 3.18 and 4.4, and there hasn't been much
> use of ext4 encryption outside of Android yet.)  I just want to make
> sure keep the latest upstream LTS kernels mostly clean with respect to
> xfstests runs, so I more easily detect regressions.

Yes, I generally haven't been backporting ext4 encryption fixes to 4.1 because
ext4 encryption is very broken in that kernel, the code is only reachable with
CONFIG_EXT4_FS_ENCRYPTION=y, everyone seemed to be using 3.18 or 4.4 instead,
and the ext4 encryption code changed a lot between 4.1 and 4.4.  But yes, it's
nice at least making it so that xfstests doesn't crash the kernel.  We should
probably also backport 9a200d075e5d ("ext4: require encryption feature for
EXT4_IOC_SET_ENCRYPTION_POLICY") so that if someone happens to have
CONFIG_EXT4_FS_ENCRYPTION turned on in their kernel, random unprivileged users
can't reach all the buggy code on any ext4 filesystem.

By the way, just looking at ext4_generate_encryption_key() in 4.1, there are
three different bugs in how it's accessing the keyring, and all allow an
unprivileged user to cause a kernel oops.  The first two were fixed upstream by
687c3c36e754 ("ext4 crypto: replace some BUG_ON()'s with error checks") and
db7730e3091a ("ext4 crypto: add missing locking for keyring_key access").  The
last will be fixed by "fscrypt: fix dereference of NULL user_key_payload" which
still needs to be applied.  (It's part of a series which fixes the same bug in 5
different places; I'll probably need to resend the patches individually to the
different maintainers unless the keyrings maintainer wants to take the whole
series.)

Eric
Theodore Ts'o Oct. 9, 2017, 12:02 a.m. UTC | #4 
On Sun, Oct 08, 2017 at 02:52:16PM -0700, Eric Biggers wrote:
> But yes, it's
> nice at least making it so that xfstests doesn't crash the kernel.  We should
> probably also backport 9a200d075e5d ("ext4: require encryption feature for
> EXT4_IOC_SET_ENCRYPTION_POLICY") so that if someone happens to have
> CONFIG_EXT4_FS_ENCRYPTION turned on in their kernel, random unprivileged users
> can't reach all the buggy code on any ext4 filesystem.

Yeah, that's a good idea.  I'll send a request to
stable@vger.kernel.org.  Commit 9a200d075e5d applies cleanly to 4.1,
so that should be straightforward.

> By the way, just looking at ext4_generate_encryption_key() in 4.1, there are
> three different bugs in how it's accessing the keyring, and all allow an
> unprivileged user to cause a kernel oops.  The first two were fixed upstream by
> 687c3c36e754 ("ext4 crypto: replace some BUG_ON()'s with error checks") and
> db7730e3091a ("ext4 crypto: add missing locking for keyring_key access").  The
> last will be fixed by "fscrypt: fix dereference of NULL user_key_payload" which
> still needs to be applied.  (It's part of a series which fixes the same bug in 5
> different places; I'll probably need to resend the patches individually to the
> different maintainers unless the keyrings maintainer wants to take the whole
> series.)

Well, I'm not particulary motivated to backport those fixes to 4.1,
but if you or someone else wants to do the backport, I certainly won't
complain.

Personally, though, if I have more time to work on the LTS kernels,
cleaning up the 3.18.x test failures is higher on my priority list,
since it might actually help some Android devices still stuck on 3.18.
(See below for the current state of the LTS kernels with my backports
and fixups to xfstests to fix some false positives due to the older
kernels not supporting ea_inode and project quotas.)

Of the failures in 4.9 and 4.4, generic/082 and generic/338 are
relatively minor quota bugs that aren't going to affect Android's use
of quota.  generic/441 tests the new error handling code, and we're
not likely to backport that to LTS kernels.  generic/451 is a buffered
vs. AIO/DIO race test, which I suspect won't be worth fixing except by
changing the Direct I/O paths to uses the new iomap infrastructure.
generic/456 will be fixed in the next merge window and backported to
the LTS kernels.  And generic/459 is a dm-thin problem.

So the only real problem children is 3.18 and 4.1, and my motivation
to clean up 4.1 is minimal.

      	    	       	      - Ted

KERNEL:    kernel 4.9.52-xfstests-00005-gc9dacd33bdcf #4 SMP Sun Oct 8 15:47:09 EDT 2017 x86_64
CMDLINE:   --kernel /build/ext4-4.9-64 -c 4k -g auto
CPUS:      2
MEM:       7680

ext4/4k: 339 tests, 5 failures, 38 skipped, 3024 seconds
  Failures: generic/082 generic/441 generic/451 generic/456
    generic/459
Totals: 339 tests, 38 skipped, 5 failures, 0 errors, 2932s


KERNEL:    kernel 4.4.89-xfstests-00004-g85c293dc2ed8 #5 SMP Sun Oct 8 15:55:01 EDT 2017 x86_64
CMDLINE:   --kernel /build/ext4-4.4-64 -c 4k -g auto -X generic/269

ext4/4k: 338 tests, 5 failures, 49 skipped, 3361 seconds
  Failures: generic/382 generic/441 generic/451 generic/456
    generic/459
Totals: 338 tests, 49 skipped, 5 failures, 0 errors, 3261s


KERNEL:    kernel 4.1.44-xfstests-00009-gd899706494c1 #6 SMP Sun Oct 8 16:38:13 EDT 2017 x86_64
CMDLINE:   --kernel /build/ext4-4.1-64 -c 4k -g auto -X generic/269

ext4/4k: 338 tests, 14 failures, 55 skipped, 3353 seconds
  Failures: ext4/022 ext4/025 generic/313 generic/382 generic/395
    generic/397 generic/398 generic/429 generic/435 generic/440
    generic/441 generic/451 generic/456 generic/459
Totals: 338 tests, 55 skipped, 14 failures, 0 errors, 3270s


KERNEL:    kernel 3.18.73-xfstests-00006-gc80a32d5a966 #2 SMP Sun Oct 8 16:35:31 EDT 2017 x86_64
CMDLINE:   --kernel /build/ext4-3.18-64 -c 4k -g auto -X generic/269 -X ext4/022

ext4/4k: 337 tests, 9 failures, 67 skipped, 3254 seconds
  Failures: ext4/001 ext4/025 generic/313 generic/382 generic/441
    generic/450 generic/451 generic/456 generic/459
Totals: 337 tests, 67 skipped, 9 failures, 0 errors, 3178s
diff mbox series

Patch

diff --git a/fs/ext4/crypto_key.c b/fs/ext4/crypto_key.c
index 52170d0b7c40..c8c3bf0286be 100644
--- a/fs/ext4/crypto_key.c
+++ b/fs/ext4/crypto_key.c
@@ -99,9 +99,14 @@  int ext4_generate_encryption_key(struct inode *inode)
 	struct ext4_encryption_context ctx;
 	struct user_key_payload *ukp;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
-	int res = ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION,
-				 EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
-				 &ctx, sizeof(ctx));
+	int res;
+
+	if (ext4_has_encryption_key(inode))
+		return 0;
+
+	res = ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION,
+			     EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
+			     &ctx, sizeof(ctx));
 
 	if (res != sizeof(ctx)) {
 		if (res > 0)