[PATCH 0/4] Add filesystem-level encryption tests

[PATCH 0/4] Add filesystem-level encryption tests

[Eric Biggers]

This patch series adds some xfstests for the filesystem-level encryption
feature currently supported by ext4 and f2fs.  I verified that they pass
on both ext4 and f2fs with the latest kernel.

Note that these new tests are complementary to and not a replacement for
running all the xfstests with encryption enabled, which for ext4 can
currently be done by setting EXT_MOUNT_OPTIONS="-o test_dummy_encryption".

Eric Biggers (4):
  generic: add utilities for testing filesystem encryption
  generic: test setting and getting encryption policies
  generic: test encrypted file access
  generic: test locking when setting encryption policy

 .gitignore            |   1 +
 common/encrypt        |  89 +++++++++
 src/Makefile          |   2 +-
 src/fscrypt_util.c    | 486 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/400     |  88 +++++++++
 tests/generic/400.out |  24 +++
 tests/generic/401     | 133 ++++++++++++++
 tests/generic/401.out |   2 +
 tests/generic/402     |  39 ++++
 tests/generic/402.out |   2 +
 tests/generic/group   |   3 +
 11 files changed, 868 insertions(+), 1 deletion(-)
 create mode 100755 common/encrypt
 create mode 100644 src/fscrypt_util.c
 create mode 100755 tests/generic/400
 create mode 100644 tests/generic/400.out
 create mode 100755 tests/generic/401
 create mode 100644 tests/generic/401.out
 create mode 100755 tests/generic/402
 create mode 100644 tests/generic/402.out

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/4] generic: add utilities for testing filesystem encryption

[Eric Biggers]

Add utilities for testing the filesystem-level encryption feature
currently supported by ext4 and f2fs.  Tests will be able to source
common/encrypt and call _begin_encryption_test to set up an
encryption-capable filesystem on the scratch device, or skip the test
when not supported.

A program fscrypt_util is also added to expose filesystem
encryption-related commands to shell scripts.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 .gitignore         |   1 +
 common/encrypt     |  89 ++++++++++++++++
 src/Makefile       |   2 +-
 src/fscrypt_util.c | 306 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 397 insertions(+), 1 deletion(-)
 create mode 100755 common/encrypt
 create mode 100644 src/fscrypt_util.c

diff --git a/.gitignore b/.gitignore
index 915d2d8..7040f67 100644
--- a/.gitignore
+++ b/.gitignore
@@ -54,6 +54,7 @@
 /src/fill
 /src/fill2
 /src/fs_perms
+/src/fscrypt_util
 /src/fssum
 /src/fstest
 /src/fsync-tester
diff --git a/common/encrypt b/common/encrypt
new file mode 100755
index 0000000..599d16f
--- /dev/null
+++ b/common/encrypt
@@ -0,0 +1,89 @@
+#!/bin/bash
+#
+# Common functions for testing filesystem-level encryption
+#
+#-----------------------------------------------------------------------
+# Copyright (C) 2016 Google, Inc.
+#
+# Author: Eric Biggers <ebiggers@google.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, see <http://www.gnu.org/licenses/>.
+#-----------------------------------------------------------------------
+
+. ./common/rc
+
+# Begin an encryption test.  This creates the scratch filesystem with encryption
+# enabled and mounts it, or skips the test if encryption isn't supported.
+_begin_encryption_test() {
+
+	_supported_os Linux
+	_supported_fs ext4 f2fs
+
+	# We use a dedicated test program 'fscrypt_util' for making API calls
+	# related to encryption.  We aren't using 'e4crypt' because 'e4crypt' is
+	# currently ext4-specific, and with a test program we can easily include
+	# test-only commands and other functionality or behavior that wouldn't
+	# make sense in a real program.
+	_require_test_program fscrypt_util
+
+	# The 'test_dummy_encryption' mount option interferes with trying to use
+	# encryption for real.  So skip the real encryption tests if the
+	# 'test_dummy_encryption' mount option was specified.
+	if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
+		_notrun "Dummy encryption is on; skipping real encryption tests"
+	fi
+
+	# Make a filesystem on the scratch device with the encryption feature
+	# enabled.  If this fails then probably the userspace tools (e.g.
+	# e2fsprogs or f2fs-tools) are too old to understand encryption.
+	_require_scratch
+	if ! _scratch_mkfs_encrypted >/dev/null; then
+		_notrun "$FSTYP userspace tools do not support encryption"
+	fi
+
+	# Try to mount the filesystem.  If this fails then probably the kernel
+	# isn't aware of encryption.
+	if ! _scratch_mount &> /dev/null; then
+		_notrun "kernel is unaware of $FSTYP encryption feature"
+	fi
+
+	# The kernel may be aware of encryption without supporting it.  For
+	# example, for ext4 this is the case with kernels configured with
+	# CONFIG_EXT4_FS_ENCRYPTION=n.  Detect support for encryption by trying
+	# to set an encryption policy.  (For ext4 we could instead check for the
+	# presence of /sys/fs/ext4/features/encryption, but this is broken on
+	# some older kernels and is ext4-specific anyway.)
+	mkdir $SCRATCH_MNT/tmpdir
+	if src/fscrypt_util set_policy 0000111122223333 $SCRATCH_MNT/tmpdir \
+		2>&1 >/dev/null |
+		egrep -q 'Inappropriate ioctl for device|Operation not supported'
+	then
+		_notrun "kernel does not support $FSTYP encryption"
+	fi
+	rmdir $SCRATCH_MNT/tmpdir
+}
+
+_scratch_mkfs_encrypted() {
+	case $FSTYP in
+	ext4)
+		# ext4 encryption requires block size = PAGE_SIZE.
+		MKFS_OPTIONS="-O encrypt -b $(getconf PAGE_SIZE)" _scratch_mkfs
+		;;
+	*)
+		MKFS_OPTIONS="-O encrypt" _scratch_mkfs
+		;;
+	esac
+}
+
+FSCRYPT_UTIL=`pwd`/src/fscrypt_util
diff --git a/src/Makefile b/src/Makefile
index dd51216..0b91402 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -21,7 +21,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	stale_handle pwrite_mmap_blocked t_dir_offset2 seek_sanity_test \
 	seek_copy_test t_readdir_1 t_readdir_2 fsync-tester nsexec cloner \
 	renameat2 t_getcwd e4compact test-nextquota punch-alternating \
-	attr-list-by-handle-cursor-test listxattr
+	attr-list-by-handle-cursor-test listxattr fscrypt_util
 
 SUBDIRS =
 
