linux-ext4 - [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

lists.openwall.net		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-next>] [day] [month] [year] [list]

Message-Id: <20191030100618.1.Ibf7a996e4a58e84f11eec910938cfc3f9159c5de@changeid>
Date:   Wed, 30 Oct 2019 10:06:25 -0700
From:   Douglas Anderson <dianders@...omium.org>
To:     Eric Biggers <ebiggers@...gle.com>
Cc:     Gwendal Grignou <gwendal@...omium.org>, Chao Yu <chao@...nel.org>,
        Ryo Hashimoto <hashimoto@...omium.org>, sukhomlinov@...gle.com,
        groeck@...omium.org, apronin@...omium.org,
        Douglas Anderson <dianders@...omium.org>,
        linux-doc@...r.kernel.org,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        "Theodore Y. Ts'o" <tytso@....edu>,
        Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        linux-fscrypt@...r.kernel.org, Eric Biggers <ebiggers@...nel.org>,
        linux-ext4@...r.kernel.org
Subject: [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

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);
 
 	case FS_IOC_GET_ENCRYPTION_POLICY_EX:
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog