Message-ID: <20191030205745.GA216218@sol.localdomain>
Date:   Wed, 30 Oct 2019 13:57:45 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Doug Anderson <dianders@...omium.org>,
        Gwendal Grignou <gwendal@...omium.org>,
        Chao Yu <chao@...nel.org>,
        Ryo Hashimoto <hashimoto@...omium.org>,
        Vadim Sukhomlinov <sukhomlinov@...gle.com>,
        Guenter Roeck <groeck@...omium.org>, apronin@...omium.org,
        linux-doc@...r.kernel.org,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        "Theodore Y. Ts'o" <tytso@....edu>,
        Jonathan Corbet <corbet@....net>,
        LKML <linux-kernel@...r.kernel.org>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        linux-fscrypt@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [PATCH] Revert "ext4 crypto: fix to check feature status before
 get policy"

On Wed, Oct 30, 2019 at 12:02:26PM -0700, 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 <ebiggers@...nel.org> 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] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/cryptohome/dircrypto_util.cc#73
> > > > [2] https://crbug.com/1018265
> > > >
> > > > Fixes: 0642ea2409f3 ("ext4 crypto: fix to check feature status before get policy")
> > > > Signed-off-by: Douglas Anderson <dianders@...omium.org>
> > > > ---
> > > >
> > > >  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
> > > >       }
> > > >       case EXT4_IOC_GET_ENCRYPTION_POLICY:
> > > > -             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:
> > >
> > > - FS_IOC_GET_ENCRYPTION_POLICY on ext4 => ENODATA
> > > - FS_IOC_GET_ENCRYPTION_POLICY on f2fs => EOPNOTSUPP
> > > - FS_IOC_GET_ENCRYPTION_POLICY_EX on ext4 => EOPNOTSUPP
> > > - FS_IOC_GET_ENCRYPTION_POLICY_EX on f2fs => EOPNOTSUPP
> > >
> > > 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).
> 
> 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?
> 

FWIW, from reading the Chrome OS code, I think the code you linked to isn't
where the breakage actually is.  I think it's actually at
https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/chromeos-common-script/share/chromeos-common.sh#375
... where an init script is using the error message printed by 'e4crypt
get_policy' to decide whether to add -O encrypt to the filesystem or not.

It really should check instead:

	[ -e /sys/fs/ext4/features/encryption ]

Anyway, since something broke I think we need to revert the kernel patch anyway.
Ted, if you can provide an Acked-by, I can apply it to fscrypt.git#for-stable
and make a pull request for 5.4.

- Eric

Powered by blists - more mailing lists