diff --git a/src/fscrypt_util.c b/src/fscrypt_util.c
new file mode 100644
index 0000000..de63667
--- /dev/null
+++ b/src/fscrypt_util.c
@@ -0,0 +1,306 @@
+/*
+ * fscrypt_util.c - test utility for filesystem-level encryption
+ *
+ * Copyright (C) 2016 Google, Inc.
+ *
+ * Author: Eric Biggers <ebiggers@google.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <linux/fs.h>
+#include <linux/keyctl.h>
+#include <stdarg.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <time.h>
+#include <unistd.h>
+
+/*
+ * Declare the encryption policy structure and the ioctl numbers if they weren't
+ * already declared in linux/fs.h.
+ */
+#ifndef FS_IOC_SET_ENCRYPTION_POLICY
+#define FS_KEY_DESCRIPTOR_SIZE  8
+
+struct fscrypt_policy {
+	__u8 version;
+	__u8 contents_encryption_mode;
+	__u8 filenames_encryption_mode;
+	__u8 flags;
+	__u8 master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
+} __attribute__((packed));
+
+#define FS_IOC_SET_ENCRYPTION_POLICY	_IOR('f', 19, struct fscrypt_policy)
+#define FS_IOC_GET_ENCRYPTION_PWSALT	_IOW('f', 20, __u8[16])
+#define FS_IOC_GET_ENCRYPTION_POLICY	_IOW('f', 21, struct fscrypt_policy)
+#endif /* FS_IOC_SET_ENCRYPTION_POLICY */
+
+
+/*
+ * As of Linux 4.9, some parts of the userspace API (flags, modes, and key
+ * format) are not yet exposed by linux/fs.h.  So we may need to declare them
+ * even if linux/fs.h declares the ioctl numbers.
+ */
+#ifndef FS_ENCRYPTION_MODE_AES_256_XTS
+#define FS_POLICY_FLAGS_PAD_4		0x00
+#define FS_POLICY_FLAGS_PAD_8		0x01
+#define FS_POLICY_FLAGS_PAD_16		0x02
+#define FS_POLICY_FLAGS_PAD_32		0x03
+#define FS_POLICY_FLAGS_PAD_MASK	0x03
+#define FS_POLICY_FLAGS_VALID		0x03
+
+#define FS_ENCRYPTION_MODE_INVALID	0
+#define FS_ENCRYPTION_MODE_AES_256_XTS	1
+#define FS_ENCRYPTION_MODE_AES_256_GCM	2
+#define FS_ENCRYPTION_MODE_AES_256_CBC	3
+#define FS_ENCRYPTION_MODE_AES_256_CTS	4
+
+#define FS_MAX_KEY_SIZE			64
+
+#define FS_KEY_DESC_PREFIX		"fscrypt:"
+#define FS_KEY_DESC_PREFIX_SIZE		8
+
+struct fscrypt_key {
+	__u32 mode;
+	__u8 raw[FS_MAX_KEY_SIZE];
+	__u32 size;
+} __attribute__((packed));
+#endif /* FS_ENCRYPTION_MODE_AES_256_XTS */
+
+static void __attribute__((noreturn))
+usage(void)
+{
+	fprintf(stderr,
+"Usage:\n"
+"    fscrypt_util gen_key\n"
+"    fscrypt_util rm_key KEYDESC\n"
+"    fscrypt_util set_policy KEYDESC DIR\n"
+);
+	exit(2);
+}
+
+static void __attribute__((noreturn, format(printf,1,2)))
+die(const char *format, ...)
+{
+	va_list va;
+
+	va_start(va, format);
+	vfprintf(stderr, format, va);
+	fputc('\n', stderr);
+	va_end(va);
+
+	exit(1);
+}
+
+static void __attribute__((noreturn, format(printf,1,2)))
+die_errno(const char *format, ...)
+{
+	va_list va;
+	int err = errno;
+
+	va_start(va, format);
+	vfprintf(stderr, format, va);
+	fprintf(stderr, ": %s\n", strerror(err));
+	va_end(va);
+
+	exit(1);
+}
+
+/*
+ * Sanity check: given a directory file descriptor on which we just set the
+ * encryption policy @expected, it should be possible to get the same policy
+ * back from the kernel using FS_IOC_GET_ENCRYPTION_POLICY.
+ */
+static void verify_policy(const char *dir, int fd,
+			  const struct fscrypt_policy *expected)
+{
+	struct fscrypt_policy actual;
+
+	memset(&actual, 0xFF, sizeof(actual));
+
+	if (ioctl(fd, FS_IOC_GET_ENCRYPTION_POLICY, &actual) < 0)
+		die_errno("%s: FS_IOC_GET_ENCRYPTION_POLICY failed", dir);
+
+	if (memcmp(&actual, expected, sizeof(actual)) != 0)
+		die("%s: encryption policy did not survive round-trip", dir);
+}
+
+/* Initialize a 'struct fscrypt_policy' */
+static void init_policy(struct fscrypt_policy *policy, const char *keydesc_str)
+{
+	unsigned long long keydesc;
+	char *tmp;
+	int i;
+
+	memset(policy, 0, sizeof(*policy));
+	policy->contents_encryption_mode = FS_ENCRYPTION_MODE_AES_256_XTS;
+	policy->filenames_encryption_mode = FS_ENCRYPTION_MODE_AES_256_CTS;
+	policy->flags = FS_POLICY_FLAGS_PAD_16;
+
+	keydesc = strtoull(keydesc_str, &tmp, 16);
+	if (tmp == keydesc_str || *tmp != '\0')
+		die("Invalid keydesc: %s", keydesc_str);
+
+	for (i = 0; i < FS_KEY_DESCRIPTOR_SIZE; i++) {
+		policy->master_key_descriptor[i] = keydesc >> 56;
+		keydesc <<= 8;
+	}
+}
+
+static void init_policy_default(struct fscrypt_policy *policy)
+{
+	init_policy(policy, "0000111122223333");
+}
+
+/*
+ * Generate a "random" fscrypt key and add it to the session keyring, identified
+ * by a "random" key descriptor.  Afterwards print out the key descriptor.
+ *
+ * Note that this is not secure at all and must only be used for testing!  Also,
+ * we are using the common key naming conventiion ("fscrypt:" instead of "ext4:"
+ * or "f2fs:"), which is only supported by 4.8+ kernels for ext4 and 4.6+
+ * kernels for f2fs.
+ */
+static int gen_key(int argc, char **argv)
+{
+	char keyname[FS_KEY_DESC_PREFIX_SIZE +
+		     (FS_KEY_DESCRIPTOR_SIZE * 2) + 1];
+	struct fscrypt_key key;
+	int32_t ringid;
+	int i;
+
+	if (argc != 0)
+		usage();
+
+	srand(time(NULL));
+
+	memset(&key, 0, sizeof(key));
+	key.size = FS_MAX_KEY_SIZE;
+	for (i = 0; i < FS_MAX_KEY_SIZE; i++)
+		key.raw[i] = rand() & 0xFF;
+
+	strcpy(keyname, FS_KEY_DESC_PREFIX);
+	for (i = 0; i < FS_KEY_DESCRIPTOR_SIZE; i++) {
+		sprintf(&keyname[FS_KEY_DESC_PREFIX_SIZE + i*2],
+			"%02x", rand() & 0xFF);
+	}
+
+	ringid = syscall(__NR_keyctl, KEYCTL_GET_KEYRING_ID,
+			 KEY_SPEC_SESSION_KEYRING, 0);
+	if (ringid == -1)
+		die_errno("Unable to find session keyring");
+
+	if (syscall(__NR_add_key, "logon", keyname, &key, sizeof(key),
+		    ringid) == -1)
+		die_errno("Unable to add key to session keyring");
+
+	printf("%s\n", keyname + FS_KEY_DESC_PREFIX_SIZE);
+	return 0;
+}
+
+/* Remove a fscrypt key from the session keyring */
+static int rm_key(int argc, char **argv)
+{
+	const char *keydesc_str;
+	char *keyname;
+	int32_t keyid;
+
+	if (argc != 1)
+		usage();
+	keydesc_str = argv[0];
+
+	keyname = malloc(FS_KEY_DESCRIPTOR_SIZE + strlen(keydesc_str) + 1);
+	sprintf(keyname, "%s%s", FS_KEY_DESC_PREFIX, keydesc_str);
+
+	keyid = syscall(__NR_keyctl, KEYCTL_SEARCH, KEY_SPEC_SESSION_KEYRING,
+			"logon", keyname, 0);
+	if (keyid == -1)
+		die_errno("Unable to find key %s\n", keyname);
+
+	if (syscall(__NR_keyctl, KEYCTL_UNLINK, keyid,
+		    KEY_SPEC_SESSION_KEYRING) == -1)
+		die_errno("Unable to unlink key %s\n", keyname);
+
+	free(keyname);
+	return 0;
+}
+
+/* Command to expose FS_IOC_SET_ENCRYPTION_POLICY to shell scripts */
+static int set_policy(int argc, char **argv)
+{
+	const char *keydesc_str;
+	const char *dir;
+	struct fscrypt_policy policy;
+	int fd;
+
+	if (argc != 2)
+		usage();
+	keydesc_str = argv[0];
+	dir = argv[1];
+
+	init_policy(&policy, keydesc_str);
+
+	fd = open(dir, O_RDONLY);
+	if (fd < 0)
+		die_errno("%s: Unable to open", dir);
+
+	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) < 0)
+		die_errno("%s: Unable to set encryption policy", dir);
+
+	verify_policy(dir, fd, &policy);
+	close(fd);
+
+	printf("%s: Successfully assigned encryption key %s\n", dir,
+	       keydesc_str);
+	return 0;
+}
+
+static const struct command {
+	const char *name;
+	int (*func)(int, char **);
+} commands[] = {
+	{"gen_key", gen_key},
+	{"rm_key", rm_key},
+	{"set_policy", set_policy},
+	{NULL, NULL}
+};
+
+int main(int argc, char **argv)
+{
+	const char *cmdname;
+	const struct command *cmd;
+
+	if (argc < 2)
+		usage();
+
+	cmdname = argv[1];
+	argc -= 2;
+	argv += 2;
+
+	for (cmd = commands; cmd->name != NULL; cmd++)
+		if (strcmp(cmdname, cmd->name) == 0)
+			return cmd->func(argc, argv);
+
+	usage();
+}
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 1/4] generic: add utilities for testing filesystem encryption

[Dave Chinner]

On Thu, Nov 17, 2016 at 11:47:04AM -0800, Eric Biggers wrote:
> Add utilities for testing the filesystem-level encryption feature
> currently supported by ext4 and f2fs.  Tests will be able to source
> common/encrypt and call _begin_encryption_test to set up an
> encryption-capable filesystem on the scratch device, or skip the test
> when not supported.
> 
> A program fscrypt_util is also added to expose filesystem
> encryption-related commands to shell scripts.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  .gitignore         |   1 +
>  common/encrypt     |  89 ++++++++++++++++
>  src/Makefile       |   2 +-
>  src/fscrypt_util.c | 306 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 397 insertions(+), 1 deletion(-)
>  create mode 100755 common/encrypt
>  create mode 100644 src/fscrypt_util.c
> 
> diff --git a/.gitignore b/.gitignore
> index 915d2d8..7040f67 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -54,6 +54,7 @@
>  /src/fill
>  /src/fill2
>  /src/fs_perms
> +/src/fscrypt_util
>  /src/fssum
>  /src/fstest
>  /src/fsync-tester
> diff --git a/common/encrypt b/common/encrypt
> new file mode 100755
> index 0000000..599d16f
> --- /dev/null
> +++ b/common/encrypt
> @@ -0,0 +1,89 @@
> +#!/bin/bash
> +#
> +# Common functions for testing filesystem-level encryption
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (C) 2016 Google, Inc.
> +#
> +# Author: Eric Biggers <ebiggers@google.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, see <http://www.gnu.org/licenses/>.
> +#-----------------------------------------------------------------------
> +
> +. ./common/rc

Tests will already have included common/rc before this file, so we
do not source it here.


