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

Eric Biggers
2017-10-08T21:52:18Z

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