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:   Mon, 4 Nov 2019 15:45:03 +0800
From:   Chao Yu <>
To:     Doug Anderson <>,
        Gwendal Grignou <>,
        Chao Yu <>,
        Ryo Hashimoto <>,
        Vadim Sukhomlinov <>,
        "Guenter Roeck" <>, <>,
        Andreas Dilger <>,
        "Theodore Y. Ts'o" <>,
        Jonathan Corbet <>,
        LKML <>,
        Jaegeuk Kim <>,
        <>, <>,
Subject: Re: [PATCH] Revert "ext4 crypto: fix to check feature status before
 get policy"

Sorry to introduce such issue... :(

On 2019/10/31 3:02, Eric Biggers wrote:
> On Wed, Oct 30, 2019 at 10:51:20AM -0700, Doug Anderson wrote:
>> Hi,
>> On Wed, Oct 30, 2019 at 10:38 AM Eric Biggers <> wrote:
>>> Hi Douglas,
>>> On Wed, Oct 30, 2019 at 10:06:25AM -0700, Douglas Anderson wrote:
>>>> This reverts commit 0642ea2409f3 ("ext4 crypto: fix to check feature
>>>> status before get policy").
>>>> The commit made a clear and documented ABI change that is not backward
>>>> compatible.  There exists userspace code [1] that relied on the old
>>>> behavior and is now broken.
>>>> While we could entertain the idea of updating the userspace code to
>>>> handle the ABI change, it's my understanding that in general ABI
>>>> changes that break userspace are frowned upon (to put it nicely).
>>>> NOTE: if we for some reason do decide to entertain the idea of
>>>> allowing the ABI change and updating userspace, I'd appreciate any
>>>> help on how we should make the change.  Specifically the old code
>>>> relied on the different return values to differentiate between
>>>> "KeyState::NO_KEY" and "KeyState::NOT_SUPPORTED".  I'm no expert on
>>>> the ext4 encryption APIs (I just ended up here tracking down the
>>>> regression [2]) so I'd need a bit of handholding from someone.
>>>> [1]
>>>> [2]
>>>> Fixes: 0642ea2409f3 ("ext4 crypto: fix to check feature status before get policy")
>>>> Signed-off-by: Douglas Anderson <>
>>>> ---
>>>>  Documentation/filesystems/fscrypt.rst | 3 +--
>>>>  fs/ext4/ioctl.c                       | 2 --
>>>>  2 files changed, 1 insertion(+), 4 deletions(-)
>>>> diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
>>>> index 8a0700af9596..4289c29d7c5a 100644
>>>> --- a/Documentation/filesystems/fscrypt.rst
>>>> +++ b/Documentation/filesystems/fscrypt.rst
>>>> @@ -562,8 +562,7 @@ FS_IOC_GET_ENCRYPTION_POLICY_EX can fail with the following errors:
>>>>    or this kernel is too old to support FS_IOC_GET_ENCRYPTION_POLICY_EX
>>>>    (try FS_IOC_GET_ENCRYPTION_POLICY instead)
>>>>  - ``EOPNOTSUPP``: the kernel was not configured with encryption
>>>> -  support for this filesystem, or the filesystem superblock has not
>>>> -  had encryption enabled on it
>>>> +  support for this filesystem
>>>>  - ``EOVERFLOW``: the file is encrypted and uses a recognized
>>>>    encryption policy version, but the policy struct does not fit into
>>>>    the provided buffer
>>>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>>>> index 0b7f316fd30f..13d97fb797b4 100644
>>>> --- a/fs/ext4/ioctl.c
>>>> +++ b/fs/ext4/ioctl.c
>>>> @@ -1181,8 +1181,6 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>>>  #endif
>>>>       }
>>>> -             if (!ext4_has_feature_encrypt(sb))
>>>> -                     return -EOPNOTSUPP;
>>>>               return fscrypt_ioctl_get_policy(filp, (void __user *)arg);
>>> Thanks for reporting this.  Can you elaborate on exactly why returning
>>> EOPNOTSUPP breaks things in the Chrome OS code?  Since encryption is indeed not
>>> supported, why isn't "KeyState::NOT_SUPPORTED" correct?
>> I guess all I know is from the cryptohome source code I sent a link
>> to, which I'm not a super expert in.  Did you get a chance to take a
>> look at that?  As far as I can tell the code is doing something like
>> this:
>> 1. If I see EOPNOTSUPP then this must be a kernel without ext4 crypto.
>> Fallback to using the old-style ecryptfs.
>> 2. If I see ENODATA then this is a kernel with ext4 crypto but there's
>> no key yet.  We should set a key and (if necessarily) enable crypto on
>> the filesystem.
>> 3. If I see no error then we're already good.
>>> Note that the state after this revert will be:
>>> So if this code change is made, the documentation would need to be updated to
>>> explain that the error code from FS_IOC_GET_ENCRYPTION_POLICY is
>>> filesystem-specific (which we'd really like to avoid...), and that
>>> FS_IOC_GET_ENCRYPTION_POLICY_EX handles this case differently.  Or else the
>>> other three would need to be changed to ENODATA -- which for
>>> FS_IOC_GET_ENCRYPTION_POLICY on f2fs would be an ABI break in its own right,
>>> though it's possible that no one would notice.
>>> Is your proposal to keep the error filesystem-specific for now?
>> I guess I'd have to leave it up to the people who know this better.
>> Mostly I just saw this as an ABI change breaking userspace which to me
>> means revert.  I have very little background here to make good
>> decisions about the right way to move forward.
> Okay, that makes sense -- cryptohome assumes that ENODATA means the kernel
> supports encryption, even if the encrypt ext4 feature flag isn't set yet.
> The way it's really supposed to work (IMO) is that all fscrypt ioctls
> consistently return EOPNOTSUPP if the feature is off, and then if userspace
> really needs to know if encryption can nevertheless still be enabled and used on
> the filesystem, it can check for the presence of
> /sys/fs/ext4/features/encryption (or /sys/fs/f2fs/features/encryption).  Or the
> feature flag can just be set by configuration before any of the fscrypt ioctls
> are attempted (this is what Android does).

How about adding above description into documentation as formal guide to check
whether ext4/f2fs can supports encryption feature, ubifs could be described

> I guess we're stuck with the existing ext4 FS_IOC_GET_ENCRYPTION_POLICY behavior
> though, so we need to take this revert for 5.4.
> For 5.5 I think we should try to make things slightly more sane by removing the
> same check from f2fs and fixing the documentation, so that at least each ioctl
> will behave consistently across filesystems and be correctly documented.
> Ted, Jaegeuk, Chao, do you agree?

I saw we're trying to fix Chromium OS code first...


> - Eric
> .

Powered by blists - more mailing lists