> +
> +# Begin an encryption test.  This creates the scratch filesystem with encryption
> +# enabled and mounts it, or skips the test if encryption isn't supported.
> +_begin_encryption_test() {
> +
> +	_supported_os Linux
> +	_supported_fs ext4 f2fs

These go in the tests, not here.

> +
> +	# We use a dedicated test program 'fscrypt_util' for making API calls
> +	# related to encryption.  We aren't using 'e4crypt' because 'e4crypt' is
> +	# currently ext4-specific, and with a test program we can easily include
> +	# test-only commands and other functionality or behavior that wouldn't
> +	# make sense in a real program.
> +	_require_test_program fscrypt_util
> +
> +	# The 'test_dummy_encryption' mount option interferes with trying to use
> +	# encryption for real.  So skip the real encryption tests if the
> +	# 'test_dummy_encryption' mount option was specified.
> +	if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
> +		_notrun "Dummy encryption is on; skipping real encryption tests"
> +	fi

_requires_real_encryption()

In each test.


> +
> +	# Make a filesystem on the scratch device with the encryption feature
> +	# enabled.  If this fails then probably the userspace tools (e.g.
> +	# e2fsprogs or f2fs-tools) are too old to understand encryption.
> +	_require_scratch
> +	if ! _scratch_mkfs_encrypted >/dev/null; then
> +		_notrun "$FSTYP userspace tools do not support encryption"
> +	fi
> +
> +	# Try to mount the filesystem.  If this fails then probably the kernel
> +	# isn't aware of encryption.
> +	if ! _scratch_mount &> /dev/null; then
> +		_notrun "kernel is unaware of $FSTYP encryption feature"
> +	fi
> +
> +	# The kernel may be aware of encryption without supporting it.  For
> +	# example, for ext4 this is the case with kernels configured with
> +	# CONFIG_EXT4_FS_ENCRYPTION=n.  Detect support for encryption by trying
> +	# to set an encryption policy.  (For ext4 we could instead check for the
> +	# presence of /sys/fs/ext4/features/encryption, but this is broken on
> +	# some older kernels and is ext4-specific anyway.)
> +	mkdir $SCRATCH_MNT/tmpdir
> +	if src/fscrypt_util set_policy 0000111122223333 $SCRATCH_MNT/tmpdir \
> +		2>&1 >/dev/null |
> +		egrep -q 'Inappropriate ioctl for device|Operation not supported'
> +	then
> +		_notrun "kernel does not support $FSTYP encryption"
> +	fi
> +	rmdir $SCRATCH_MNT/tmpdir

And this should all be in a _requires_encryption() function.

> +}
> +
> +_scratch_mkfs_encrypted() {
> +	case $FSTYP in
> +	ext4)
> +		# ext4 encryption requires block size = PAGE_SIZE.
> +		MKFS_OPTIONS="-O encrypt -b $(getconf PAGE_SIZE)" _scratch_mkfs
> +		;;
> +	*)
> +		MKFS_OPTIONS="-O encrypt" _scratch_mkfs
> +		;;
> +	esac
> +}

THis completely overrides any user supplied mkfs options, which we
try to avoid doing. Instead, this should simply be

	_scratch_mkfs -O encrypt

so that _scratch_mkfs_encrypted() fails if there are conflicting
mkfs options specified. This means _requires_encryption() will
not_run the test when this occurrs...


> +FSCRYPT_UTIL=`pwd`/src/fscrypt_util

_require_test_program fscrypt_util

FSCRYPT_UTIL=src/fscrypt_util

> diff --git a/src/fscrypt_util.c b/src/fscrypt_util.c
> new file mode 100644
> index 0000000..de63667
> --- /dev/null
> +++ b/src/fscrypt_util.c

....

Ok, can we get this added to xfs_io rather than a stand-alone
fstests helper? There are three clear commands here:

	{"gen_key", gen_key},
	{"rm_key", rm_key},
	{"set_policy", set_policy},

So it should plug straight into the xfs_io command parsing
infrastructure without much change at all.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] generic: add utilities for testing filesystem encryption

[Eric Biggers]

Hi Dave, thanks for reviewing.

On Mon, Nov 21, 2016 at 08:33:13AM +1100, Dave Chinner wrote:
> > +
> > +. ./common/rc
> 
> Tests will already have included common/rc before this file, so we
> do not source it here.
...
> These go in the tests, not here.
...
> _requires_real_encryption()
> 
> In each test.
...
> And this should all be in a _requires_encryption() function.
> 

I'll do all of these.  Of course the intent was to avoid duplicating code in
each test, but I will use the more verbose style if that's preferred.  I assume
you'd also prefer explicitly formatting and mounting the scratch device in each
test even though _require_encryption would already have to do that?

> > +}
> > +
> > +_scratch_mkfs_encrypted() {
> > +	case $FSTYP in
> > +	ext4)
> > +		# ext4 encryption requires block size = PAGE_SIZE.
> > +		MKFS_OPTIONS="-O encrypt -b $(getconf PAGE_SIZE)" _scratch_mkfs
> > +		;;
> > +	*)
> > +		MKFS_OPTIONS="-O encrypt" _scratch_mkfs
> > +		;;
> > +	esac
> > +}
> 
> THis completely overrides any user supplied mkfs options, which we
> try to avoid doing. Instead, this should simply be
> 
> 	_scratch_mkfs -O encrypt
> 
> so that _scratch_mkfs_encrypted() fails if there are conflicting
> mkfs options specified. This means _requires_encryption() will
> not_run the test when this occurrs...
> 

It wouldn't work exactly like that because mkfs.ext4 currently allows creating a
filesystem with -O encrypt and a block size incompatible with encryption.  The
kernel then refuses to mount the filesystem.  But your suggestion would
basically still work, since we have to try mounting the filesystem anyway to
check for kernel support for encryption.

> > diff --git a/src/fscrypt_util.c b/src/fscrypt_util.c
> > new file mode 100644
> > index 0000000..de63667
> > --- /dev/null
> > +++ b/src/fscrypt_util.c
> 
> ....
> 
> Ok, can we get this added to xfs_io rather than a stand-alone
> fstests helper? There are three clear commands here:
> 
> 	{"gen_key", gen_key},
> 	{"rm_key", rm_key},
> 	{"set_policy", set_policy},
> 
> So it should plug straight into the xfs_io command parsing
> infrastructure without much change at all.

I see that xfs_io is part of xfsprogs, not xfstests.  Does it make sense to add
filesystem encryption commands to xfs_io even though XFS doesn't support them
yet?  Currently only ext4 and f2fs support filesystem encryption via this common
API.  (ubifs support has been proposed too.)

If we do go that route then it should be considered only adding "set_policy" and
"get_policy" commands, and for "gen_key" and "rm_key" instead using shell
wrappers around 'keyctl' instead.  gen_key and rm_key don't touch the filesystem
at all; they only work with the keyring.  It's possible to use 'keyctl padd' to
add a fscrypt key, though it's not completely trivial because you'd have to use
'echo -e' to generate the C structure 'struct fscrypt_key' with mode = 0, raw =
actual key in binary, size = 64.

Eric

Re: [PATCH 1/4] generic: add utilities for testing filesystem encryption

[Dave Chinner]

On Mon, Nov 21, 2016 at 10:40:50AM -0800, Eric Biggers wrote:
> Hi Dave, thanks for reviewing.
> 
> On Mon, Nov 21, 2016 at 08:33:13AM +1100, Dave Chinner wrote:
> > > +
> > > +. ./common/rc
> > 
> > Tests will already have included common/rc before this file, so we
> > do not source it here.
> ...
> > These go in the tests, not here.
> ...
> > _requires_real_encryption()
> > 
> > In each test.
> ...
> > And this should all be in a _requires_encryption() function.
> > 
> 
> I'll do all of these.  Of course the intent was to avoid duplicating code in
> each test, but I will use the more verbose style if that's preferred.  I assume
> you'd also prefer explicitly formatting and mounting the scratch device in each
> test even though _require_encryption would already have to do that?

Yes, because in future _require_encryption may change to no require
touching the scratch device. Also, checking for encryption may
create a filesystem with different configuration than the one we
want to test. So it's better to be consistent across all tests and
require the scratch_mkfs call in each test so we know the exact
state of the filesystem before the test starts....

> > Ok, can we get this added to xfs_io rather than a stand-alone
> > fstests helper? There are three clear commands here:
> > 
> > 	{"gen_key", gen_key},
> > 	{"rm_key", rm_key},
> > 	{"set_policy", set_policy},
> > 
> > So it should plug straight into the xfs_io command parsing
> > infrastructure without much change at all.
> 
> I see that xfs_io is part of xfsprogs, not xfstests.  Does it make sense to add
> filesystem encryption commands to xfs_io even though XFS doesn't support them
> yet?  Currently only ext4 and f2fs support filesystem encryption via this common
> API.  (ubifs support has been proposed too.)

Yes, because it is in the plan to support the generic encryption in
XFS, too. It'll just take us a little while to get to it, but it
won't hurt to support these operations ahead of that time...

> If we do go that route then it should be considered only adding
> "set_policy" and "get_policy" commands, and for "gen_key" and
> "rm_key" instead using shell wrappers around 'keyctl' instead.
> gen_key and rm_key don't touch the filesystem at all; they only
> work with the keyring.  It's possible to use 'keyctl padd' to add
> a fscrypt key, though it's not completely trivial because you'd
> have to use 'echo -e' to generate the C structure 'struct
> fscrypt_key' with mode = 0, raw = actual key in binary, size = 64.

Sounds fine to me.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 2/4] generic: test setting and getting encryption policies

[Eric Biggers]

Several kernel bugs were recently fixed regarding the constraints for
setting encryption policies.  Add tests for these cases and a few more.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 src/fscrypt_util.c    | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/400     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/400.out | 24 ++++++++++++++
 tests/generic/group   |  1 +
 4 files changed, 195 insertions(+)
 create mode 100755 tests/generic/400
 create mode 100644 tests/generic/400.out

diff --git a/src/fscrypt_util.c b/src/fscrypt_util.c
index de63667..9428cb4 100644
--- a/src/fscrypt_util.c
+++ b/src/fscrypt_util.c
@@ -96,6 +96,7 @@ usage(void)
 "    fscrypt_util gen_key\n"
 "    fscrypt_util rm_key KEYDESC\n"
 "    fscrypt_util set_policy KEYDESC DIR\n"
+"    fscrypt_util test_ioctl_validation DIR\n"
 );
 	exit(2);
 }
@@ -276,6 +277,86 @@ static int set_policy(int argc, char **argv)
 	return 0;
 }
 
+/*
+ * Test that the kernel does basic validation of the arguments to
+ * FS_IOC_SET_ENCRYPTION_POLICY and FS_IOC_GET_ENCRYPTION_POLICY.
+ */
+static int test_ioctl_validation(int argc, char **argv)
+{
+	const char *dir;
+	int fd;
+	struct fscrypt_policy policy;
+
+	if (argc != 1)
+		usage();
+	dir = argv[0];
+
+	fd = open(dir, O_RDONLY);
+	if (fd < 0)
+		die_errno("%s: Unable to open", dir);
+
+	/* trying to get encryption policy for unencrypted file */
+	if (ioctl(fd, FS_IOC_GET_ENCRYPTION_POLICY, NULL) != -1 ||
+	    (errno != ENODATA && errno != ENOENT)) {
+		die("expected FS_IOC_GET_ENCRYPTION_POLICY to fail with "
+		    "ENODATA or ENOENT when unencrypted file specified");
+	}
+
+	/* invalid pointer */
+	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, NULL) != -1 ||
+	    errno != EFAULT) {
+		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
+		    "EFAULT when invalid pointer specified");
+	}
+
+	/* invalid flags */
+	init_policy_default(&policy);
+	policy.flags = 0xFF;
+	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != -1 ||
+	    errno != EINVAL) {
+		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
+		    "EINVAL when invalid flags specified");
+	}
+
+	/* invalid encryption modes */
+	init_policy_default(&policy);
+	policy.contents_encryption_mode = 0xFF;
+	policy.filenames_encryption_mode = 0xFF;
+	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != -1 ||
+	    errno != EINVAL) {
+		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
+		    "EINVAL when invalid encryption modes specified");
+	}
+
+	/* invalid policy version */
+	init_policy_default(&policy);
+	policy.version = 0xFF;
+	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != -1 ||
+	    errno != EINVAL) {
+		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
+		    "EINVAL when invalid policy version specified");
+	}
+
+	/* success case */
+	init_policy_default(&policy);
+	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != 0)
+		die_errno("expected FS_IOC_SET_ENCRYPTION_POLICY to succeed");
+
+	verify_policy(dir, fd, &policy);
+
+	/* invalid pointer (get) */
+	if (ioctl(fd, FS_IOC_GET_ENCRYPTION_POLICY, NULL) != -1 ||
+	    errno != EFAULT) {
+		die("expected FS_IOC_GET_ENCRYPTION_POLICY to fail with "
+		    "EFAULT when invalid pointer specified");
+	}
+
+	close(fd);
+
+	printf("%s: test_ioctl_validation passed\n", dir);
+	return 0;
+}
+
 static const struct command {
 	const char *name;
 	int (*func)(int, char **);
@@ -283,6 +364,7 @@ static const struct command {
 	{"gen_key", gen_key},
 	{"rm_key", rm_key},
 	{"set_policy", set_policy},
+	{"test_ioctl_validation", test_ioctl_validation},
 	{NULL, NULL}
 };
 
diff --git a/tests/generic/400 b/tests/generic/400
new file mode 100755
index 0000000..b077612
--- /dev/null
+++ b/tests/generic/400
@@ -0,0 +1,88 @@
+#!/bin/bash
+# FS QA Test generic/400
+#
+# Test setting and getting encryption policies.
+#
+# This test only exercises the ioctls; it does not set up encryption keys.
+#
+#-----------------------------------------------------------------------
+# Copyright (C) 2016 Google, Inc.
+#
+# Author: Eric Biggers <ebiggers@google.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, see <http://www.gnu.org/licenses/>.
+#-----------------------------------------------------------------------
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+here=`pwd`
+echo "QA output created by $seq"
+
+. ./common/encrypt
+
+_require_user
+_begin_encryption_test
+
+cd $SCRATCH_MNT
+
+# Should be able to set an encryption policy on an empty directory
+echo -e "\n*** Setting encryption policy on empty directory ***"
+mkdir empty_dir
+$FSCRYPT_UTIL set_policy 0000111122223333 empty_dir
+
+# Should be able to set the same policy again, but not a different one
+echo -e "\n*** Setting same encryption policy again ***"
+$FSCRYPT_UTIL set_policy 0000111122223333 empty_dir
+$FSCRYPT_UTIL set_policy 4444555566667777 empty_dir
+
+# Should *not* be able to set an encryption policy on a nonempty directory
+echo -e "\n*** Setting encryption policy on nonempty directory ***"
+mkdir nonempty_dir
+touch nonempty_dir/file
+$FSCRYPT_UTIL set_policy 0000111122223333 nonempty_dir
+
+# Should *not* be able to set an encryption policy on a nondirectory file, even
+# an empty one.  Regression test for 002ced4be642: "fscrypto: only allow setting
+# encryption policy on directories".
+echo -e "\n*** Setting encryption policy on nondirectory ***"
+touch nondirectory
+$FSCRYPT_UTIL set_policy 0000111122223333 nondirectory
+
+# Should *not* be able to set an encryption policy on another user's directory.
+# Regression test for 163ae1c6ad62: "fscrypto: add authorization check for
+# setting encryption policy".
+echo -e "\n*** Setting encryption policy on another user's directory ***"
+mkdir unauthorized_dir
+su $qa_user -c "$FSCRYPT_UTIL set_policy 0000111122223333 unauthorized_dir"
+
+# Should *not* be able to set an encryption policy on a directory on a
+# filesystem mounted readonly.  Regression test for ba63f23d69a3: "fscrypto:
+# require write access to mount to set encryption policy".  Test both a regular
+# readonly filesystem and a read-write filesystem remounted with "ro,bind",
+# which creates a readonly mount for a read-write filesystem.
+echo -e "\n*** Setting encryption policy on readonly filesystem ***"
+mkdir readonly_mnt_dir
+_scratch_mount -o ro,remount
+$FSCRYPT_UTIL set_policy 0000111122223333 readonly_mnt_dir
+_scratch_mount -o rw,remount
+_scratch_mount -o remount,ro,bind
+$FSCRYPT_UTIL set_policy 0000111122223333 readonly_mnt_dir
+_scratch_mount -o rw,remount
+
+# Test basic validation of set_policy / get_policy ioctl arguments
+echo -e "\n*** ioctl validation ***"
+mkdir validation_dir
+$FSCRYPT_UTIL test_ioctl_validation validation_dir
+
+exit 0
diff --git a/tests/generic/400.out b/tests/generic/400.out
new file mode 100644
index 0000000..dbae79d
--- /dev/null
+++ b/tests/generic/400.out
@@ -0,0 +1,24 @@
+QA output created by 400
+
+*** Setting encryption policy on empty directory ***
+empty_dir: Successfully assigned encryption key 0000111122223333
+
+*** Setting same encryption policy again ***
+empty_dir: Successfully assigned encryption key 0000111122223333
+empty_dir: Unable to set encryption policy: Invalid argument
+
+*** Setting encryption policy on nonempty directory ***
+nonempty_dir: Unable to set encryption policy: Directory not empty
+
+*** Setting encryption policy on nondirectory ***
+nondirectory: Unable to set encryption policy: Invalid argument
+
+*** Setting encryption policy on another user's directory ***
+unauthorized_dir: Unable to set encryption policy: Permission denied
+
+*** Setting encryption policy on readonly filesystem ***
+readonly_mnt_dir: Unable to set encryption policy: Read-only file system
+readonly_mnt_dir: Unable to set encryption policy: Read-only file system
+
+*** ioctl validation ***
+validation_dir: test_ioctl_validation passed
diff --git a/tests/generic/group b/tests/generic/group
index 08007d7..cf89f06 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -392,3 +392,4 @@
 387 auto clone
 388 auto log metadata
 389 auto quick acl
+400 auto quick encrypt
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 2/4] generic: test setting and getting encryption policies

[Dave Chinner]

On Thu, Nov 17, 2016 at 11:47:05AM -0800, Eric Biggers wrote:
> Several kernel bugs were recently fixed regarding the constraints for
> setting encryption policies.  Add tests for these cases and a few more.

more comments below, but in general this sort of test should be
driven through xfs_io command line parameters.

i.e. we put all the functionality into the xfs_io comaand interface,
and it just passes through whatever the test script tells it. In
this case, the set_policy command needs several options to set
different parts of the policy appropriately.

The reason we tend to put this sort of thing into xfs_io is that
when we need to write a new test, all the commands we need to
construct specific policies/contexts already exist and we don't have
to write new helpers for each test....

> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  src/fscrypt_util.c    | 82 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/400     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/400.out | 24 ++++++++++++++
>  tests/generic/group   |  1 +
>  4 files changed, 195 insertions(+)
>  create mode 100755 tests/generic/400
>  create mode 100644 tests/generic/400.out
> 
> diff --git a/src/fscrypt_util.c b/src/fscrypt_util.c
> index de63667..9428cb4 100644
> --- a/src/fscrypt_util.c
> +++ b/src/fscrypt_util.c
> @@ -96,6 +96,7 @@ usage(void)
>  "    fscrypt_util gen_key\n"
>  "    fscrypt_util rm_key KEYDESC\n"
>  "    fscrypt_util set_policy KEYDESC DIR\n"
> +"    fscrypt_util test_ioctl_validation DIR\n"
>  );
>  	exit(2);
>  }
> @@ -276,6 +277,86 @@ static int set_policy(int argc, char **argv)
>  	return 0;
>  }
>  
> +/*
> + * Test that the kernel does basic validation of the arguments to
> + * FS_IOC_SET_ENCRYPTION_POLICY and FS_IOC_GET_ENCRYPTION_POLICY.
> + */
> +static int test_ioctl_validation(int argc, char **argv)
> +{
> +	const char *dir;
> +	int fd;
> +	struct fscrypt_policy policy;
> +
> +	if (argc != 1)
> +		usage();
> +	dir = argv[0];
> +
> +	fd = open(dir, O_RDONLY);
> +	if (fd < 0)
> +		die_errno("%s: Unable to open", dir);
> +
> +	/* trying to get encryption policy for unencrypted file */
> +	if (ioctl(fd, FS_IOC_GET_ENCRYPTION_POLICY, NULL) != -1 ||
> +	    (errno != ENODATA && errno != ENOENT)) {
> +		die("expected FS_IOC_GET_ENCRYPTION_POLICY to fail with "
> +		    "ENODATA or ENOENT when unencrypted file specified");
> +	}

Can we format these in the normal way? i.e.

	error = ioctl();
	if (error < 0 &&
	    (errno exceptions))
		die()

Also, shouldn't a get without an args parameter always return
EINVAL, regardless of whether the underlying file is encrypted or
not?

> +	/* invalid pointer */
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, NULL) != -1 ||
> +	    errno != EFAULT) {
> +		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
> +		    "EFAULT when invalid pointer specified");
> +	}

>From the command line, shouldn't this be triggered by "set_policy
NULL"?

> +	/* invalid flags */
> +	init_policy_default(&policy);
> +	policy.flags = 0xFF;
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != -1 ||
> +	    errno != EINVAL) {
> +		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
> +		    "EINVAL when invalid flags specified");
> +	}

"set_policy -f 0xff"

> +
> +	/* invalid encryption modes */
> +	init_policy_default(&policy);
> +	policy.contents_encryption_mode = 0xFF;
> +	policy.filenames_encryption_mode = 0xFF;
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != -1 ||
> +	    errno != EINVAL) {
> +		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
> +		    "EINVAL when invalid encryption modes specified");
> +	}

"set_policy -c 0xff -n 0xff"

> +
> +	/* invalid policy version */
> +	init_policy_default(&policy);
> +	policy.version = 0xFF;
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != -1 ||
> +	    errno != EINVAL) {
> +		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
> +		    "EINVAL when invalid policy version specified");
> +	}

"set_policy -v 0xff"

> +
> +	/* success case */
> +	init_policy_default(&policy);
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != 0)
> +		die_errno("expected FS_IOC_SET_ENCRYPTION_POLICY to succeed");

"set_policy default"

> +	verify_policy(dir, fd, &policy);
> +
> +	/* invalid pointer (get) */
> +	if (ioctl(fd, FS_IOC_GET_ENCRYPTION_POLICY, NULL) != -1 ||
> +	    errno != EFAULT) {
> +		die("expected FS_IOC_GET_ENCRYPTION_POLICY to fail with "
> +		    "EFAULT when invalid pointer specified");
> +	}

EINVAL - this should never get to copyout to generate EFAULT, so
should not require separate tests for having no policy vs a valid
policy.

These should all be in a single xfstest that "tests ioctl validity",
rather than appended to a "set_policy behaviour" test.

> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, see <http://www.gnu.org/licenses/>.
> +#-----------------------------------------------------------------------
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +here=`pwd`
> +echo "QA output created by $seq"
> +
> +. ./common/encrypt

This is not the way to include all the required scripts, as I
mentioned in my last email....

Also, please do not gut the test script preamble - it's there in the
new test template for good reason and that is that all the common
code that is included relies on the setup it does. e.g. this means $tmp
is not properly set, so any common code that has been included that
does 'rm -rf $tmp/*' if going to erase your root filesystem.

> +_require_user
> +_begin_encryption_test
> +
> +cd $SCRATCH_MNT
> +
> +_require_user
> +_begin_encryption_test
> +
> +cd $SCRATCH_MNT

... because mounting scratch without having first run _scratch_mkfs
is just wrong. People familiar with xfstests setup are going to look
at this and think the test is broken, because it doesn't
_require_scratch, it doesn't run mkfs or mount, etc....

> +# Should *not* be able to set an encryption policy on a directory on a
> +# filesystem mounted readonly.  Regression test for ba63f23d69a3: "fscrypto:
> +# require write access to mount to set encryption policy".  Test both a regular
> +# readonly filesystem and a read-write filesystem remounted with "ro,bind",
> +# which creates a readonly mount for a read-write filesystem.
> +echo -e "\n*** Setting encryption policy on readonly filesystem ***"
> +mkdir readonly_mnt_dir
> +_scratch_mount -o ro,remount

scratch_remount ro

> +$FSCRYPT_UTIL set_policy 0000111122223333 readonly_mnt_dir
> +_scratch_mount -o rw,remount

scratch_remount rw

> +_scratch_mount -o remount,ro,bind

Umm, what does a bind mount do when there's no source/target
directory? Whatever you are doing here is not documented in the
mount(8) man page....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/4] generic: test setting and getting encryption policies

[Eric Biggers]

On Mon, Nov 21, 2016 at 09:07:18AM +1100, Dave Chinner wrote:
> 
> i.e. we put all the functionality into the xfs_io comaand interface,
> and it just passes through whatever the test script tells it. In
> this case, the set_policy command needs several options to set
> different parts of the policy appropriately.
> 
> The reason we tend to put this sort of thing into xfs_io is that
> when we need to write a new test, all the commands we need to
> construct specific policies/contexts already exist and we don't have
> to write new helpers for each test....
> 

Thanks, I'll consider this.

> 
> Also, shouldn't a get without an args parameter always return
> EINVAL, regardless of whether the underlying file is encrypted or
> not?
> 

For most syscalls/ioctls, including this one (FS_IOC_GET_ENCRYPTION_POLICY),
it's not expected for the kernel to check that a userspace pointer is NULL as
opposed to some other random invalid value.  It will only notice at the very end
of the operation, when copying data to userspace, in which case it's expected to
fail with EFAULT.

It would still make sense to pass in a valid pointer when testing some other
failure case, though, to avoid confusion.

> > +	verify_policy(dir, fd, &policy);
> > +
> > +	/* invalid pointer (get) */
> > +	if (ioctl(fd, FS_IOC_GET_ENCRYPTION_POLICY, NULL) != -1 ||
> > +	    errno != EFAULT) {
> > +		die("expected FS_IOC_GET_ENCRYPTION_POLICY to fail with "
> > +		    "EFAULT when invalid pointer specified");
> > +	}
> 
> EINVAL - this should never get to copyout to generate EFAULT, so
> should not require separate tests for having no policy vs a valid
> policy.

This is specifically testing the EFAULT case.  The directory has been assigned
an encryption policy, so it does indeed get to this case.

> 
> These should all be in a single xfstest that "tests ioctl validity",
> rather than appended to a "set_policy behaviour" test.

Yes this may make sense.  It gets a little blurry when you talk about testing
behavior like "an encryption policy cannot be set on a directory", since that's
a type of validation too.

> 
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +here=`pwd`
> > +echo "QA output created by $seq"
> > +
> > +. ./common/encrypt
> 
> This is not the way to include all the required scripts, as I
> mentioned in my last email....
> 
> Also, please do not gut the test script preamble - it's there in the
> new test template for good reason and that is that all the common
> code that is included relies on the setup it does. e.g. this means $tmp
> is not properly set, so any common code that has been included that
> does 'rm -rf $tmp/*' if going to erase your root filesystem.
> 

Will do.  I think it's pretty riduculous for xfstests to potentially erase your
root filesystem if you don't set some random variable though...

What would be nice is for there to be a preamble that all the tests source that
handles these boilerplate tasks.

> > +# Should *not* be able to set an encryption policy on a directory on a
> > +# filesystem mounted readonly.  Regression test for ba63f23d69a3: "fscrypto:
> > +# require write access to mount to set encryption policy".  Test both a regular
> > +# readonly filesystem and a read-write filesystem remounted with "ro,bind",
> > +# which creates a readonly mount for a read-write filesystem.
> > +echo -e "\n*** Setting encryption policy on readonly filesystem ***"
> > +mkdir readonly_mnt_dir
> > +_scratch_mount -o ro,remount
> 
> scratch_remount ro
> 
> > +$FSCRYPT_UTIL set_policy 0000111122223333 readonly_mnt_dir
> > +_scratch_mount -o rw,remount
> 
> scratch_remount rw
> 
> > +_scratch_mount -o remount,ro,bind
> 
> Umm, what does a bind mount do when there's no source/target
> directory? Whatever you are doing here is not documented in the
> mount(8) man page....

As I noted in the comment above this is creating a readonly mount for a
read-write filesystem.  This is the way to tell the kernel to change the mount
flags to readonly but not the filesystem.  Think of "bind" as meaning "operate
on the mount, not on the filesystem".  Yes, mount(8) isn't clear that you can do
this.  It does document setting the readonly flag in the context of setting up a
new mount:

	mount --bind olddir newdir
	mount -o remount,ro,bind olddir newdir

The second command is misleading because 'olddir' isn't actually used at all;
the command is really just operating on 'newdir'.

Newer versions of mount also support 'mount -o bind,ro olddir newdir', which is
really only a shorthand for the above two commands.  mount can still only set
the readonly flag on the new mount using the mount syscall with
MS_RDONLY|MS_BIND|MS_REMOUNT.

Perhaps using the new mount syntax to set up a bind mount on a different
directory, rather than reusing the same directory, would make things less
confusing?

Eric

Re: [PATCH 2/4] generic: test setting and getting encryption policies

[Dave Chinner]

On Mon, Nov 21, 2016 at 11:11:45AM -0800, Eric Biggers wrote:
> On Mon, Nov 21, 2016 at 09:07:18AM +1100, Dave Chinner wrote:
> > Also, shouldn't a get without an args parameter always return
> > EINVAL, regardless of whether the underlying file is encrypted or
> > not?
> > 
> 
> For most syscalls/ioctls, including this one (FS_IOC_GET_ENCRYPTION_POLICY),
> it's not expected for the kernel to check that a userspace pointer is NULL as
> opposed to some other random invalid value.  It will only notice at the very end
> of the operation, when copying data to userspace, in which case it's expected to
> fail with EFAULT.

Ok, makes sense.

> > These should all be in a single xfstest that "tests ioctl
> > validity", rather than appended to a "set_policy behaviour"
> > test.
> 
> Yes this may make sense.  It gets a little blurry when you talk
> about testing behavior like "an encryption policy cannot be set on
> a directory", since that's a type of validation too.

Yes, it's a a type of validation, but I see it as a completely
different class of validation. That is, one set of tests is doing
boundary condition testing on the ioctl (i.e. does it reject invalid
input?), the other is doing behavioural tests (i.e. does it behave
correctly on different types of inodes given otherwise valid input?).

> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +here=`pwd`
> > > +echo "QA output created by $seq"
> > > +
> > > +. ./common/encrypt
> > 
> > This is not the way to include all the required scripts, as I
> > mentioned in my last email....
> > 
> > Also, please do not gut the test script preamble - it's there in the
> > new test template for good reason and that is that all the common
> > code that is included relies on the setup it does. e.g. this means $tmp
> > is not properly set, so any common code that has been included that
> > does 'rm -rf $tmp/*' if going to erase your root filesystem.
> > 
> 
> Will do.  I think it's pretty riduculous for xfstests to potentially erase your
> root filesystem if you don't set some random variable though...

$tmp is /not a random variable/. It is required to be set in every
test. The common code is unlikely to need to do something like "rm -rf
$tmp/*" but it does require it to be set. The rm -rf commands are more
likely to be found in test specific _cleanup functions which should
be defined immediately after $tmp....

> What would be nice is for there to be a preamble that all the tests source that
> handles these boilerplate tasks.

Just use the 'new' script to generate your test templates, and you
don't have to care.

> > > +_scratch_mount -o remount,ro,bind
> > 
> > Umm, what does a bind mount do when there's no source/target
> > directory? Whatever you are doing here is not documented in the
> > mount(8) man page....
> 
> As I noted in the comment above this is creating a readonly mount for a
> read-write filesystem.

I realise that. I was commenting on it not being done in the
documented way...

[snip the crazy]

> Perhaps using the new mount syntax to set up a bind mount on a different
> directory, rather than reusing the same directory, would make things less
> confusing?

Yes, that's a good idea...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 3/4] generic: test encrypted file access

[Eric Biggers]

Test accessing encrypted files with and without the encryption key.
Access with the key is more of a sanity check, whereas access without
the key should result in some particular behaviors.

As noted in the comment, as currently written this test is expected to
fail on ext4 pre-4.8 and f2fs pre-4.6.  This could be avoided by using
the filesystem-specific key prefix instead of the generic one, but I'd
prefer to have the tests use the generic prefix.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 tests/generic/401     | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/401.out |   2 +
 tests/generic/group   |   1 +
 3 files changed, 136 insertions(+)
 create mode 100755 tests/generic/401
 create mode 100644 tests/generic/401.out

diff --git a/tests/generic/401 b/tests/generic/401
new file mode 100755
index 0000000..f224852
--- /dev/null
+++ b/tests/generic/401
@@ -0,0 +1,133 @@
+#!/bin/bash
+# FS QA Test generic/401
+#
+# Test accessing encrypted files and directories, both with and without the
+# encryption key.
+#
+# This is *not* intended to fully test all the encrypted I/O paths.  To do that
+# you'd need to run all the xfstests with encryption enabled.
+#
+# Also, this is expected to fail on ext4 pre-4.8 and f2fs pre-4.6 because it
+# assumes the common key naming convention ("fscrypt:" instead of "ext4:" or
+# "f2fs:"), which wasn't added until those versions.
+#
+#-----------------------------------------------------------------------
+# Copyright (C) 2016 Google, Inc.
+#
+# Author: Eric Biggers <ebiggers@google.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, see <http://www.gnu.org/licenses/>.
+#-----------------------------------------------------------------------
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+here=`pwd`
+echo "QA output created by $seq"
+
+. ./common/encrypt
+
+_begin_encryption_test
+
+cd $SCRATCH_MNT
+
+mkdir edir ref_dir
+keydesc=$($FSCRYPT_UTIL gen_key)
+$FSCRYPT_UTIL set_policy $keydesc edir > /dev/null
+for dir in edir ref_dir; do
+	touch $dir/empty > /dev/null
+	$XFS_IO_PROG -t -f -c "pwrite 0 4k" $dir/a > /dev/null
+	$XFS_IO_PROG -t -f -c "pwrite 0 33k" $dir/abcdefghijklmnopqrstuvwxyz > /dev/null
+	maxname=$(yes | head -255 | tr -d '\n') # 255 character filename
+	$XFS_IO_PROG -t -f -c "pwrite 0 1k" $dir/$maxname > /dev/null
+	ln -s a $dir/symlink
+	ln -s abcdefghijklmnopqrstuvwxyz $dir/symlink2
+	ln -s $maxname $dir/symlink3
+	mkdir $dir/subdir
+	mkdir $dir/subdir/subsubdir
+done
+# Diff encrypted directory with unencrypted reference directory
+diff -r edir ref_dir
+# Cycle mount and diff again
+cd $here
+_scratch_cycle_mount
+cd $SCRATCH_MNT
+diff -r edir ref_dir
+
+# Now try accessing the files without the encryption key.
+# It should still be possible to list the directory and remove files.
+# But filenames should be encrypted, and it should not be possible to read
+# regular files or to create new files or subdirectories.
+cd $here
+_scratch_unmount
+$FSCRYPT_UTIL rm_key $keydesc
+_scratch_mount
+cd $SCRATCH_MNT
+if [ $(ls edir | wc -l) -ne 8 ]; then
+	echo "Wrong number of files"
+	exit 1
+fi
+if [ -e edir/empty -o -e edir/symlink ]; then
+	echo "Filenames were not encrypted!"
+	exit 1
+fi
+if [ $(find edir -mindepth 2 -maxdepth 2 -type d | wc -l) -ne 1 ]; then
+	echo "Wrong number of subdirs"
+	exit 1
+fi
+cat $(find edir -maxdepth 1 -type f | head -1) 2>tmp
+if ! egrep -q 'Required key not available' tmp; then
+	echo "Reading encrypted file w/o key didn't fail with ENOKEY"
+	cat tmp
+	exit 1
+fi
+ls -l edir > /dev/null # should succeed
+
+# There are some inconsistencies in which error codes are returned on different
+# kernel versions and filesystems when trying to create a file or subdirectory
+# without access to the parent directory's encryption key.  Furthermore, on some
+# kernels correctly encoded filenames cause a different error (EACCES instead of
+# ENOENT) because these names make it though ->lookup() and fail in ->create()
+# or ->mkdir() instead.  For now we just accept multiple error codes.
+
+$XFS_IO_PROG -f edir/newfile &> tmp
+if ! egrep -q 'Permission denied|No such file or directory' tmp; then
+	echo "Creating file w/o key (unencoded) didn't fail with EACCES or ENOENT"
+	cat tmp
+	exit 1
+fi
+mkdir edir/newdir &> tmp
+if ! egrep -q 'Permission denied|No such file or directory' tmp; then
+	echo "Creating dir w/o key (unencoded) didn't fail with EACCES or ENOENT"
+	cat tmp
+	exit 1
+fi
+$XFS_IO_PROG -f edir/0123456789abcdef &> tmp
+if ! egrep -q 'Permission denied|Operation not permitted' tmp; then
+	echo "Creating file w/o key (encoded) didn't fail with EACCES or EPERM"
+	cat tmp
+	exit 1
+fi
+mkdir edir/0123456789abcdef &> tmp
+if ! egrep -q 'Permission denied|Operation not permitted' tmp; then
+	echo "Creating dir w/o key (encoded) didn't fail with EACCES or EPERM"
+	cat tmp
+	exit 1
+fi
+
+rm -r edir # should succeed
+[ -e edir ] && echo "Directory wasn't deleted!"
+
+echo "Silence is golden."
+
+exit 0
diff --git a/tests/generic/401.out b/tests/generic/401.out
new file mode 100644
index 0000000..3625570
--- /dev/null
+++ b/tests/generic/401.out
@@ -0,0 +1,2 @@
+QA output created by 401
+Silence is golden.
diff --git a/tests/generic/group b/tests/generic/group
index cf89f06..ab4edae 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -393,3 +393,4 @@
 388 auto log metadata
 389 auto quick acl
 400 auto quick encrypt
+401 auto quick encrypt
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 3/4] generic: test encrypted file access

[Dave Chinner]

On Thu, Nov 17, 2016 at 11:47:06AM -0800, Eric Biggers wrote:
> Test accessing encrypted files with and without the encryption key.
> Access with the key is more of a sanity check, whereas access without
> the key should result in some particular behaviors.
> 
> As noted in the comment, as currently written this test is expected to
> fail on ext4 pre-4.8 and f2fs pre-4.6.  This could be avoided by using
> the filesystem-specific key prefix instead of the generic one, but I'd
> prefer to have the tests use the generic prefix.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
....
> +
> +cd $SCRATCH_MNT

Just noticed this, but it's in all the tests. We don't tend to
move into the test directories like this, because that means we
have to cd back out when we need to cycle a mount.

Just prepend things that are created with $SCRATCH_MNT/, or use
local variables for everything that hide this. e.g:

dirs="$SCRATCH_MNT/edir $SCRATCH_MNT/ref_dir"

mkdir $dirs
for dir in $dirs; do
....

> +
> +mkdir edir ref_dir
> +keydesc=$($FSCRYPT_UTIL gen_key)
> +$FSCRYPT_UTIL set_policy $keydesc edir > /dev/null
> +for dir in edir ref_dir; do
> +	touch $dir/empty > /dev/null
> +	$XFS_IO_PROG -t -f -c "pwrite 0 4k" $dir/a > /dev/null
> +	$XFS_IO_PROG -t -f -c "pwrite 0 33k" $dir/abcdefghijklmnopqrstuvwxyz > /dev/null
> +	maxname=$(yes | head -255 | tr -d '\n') # 255 character filename
> +	$XFS_IO_PROG -t -f -c "pwrite 0 1k" $dir/$maxname > /dev/null
> +	ln -s a $dir/symlink
> +	ln -s abcdefghijklmnopqrstuvwxyz $dir/symlink2
> +	ln -s $maxname $dir/symlink3
> +	mkdir $dir/subdir
> +	mkdir $dir/subdir/subsubdir
> +done
> +# Diff encrypted directory with unencrypted reference directory
> +diff -r edir ref_dir
> +# Cycle mount and diff again
> +cd $here
> +_scratch_cycle_mount
> +cd $SCRATCH_MNT
> +diff -r edir ref_dir
> +
> +# Now try accessing the files without the encryption key.
> +# It should still be possible to list the directory and remove files.
> +# But filenames should be encrypted, and it should not be possible to read
> +# regular files or to create new files or subdirectories.
> +cd $here
> +_scratch_unmount
> +$FSCRYPT_UTIL rm_key $keydesc
> +_scratch_mount
> +cd $SCRATCH_MNT
> +if [ $(ls edir | wc -l) -ne 8 ]; then
> +	echo "Wrong number of files"
> +	exit 1
> +fi

Number matching can done at the end of the test via the golden
image comparison. i.e. all you do here is:

ls edir | wc -l

And if the result is wrong then the test harness will catch it at
the end of the test.

But, then again, why wouldn't you just dump:

ls -lR edir |_filter_scratch

to the golden output file to confirm everything is exactly as you
expect it to be in the encrypted directory? It'll catch un-encrypted
names, wrong subdir depth, etc.


> +if [ -e edir/empty -o -e edir/symlink ]; then
> +	echo "Filenames were not encrypted!"
> +	exit 1
> +fi

stat edir/empty edir/symlink > /dev/null

And that will dump the error messages (No such file or directory)
to the golden output file. Again, test harness will catch
mismatches.

> +if [ $(find edir -mindepth 2 -maxdepth 2 -type d | wc -l) -ne 1 ]; then
> +	echo "Wrong number of subdirs"
> +	exit 1
> +fi

find edir -mindepth 2 -maxdepth 2 -type d | wc -l

> +cat $(find edir -maxdepth 1 -type f | head -1) 2>tmp
> +if ! egrep -q 'Required key not available' tmp; then
> +	echo "Reading encrypted file w/o key didn't fail with ENOKEY"
> +	cat tmp
> +	exit 1
> +fi

md5sum `find edir -maxdepth 1 -type f | head -1` | _filter_scratch

You'll either get a md5sum of the data or an error message
in the golden output. The wrong one will trigger a failure.

> +ls -l edir > /dev/null # should succeed

No necessary if you've already use ls -lR to validate all the above
files...

> +# There are some inconsistencies in which error codes are returned on different
> +# kernel versions and filesystems when trying to create a file or subdirectory
> +# without access to the parent directory's encryption key.  Furthermore, on some
> +# kernels correctly encoded filenames cause a different error (EACCES instead of
> +# ENOENT) because these names make it though ->lookup() and fail in ->create()
> +# or ->mkdir() instead.  For now we just accept multiple error codes.
> +
> +$XFS_IO_PROG -f edir/newfile &> tmp
> +if ! egrep -q 'Permission denied|No such file or directory' tmp; then
> +	echo "Creating file w/o key (unencoded) didn't fail with EACCES or ENOENT"
> +	cat tmp
> +	exit 1
> +fi

filter_enoent()
{
	sed -e 's/No such file or directory/Permission denied/'
}

filter_eperm()
{
	sed -e 's/Operation not permitted/Permission denied/'
}

$XFS_IO_PROG -f edir/newfile 2>&1 >/dev/null | _filter_enoent
mkdir edir/newfile  2>&1 >/dev/null | filter_enoent
$XFS_IO_PROG -f edir/0123456789abcdef 2>&1 >/dev/null | filter_eperm
mkdir edir/0123456789abcdef  2>&1 >/dev/null | filter_eperm

> +rm -r edir # should succeed
> +[ -e edir ] && echo "Directory wasn't deleted!"

ls edir

> +echo "Silence is golden."

No, not in this case. :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/4] generic: test encrypted file access

[Eric Biggers]

On Mon, Nov 21, 2016 at 09:31:51AM +1100, Dave Chinner wrote:
> 
> But, then again, why wouldn't you just dump:
> 
> ls -lR edir |_filter_scratch
> 
> to the golden output file to confirm everything is exactly as you
> expect it to be in the encrypted directory? It'll catch un-encrypted
> names, wrong subdir depth, etc.
> 

This won't work because the encrypted filenames are unpredictable.  The
filenames in a directory are encrypted by a key unique to that directory which
is derived from the designated keyring key and a per-inode nonce.  Nonces are
generated randomly by the kernel, so the per-inode encryption keys cannot be
predicted even if you were to put a fixed key into the keyring rather than a
random one.  This is by design because for confidentiality reasons, the same
filename in different directories must not encrypt to the same ciphertext.  A
similar argument applies to the contents of regular files and to symlink
targets.

(Yes, I should make this clear in a comment in the test.)

> 
> > +cat $(find edir -maxdepth 1 -type f | head -1) 2>tmp
> > +if ! egrep -q 'Required key not available' tmp; then
> > +	echo "Reading encrypted file w/o key didn't fail with ENOKEY"
> > +	cat tmp
> > +	exit 1
> > +fi
> 
> md5sum `find edir -maxdepth 1 -type f | head -1` | _filter_scratch
> 
> You'll either get a md5sum of the data or an error message
> in the golden output. The wrong one will trigger a failure.

This won't quite work because the encrypted filename cannot be predicted, but it
would work if the filename were to be filtered out.

I'll take all your suggestions which don't assume predictable filenames.

Thanks,

Eric

Re: [PATCH 3/4] generic: test encrypted file access

[Dave Chinner]

On Mon, Nov 21, 2016 at 11:23:30AM -0800, Eric Biggers wrote:
> On Mon, Nov 21, 2016 at 09:31:51AM +1100, Dave Chinner wrote:
> > 
> > But, then again, why wouldn't you just dump:
> > 
> > ls -lR edir |_filter_scratch
> > 
> > to the golden output file to confirm everything is exactly as you
> > expect it to be in the encrypted directory? It'll catch un-encrypted
> > names, wrong subdir depth, etc.
> > 
> 
> This won't work because the encrypted filenames are unpredictable.  The
> filenames in a directory are encrypted by a key unique to that directory which
> is derived from the designated keyring key and a per-inode nonce.  Nonces are
> generated randomly by the kernel, so the per-inode encryption keys cannot be
> predicted even if you were to put a fixed key into the keyring rather than a
> random one.  This is by design because for confidentiality reasons, the same
> filename in different directories must not encrypt to the same ciphertext.  A
> similar argument applies to the contents of regular files and to symlink
> targets.
> 
> (Yes, I should make this clear in a comment in the test.)

Please do! :P

> 
> > 
> > > +cat $(find edir -maxdepth 1 -type f | head -1) 2>tmp
> > > +if ! egrep -q 'Required key not available' tmp; then
> > > +	echo "Reading encrypted file w/o key didn't fail with ENOKEY"
> > > +	cat tmp
> > > +	exit 1
> > > +fi
> > 
> > md5sum `find edir -maxdepth 1 -type f | head -1` | _filter_scratch
> > 
> > You'll either get a md5sum of the data or an error message
> > in the golden output. The wrong one will trigger a failure.
> 
> This won't quite work because the encrypted filename cannot be predicted, but it
> would work if the filename were to be filtered out.

*nod*. cut or awk will do that quickly and easily, and can replace
the _filter_scratch call...

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

[PATCH 4/4] generic: test locking when setting encryption policy

[Eric Biggers]

This test tries to reproduce (with a moderate chance of success on ext4)
a race condition where a file could be created in a directory
concurrently to an encryption policy being set on that directory,
causing the directory to become corrupted.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 src/fscrypt_util.c    | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/402     | 39 ++++++++++++++++++++
 tests/generic/402.out |  2 ++
 tests/generic/group   |  1 +
 4 files changed, 140 insertions(+)
 create mode 100755 tests/generic/402
 create mode 100644 tests/generic/402.out

diff --git a/src/fscrypt_util.c b/src/fscrypt_util.c
index 9428cb4..5ca0996 100644
--- a/src/fscrypt_util.c
+++ b/src/fscrypt_util.c
@@ -19,11 +19,13 @@
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <inttypes.h>
 #include <linux/fs.h>
 #include <linux/keyctl.h>
+#include <pthread.h>
 #include <stdarg.h>
 #include <stdbool.h>
 #include <stdio.h>
@@ -97,6 +99,7 @@ usage(void)
 "    fscrypt_util rm_key KEYDESC\n"
 "    fscrypt_util set_policy KEYDESC DIR\n"
 "    fscrypt_util test_ioctl_validation DIR\n"
+"    fscrypt_util test_set_policy_locking DIR\n"
 );
 	exit(2);
 }
@@ -357,6 +360,100 @@ static int test_ioctl_validation(int argc, char **argv)
 	return 0;
 }
 
+struct subdir_thread_args {
+	pthread_cond_t cond;
+	pthread_mutex_t mutex;
+	char *subdir;
+	bool done;
+};
+
+static void *subdir_thrproc(void *arg)
+{
+	struct subdir_thread_args *args = arg;
+
+	pthread_mutex_lock(&args->mutex);
+	while (!args->done) {
+		pthread_cond_wait(&args->cond, &args->mutex);
+		mkdir(args->subdir, 0755);
+	}
+	pthread_mutex_unlock(&args->mutex);
+	return NULL;
+}
+
+/*
+ * Test that FS_IOC_SET_ENCRYPTION_POLICY is correctly serialized with regard to
+ * creation of new files in the directory.
+ *
+ * To test this we repeatedly attempt to create a subdirectory concurrently with
+ * setting an encryption policy on the parent directory.  After each attempt, we
+ * do readdir() on the directory.  readdir() should always succeed regardless of
+ * whether the directory ended up with an encryption policy or not.  But without
+ * the proper locking of the directory inode, on ext4 it sometimes failed with
+ * EUCLEAN, and the filesystem was also left in an inconsistent state for fsck.
+ */
+static int test_set_policy_locking(int argc, char **argv)
+{
+	const char *dir;
+	struct subdir_thread_args args;
+	pthread_t subdir_thread;
+	struct fscrypt_policy policy;
+	int i;
+
+	if (argc != 1)
+		usage();
+	dir = argv[0];
+
+	args.subdir = malloc(strlen(dir) + 8);
+	sprintf(args.subdir, "%s/subdir", dir);
+	pthread_cond_init(&args.cond, NULL);
+	pthread_mutex_init(&args.mutex, NULL);
+	args.done = false;
+
+	if (pthread_create(&subdir_thread, NULL, subdir_thrproc, &args) != 0)
+		die("Unable to create thread");
+
+	init_policy_default(&policy);
+
+	for (i = 0; i < 20000; i++) {
+		int fd;
+		DIR *d;
+
+		rmdir(args.subdir);
+		rmdir(dir);
+		mkdir(dir, 0755);
+		fd = open(dir, O_RDONLY);
+		pthread_mutex_lock(&args.mutex);
+		pthread_cond_signal(&args.cond);
+		pthread_mutex_unlock(&args.mutex);
+		if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != 0 &&
+		    errno != ENOTEMPTY) {
+			die_errno("Unexpected error from "
+				  "FS_IOC_SET_ENCRYPTION_POLICY");
+		}
+		d = fdopendir(fd);
+		if (!d)
+			die_errno("Unexpected fdopendir() error");
+		errno = 0;
+		while (readdir(d) != NULL)
+			;
+		if (errno != 0)
+			die_errno("Unexpected readdir() error");
+		closedir(d);
+	}
+
+	pthread_mutex_lock(&args.mutex);
+	args.done = true;
+	pthread_cond_signal(&args.cond);
+	pthread_mutex_unlock(&args.mutex);
+
+	if (pthread_join(subdir_thread, NULL) != 0)
+		die("Unable to join thread");
+
+	free(args.subdir);
+	printf("%s: test_set_policy_locking passed\n", dir);
+	return 0;
+}
+
 static const struct command {
 	const char *name;
 	int (*func)(int, char **);
@@ -365,6 +462,7 @@ static const struct command {
 	{"rm_key", rm_key},
 	{"set_policy", set_policy},
 	{"test_ioctl_validation", test_ioctl_validation},
+	{"test_set_policy_locking", test_set_policy_locking},
 	{NULL, NULL}
 };
 
diff --git a/tests/generic/402 b/tests/generic/402
new file mode 100755
index 0000000..e26c0c9
--- /dev/null
+++ b/tests/generic/402
@@ -0,0 +1,39 @@
+#!/bin/bash
+# FS QA Test generic/402
+#
+# The FS_IOC_SET_ENCRYPTION_POLICY ioctl must be correctly serialized with
+# regard to creation of new files in the directory.  Regression test for
+# 8906a8223ad4: "fscrypto: lock inode while setting encryption policy".
+#
+#-----------------------------------------------------------------------
+# Copyright (C) 2016 Google, Inc.
+#
+# Author: Eric Biggers <ebiggers@google.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, see <http://www.gnu.org/licenses/>.
+#-----------------------------------------------------------------------
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+. ./common/encrypt
+
+_begin_encryption_test
+
+cd $SCRATCH_MNT
+mkdir dir
+$FSCRYPT_UTIL test_set_policy_locking dir
+
+exit 0
diff --git a/tests/generic/402.out b/tests/generic/402.out
new file mode 100644
index 0000000..947e830
--- /dev/null
+++ b/tests/generic/402.out
@@ -0,0 +1,2 @@
+QA output created by 402
+dir: test_set_policy_locking passed
diff --git a/tests/generic/group b/tests/generic/group
index ab4edae..ed6b926 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -394,3 +394,4 @@
 389 auto quick acl
 400 auto quick encrypt
 401 auto quick encrypt
+402 auto encrypt
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 4/4] generic: test locking when setting encryption policy

[Dave Chinner]

On Thu, Nov 17, 2016 at 11:47:07AM -0800, Eric Biggers wrote:
> This test tries to reproduce (with a moderate chance of success on ext4)
> a race condition where a file could be created in a directory
> concurrently to an encryption policy being set on that directory,
> causing the directory to become corrupted.

Why can't this be done with shell loops rather than requiring a
helper application?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/4] generic: test locking when setting encryption policy

[Eric Biggers]

On Mon, Nov 21, 2016 at 09:35:36AM +1100, Dave Chinner wrote:
> On Thu, Nov 17, 2016 at 11:47:07AM -0800, Eric Biggers wrote:
> > This test tries to reproduce (with a moderate chance of success on ext4)
> > a race condition where a file could be created in a directory
> > concurrently to an encryption policy being set on that directory,
> > causing the directory to become corrupted.
> 
> Why can't this be done with shell loops rather than requiring a
> helper application?
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

That's what I tried originally, but the race was too difficult to hit with the
overhead of execing a program for every mkdir and ioctl syscall.

Eric

Re: [PATCH 4/4] generic: test locking when setting encryption policy

[Dave Chinner]

On Mon, Nov 21, 2016 at 11:25:19AM -0800, Eric Biggers wrote:
> On Mon, Nov 21, 2016 at 09:35:36AM +1100, Dave Chinner wrote:
> > On Thu, Nov 17, 2016 at 11:47:07AM -0800, Eric Biggers wrote:
> > > This test tries to reproduce (with a moderate chance of success on ext4)
> > > a race condition where a file could be created in a directory
> > > concurrently to an encryption policy being set on that directory,
> > > causing the directory to become corrupted.
> > 
> > Why can't this be done with shell loops rather than requiring a
> > helper application?
> 
> That's what I tried originally, but the race was too difficult to hit with the
> overhead of execing a program for every mkdir and ioctl syscall.

This means that reproducing the race condition is going to be
machine dependent regardless of how the test is written.

In cases like this for XFS, we tend towards adding a debug sysfs
file to introduce a delay into the code that allows the race to be
triggered reliably. The delay is only included in CONFIG_XFS_DEBUG=y
builds, and the test is conditional on the sysfs file being present.

e.g. xfs/051 uses a log recovery delay to allow us to reliably
trigger IO errors in the middle of log recovery and hence exercise
the IO error failure paths in the middle of recovery. This made an
extremely unreliable reproducer into a test case that triggered
reliably on every machine the test is run on....

Can something like this be done in this case?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/4] generic: test locking when setting encryption policy

[Eric Biggers]

On Tue, Nov 22, 2016 at 08:32:51AM +1100, Dave Chinner wrote:
> 
> This means that reproducing the race condition is going to be
> machine dependent regardless of how the test is written.
> 
> In cases like this for XFS, we tend towards adding a debug sysfs
> file to introduce a delay into the code that allows the race to be
> triggered reliably. The delay is only included in CONFIG_XFS_DEBUG=y
> builds, and the test is conditional on the sysfs file being present.
> 
> e.g. xfs/051 uses a log recovery delay to allow us to reliably
> trigger IO errors in the middle of log recovery and hence exercise
> the IO error failure paths in the middle of recovery. This made an
> extremely unreliable reproducer into a test case that triggered
> reliably on every machine the test is run on....
> 
> Can something like this be done in this case?
> 

I'd really rather not do something like that just for this test, which is really
just testing that the kernel does inode_lock()/inode_unlock() in
fscrypt_process_policy().  It would be more worthwhile if the testing-only
kernel code would help expose many race conditions, not just this one particular
race in this particular ioctl.

So if you don't want to have the C version of this test in xfstests, I think it
should just be dropped from the series.

Eric

Re: [PATCH 4/4] generic: test locking when setting encryption policy

[Dave Chinner]

On Mon, Nov 21, 2016 at 03:41:06PM -0800, Eric Biggers wrote:
> On Tue, Nov 22, 2016 at 08:32:51AM +1100, Dave Chinner wrote:
> > 
> > This means that reproducing the race condition is going to be
> > machine dependent regardless of how the test is written.
> > 
> > In cases like this for XFS, we tend towards adding a debug sysfs
> > file to introduce a delay into the code that allows the race to be
> > triggered reliably. The delay is only included in CONFIG_XFS_DEBUG=y
> > builds, and the test is conditional on the sysfs file being present.
> > 
> > e.g. xfs/051 uses a log recovery delay to allow us to reliably
> > trigger IO errors in the middle of log recovery and hence exercise
> > the IO error failure paths in the middle of recovery. This made an
> > extremely unreliable reproducer into a test case that triggered
> > reliably on every machine the test is run on....
> > 
> > Can something like this be done in this case?
> > 
> 
> I'd really rather not do something like that just for this test, which is really
> just testing that the kernel does inode_lock()/inode_unlock() in
> fscrypt_process_policy().  It would be more worthwhile if the testing-only
> kernel code would help expose many race conditions, not just this one particular
> race in this particular ioctl.

OK.

> So if you don't want to have the C version of this test in
> xfstests, I think it should just be dropped from the series.

If there's no other alternative, a test written in C is better than
nothing.

Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com