Linux-FSCrypt Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
@ 2019-11-07  0:12 Eric Biggers
  2019-11-13 20:35 ` Eric Biggers
  2019-11-15 17:28 ` Jarkko Sakkinen
  0 siblings, 2 replies; 14+ messages in thread
From: Eric Biggers @ 2019-11-07  0:12 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Paul Crowley, Paul Lawrence,
	keyrings, linux-ext4, linux-f2fs-devel, linux-mtd, David Howells,
	Jarkko Sakkinen, Ondrej Mosnacek, Ondrej Kozina

From: Eric Biggers <ebiggers@google.com>

Extend the FS_IOC_ADD_ENCRYPTION_KEY ioctl to allow the raw key to be
specified by a Linux keyring key, rather than specified directly.

This is useful because fscrypt keys belong to a particular filesystem
instance, so they are destroyed when that filesystem is unmounted.
Usually this is desired.  But in some cases, userspace may need to
unmount and re-mount the filesystem while keeping the keys, e.g. during
a system update.  This requires keeping the keys somewhere else too.

The keys could be kept in memory in a userspace daemon.  But depending
on the security architecture and assumptions, it can be preferable to
keep them only in kernel memory, where they are unreadable by userspace.

We also can't solve this by going back to the original fscrypt API
(where for each file, the master key was looked up in the process's
keyring hierarchy) because that caused lots of problems of its own.

Therefore, add the ability for FS_IOC_ADD_ENCRYPTION_KEY to accept a
Linux keyring key.  This solves the problem by allowing userspace to (if
needed) save the keys securely in a Linux keyring for re-provisioning,
while still using the new fscrypt key management ioctls.

This is analogous to how dm-crypt accepts a Linux keyring key, but the
key is then stored internally in the dm-crypt data structures rather
than being looked up again each time the dm-crypt device is accessed.

Use a custom key type "fscrypt-provisioning" rather than one of the
existing key types such as "logon".  This is strongly desired because it
enforces that these keys are only usable for a particular purpose: for
fscrypt as input to a particular KDF.  Otherwise, the keys could also be
passed to any kernel API that accepts a "logon" key with any service
prefix, e.g. dm-crypt, UBIFS, or (recently proposed) AF_ALG.  This would
risk leaking information about the raw key despite it ostensibly being
unreadable.  Of course, this mistake has already been made for multiple
kernel APIs; but since this is a new API, let's do it right.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 Documentation/filesystems/fscrypt.rst |  35 ++++++-
 fs/crypto/keyring.c                   | 126 ++++++++++++++++++++++++--
 include/uapi/linux/fscrypt.h          |  13 ++-
 3 files changed, 162 insertions(+), 12 deletions(-)

diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
index 471a511c75088d..4d15dda36402e0 100644
--- a/Documentation/filesystems/fscrypt.rst
+++ b/Documentation/filesystems/fscrypt.rst
@@ -638,7 +638,8 @@ follows::
     struct fscrypt_add_key_arg {
             struct fscrypt_key_specifier key_spec;
             __u32 raw_size;
-            __u32 __reserved[9];
+            __u32 key_id;
+            __u32 __reserved[8];
             __u8 raw[];
     };
 
@@ -655,6 +656,12 @@ follows::
             } u;
     };
 
+    struct fscrypt_key_provisioning_payload {
+            __u32 type;
+            __u32 __reserved;
+            __u8 raw[];
+    };
+
 :c:type:`struct fscrypt_add_key_arg` must be zeroed, then initialized
 as follows:
 
@@ -677,9 +684,26 @@ as follows:
   ``Documentation/security/keys/core.rst``).
 
 - ``raw_size`` must be the size of the ``raw`` key provided, in bytes.
+  Alternatively, if ``key_id`` is nonzero, this field must be 0, since
+  in that case the size is implied by the specified Linux keyring key.
+
+- ``key_id`` is 0 if the raw key is given directly in the ``raw``
+  field.  Otherwise ``key_id`` is the ID of a Linux keyring key of
+  type "fscrypt-provisioning" whose payload is a ``struct
+  fscrypt_key_provisioning_payload`` whose ``raw`` field contains the
+  raw key and whose ``type`` field matches ``key_spec.type``.  Since
+  ``raw`` is variable-length, the total size of this key's payload
+  must be ``sizeof(struct fscrypt_key_provisioning_payload)`` plus the
+  raw key size.  The process must have Search permission on this key.
+
+  Most users should leave this 0 and specify the raw key directly.
+  The support for specifying a Linux keyring key is intended mainly to
+  allow re-adding keys after a filesystem is unmounted and re-mounted,
+  without having to store the raw keys in userspace memory.
 
 - ``raw`` is a variable-length field which must contain the actual
-  key, ``raw_size`` bytes long.
+  key, ``raw_size`` bytes long.  Alternatively, if ``key_id`` is
+  nonzero, then this field is unused.
 
 For v2 policy keys, the kernel keeps track of which user (identified
 by effective user ID) added the key, and only allows the key to be
@@ -701,11 +725,16 @@ FS_IOC_ADD_ENCRYPTION_KEY can fail with the following errors:
 
 - ``EACCES``: FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR was specified, but the
   caller does not have the CAP_SYS_ADMIN capability in the initial
-  user namespace
+  user namespace; or the raw key was specified by Linux key ID but the
+  process lacks Search permission on the key.
 - ``EDQUOT``: the key quota for this user would be exceeded by adding
   the key
 - ``EINVAL``: invalid key size or key specifier type, or reserved bits
   were set
+- ``EKEYREJECTED``: the raw key was specified by Linux key ID, but the
+  key has the wrong type
+- ``ENOKEY``: the raw key was specified by Linux key ID, but no key
+  exists with that ID
 - ``ENOTTY``: this type of filesystem does not implement encryption
 - ``EOPNOTSUPP``: the kernel was not configured with encryption
   support for this filesystem, or the filesystem superblock has not
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 040df1f5e1c8b1..ef5b171c0f1d64 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -465,6 +465,103 @@ static int add_master_key(struct super_block *sb,
 	return err;
 }
 
+static int fscrypt_provisioning_key_preparse(struct key_preparsed_payload *prep)
+{
+	const struct fscrypt_key_provisioning_payload *payload = prep->data;
+
+	if (prep->datalen < sizeof(*payload) + FSCRYPT_MIN_KEY_SIZE ||
+	    prep->datalen > sizeof(*payload) + FSCRYPT_MAX_KEY_SIZE)
+		return -EINVAL;
+	if (payload->type != FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR &&
+	    payload->type != FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER)
+		return -EINVAL;
+	if (payload->__reserved)
+		return -EINVAL;
+	prep->payload.data[0] = kmemdup(payload, prep->datalen, GFP_KERNEL);
+	if (!prep->payload.data[0])
+		return -ENOMEM;
+	prep->quotalen = prep->datalen;
+	return 0;
+}
+
+static void fscrypt_provisioning_key_free_preparse(
+					struct key_preparsed_payload *prep)
+{
+	kzfree(prep->payload.data[0]);
+}
+
+static void fscrypt_provisioning_key_describe(const struct key *key,
+					      struct seq_file *m)
+{
+	seq_puts(m, key->description);
+	if (key_is_positive(key)) {
+		const struct fscrypt_key_provisioning_payload *payload =
+			key->payload.data[0];
+
+		seq_printf(m, ": %u [%u]", key->datalen, payload->type);
+	}
+}
+
+static void fscrypt_provisioning_key_destroy(struct key *key)
+{
+	kzfree(key->payload.data[0]);
+}
+
+static struct key_type key_type_fscrypt_provisioning = {
+	.name			= "fscrypt-provisioning",
+	.preparse		= fscrypt_provisioning_key_preparse,
+	.free_preparse		= fscrypt_provisioning_key_free_preparse,
+	.instantiate		= generic_key_instantiate,
+	.describe		= fscrypt_provisioning_key_describe,
+	.destroy		= fscrypt_provisioning_key_destroy,
+};
+
+/*
+ * Retrieve the raw key from the Linux keyring key specified by 'key_id', and
+ * store it into 'secret'.
+ *
+ * The key must be of type "fscrypt-provisioning" and must have the field
+ * fscrypt_key_provisioning_payload::type set to 'type', indicating that it's
+ * only usable with fscrypt with the particular KDF version identified by
+ * 'type'.  We don't use the "logon" key type because there's no way to
+ * completely restrict the use of such keys; they can be used by any kernel API
+ * that accepts "logon" keys and doesn't require a specific service prefix.
+ *
+ * The ability to specify the key via Linux keyring key is intended for cases
+ * where userspace needs to re-add keys after the filesystem is unmounted and
+ * re-mounted.  Most users should just provide the raw key directly instead.
+ */
+static int get_keyring_key(u32 key_id, u32 type,
+			   struct fscrypt_master_key_secret *secret)
+{
+	key_ref_t ref;
+	struct key *key;
+	const struct fscrypt_key_provisioning_payload *payload;
+	int err;
+
+	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
+	if (IS_ERR(ref))
+		return PTR_ERR(ref);
+	key = key_ref_to_ptr(ref);
+	if (key->type != &key_type_fscrypt_provisioning)
+		goto bad_key;
+	payload = key->payload.data[0];
+
+	/* Don't allow fscrypt v1 keys to be used as v2 keys and vice versa. */
+	if (payload->type != type)
+		goto bad_key;
+
+	secret->size = key->datalen - sizeof(*payload);
+	memcpy(secret->raw, payload->raw, secret->size);
+	err = 0;
+	goto out_put;
+bad_key:
+	err = -EKEYREJECTED;
+out_put:
+	key_ref_put(ref);
+	return err;
+}
+
 /*
  * Add a master encryption key to the filesystem, causing all files which were
  * encrypted with it to appear "unlocked" (decrypted) when accessed.
@@ -503,18 +600,25 @@ int fscrypt_ioctl_add_key(struct file *filp, void __user *_uarg)
 	if (!valid_key_spec(&arg.key_spec))
 		return -EINVAL;
 
-	if (arg.raw_size < FSCRYPT_MIN_KEY_SIZE ||
-	    arg.raw_size > FSCRYPT_MAX_KEY_SIZE)
-		return -EINVAL;
-
 	if (memchr_inv(arg.__reserved, 0, sizeof(arg.__reserved)))
 		return -EINVAL;
 
 	memset(&secret, 0, sizeof(secret));
-	secret.size = arg.raw_size;
-	err = -EFAULT;
-	if (copy_from_user(secret.raw, uarg->raw, secret.size))
-		goto out_wipe_secret;
+	if (arg.key_id) {
+		if (arg.raw_size != 0)
+			return -EINVAL;
+		err = get_keyring_key(arg.key_id, arg.key_spec.type, &secret);
+		if (err)
+			goto out_wipe_secret;
+	} else {
+		if (arg.raw_size < FSCRYPT_MIN_KEY_SIZE ||
+		    arg.raw_size > FSCRYPT_MAX_KEY_SIZE)
+			return -EINVAL;
+		secret.size = arg.raw_size;
+		err = -EFAULT;
+		if (copy_from_user(secret.raw, uarg->raw, secret.size))
+			goto out_wipe_secret;
+	}
 
 	switch (arg.key_spec.type) {
 	case FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR:
@@ -978,8 +1082,14 @@ int __init fscrypt_init_keyring(void)
 	if (err)
 		goto err_unregister_fscrypt;
 
+	err = register_key_type(&key_type_fscrypt_provisioning);
+	if (err)
+		goto err_unregister_fscrypt_user;
+
 	return 0;
 
+err_unregister_fscrypt_user:
+	unregister_key_type(&key_type_fscrypt_user);
 err_unregister_fscrypt:
 	unregister_key_type(&key_type_fscrypt);
 	return err;
diff --git a/include/uapi/linux/fscrypt.h b/include/uapi/linux/fscrypt.h
index 1beb174ad95056..605dde7343a4e4 100644
--- a/include/uapi/linux/fscrypt.h
+++ b/include/uapi/linux/fscrypt.h
@@ -109,11 +109,22 @@ struct fscrypt_key_specifier {
 	} u;
 };
 
+/*
+ * Payload for Linux keyring key of type "fscrypt-provisioning", referenced by
+ * fscrypt_add_key_arg::key_id as an alternative to fscrypt_add_key_arg::raw.
+ */
+struct fscrypt_key_provisioning_payload {
+	__u32 type;
+	__u32 __reserved;
+	__u8 raw[];
+};
+
 /* Struct passed to FS_IOC_ADD_ENCRYPTION_KEY */
 struct fscrypt_add_key_arg {
 	struct fscrypt_key_specifier key_spec;
 	__u32 raw_size;
-	__u32 __reserved[9];
+	__u32 key_id;
+	__u32 __reserved[8];
 	__u8 raw[];
 };
 
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-07  0:12 [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY Eric Biggers
@ 2019-11-13 20:35 ` Eric Biggers
  2019-11-15 17:29   ` Jarkko Sakkinen
  2019-11-15 17:28 ` Jarkko Sakkinen
  1 sibling, 1 reply; 14+ messages in thread
From: Eric Biggers @ 2019-11-13 20:35 UTC (permalink / raw)
  To: David Howells, Jarkko Sakkinen, keyrings
  Cc: linux-fscrypt, Theodore Y . Ts'o, Ondrej Mosnacek,
	linux-f2fs-devel, Paul Lawrence, linux-mtd, Ondrej Kozina,
	Jaegeuk Kim, linux-ext4, Paul Crowley

On Wed, Nov 06, 2019 at 04:12:59PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Extend the FS_IOC_ADD_ENCRYPTION_KEY ioctl to allow the raw key to be
> specified by a Linux keyring key, rather than specified directly.
> 
> This is useful because fscrypt keys belong to a particular filesystem
> instance, so they are destroyed when that filesystem is unmounted.
> Usually this is desired.  But in some cases, userspace may need to
> unmount and re-mount the filesystem while keeping the keys, e.g. during
> a system update.  This requires keeping the keys somewhere else too.
> 
> The keys could be kept in memory in a userspace daemon.  But depending
> on the security architecture and assumptions, it can be preferable to
> keep them only in kernel memory, where they are unreadable by userspace.
> 
> We also can't solve this by going back to the original fscrypt API
> (where for each file, the master key was looked up in the process's
> keyring hierarchy) because that caused lots of problems of its own.
> 
> Therefore, add the ability for FS_IOC_ADD_ENCRYPTION_KEY to accept a
> Linux keyring key.  This solves the problem by allowing userspace to (if
> needed) save the keys securely in a Linux keyring for re-provisioning,
> while still using the new fscrypt key management ioctls.
> 
> This is analogous to how dm-crypt accepts a Linux keyring key, but the
> key is then stored internally in the dm-crypt data structures rather
> than being looked up again each time the dm-crypt device is accessed.
> 
> Use a custom key type "fscrypt-provisioning" rather than one of the
> existing key types such as "logon".  This is strongly desired because it
> enforces that these keys are only usable for a particular purpose: for
> fscrypt as input to a particular KDF.  Otherwise, the keys could also be
> passed to any kernel API that accepts a "logon" key with any service
> prefix, e.g. dm-crypt, UBIFS, or (recently proposed) AF_ALG.  This would
> risk leaking information about the raw key despite it ostensibly being
> unreadable.  Of course, this mistake has already been made for multiple
> kernel APIs; but since this is a new API, let's do it right.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

David and Jarkko, are you okay with this patch from a keyrings subsystem
perspective?

- Eric

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-07  0:12 [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY Eric Biggers
  2019-11-13 20:35 ` Eric Biggers
@ 2019-11-15 17:28 ` Jarkko Sakkinen
  2019-11-15 19:22   ` Eric Biggers
  1 sibling, 1 reply; 14+ messages in thread
From: Jarkko Sakkinen @ 2019-11-15 17:28 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, Theodore Y . Ts'o, Jaegeuk Kim, Paul Crowley,
	Paul Lawrence, keyrings, linux-ext4, linux-f2fs-devel, linux-mtd,
	David Howells, Ondrej Mosnacek, Ondrej Kozina

On Wed, Nov 06, 2019 at 04:12:59PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Extend the FS_IOC_ADD_ENCRYPTION_KEY ioctl to allow the raw key to be
> specified by a Linux keyring key, rather than specified directly.
> 
> This is useful because fscrypt keys belong to a particular filesystem
> instance, so they are destroyed when that filesystem is unmounted.
> Usually this is desired.  But in some cases, userspace may need to
> unmount and re-mount the filesystem while keeping the keys, e.g. during
> a system update.  This requires keeping the keys somewhere else too.
> 
> The keys could be kept in memory in a userspace daemon.  But depending
> on the security architecture and assumptions, it can be preferable to
> keep them only in kernel memory, where they are unreadable by userspace.
> 
> We also can't solve this by going back to the original fscrypt API
> (where for each file, the master key was looked up in the process's
> keyring hierarchy) because that caused lots of problems of its own.
> 
> Therefore, add the ability for FS_IOC_ADD_ENCRYPTION_KEY to accept a
> Linux keyring key.  This solves the problem by allowing userspace to (if
> needed) save the keys securely in a Linux keyring for re-provisioning,
> while still using the new fscrypt key management ioctls.
> 
> This is analogous to how dm-crypt accepts a Linux keyring key, but the
> key is then stored internally in the dm-crypt data structures rather
> than being looked up again each time the dm-crypt device is accessed.
> 
> Use a custom key type "fscrypt-provisioning" rather than one of the
> existing key types such as "logon".  This is strongly desired because it
> enforces that these keys are only usable for a particular purpose: for
> fscrypt as input to a particular KDF.  Otherwise, the keys could also be
> passed to any kernel API that accepts a "logon" key with any service
> prefix, e.g. dm-crypt, UBIFS, or (recently proposed) AF_ALG.  This would
> risk leaking information about the raw key despite it ostensibly being
> unreadable.  Of course, this mistake has already been made for multiple
> kernel APIs; but since this is a new API, let's do it right.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  Documentation/filesystems/fscrypt.rst |  35 ++++++-
>  fs/crypto/keyring.c                   | 126 ++++++++++++++++++++++++--
>  include/uapi/linux/fscrypt.h          |  13 ++-
>  3 files changed, 162 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> index 471a511c75088d..4d15dda36402e0 100644
> --- a/Documentation/filesystems/fscrypt.rst
> +++ b/Documentation/filesystems/fscrypt.rst
> @@ -638,7 +638,8 @@ follows::
>      struct fscrypt_add_key_arg {
>              struct fscrypt_key_specifier key_spec;
>              __u32 raw_size;
> -            __u32 __reserved[9];
> +            __u32 key_id;
> +            __u32 __reserved[8];
>              __u8 raw[];
>      };
>  
> @@ -655,6 +656,12 @@ follows::
>              } u;
>      };
>  
> +    struct fscrypt_key_provisioning_payload {
> +            __u32 type;
> +            __u32 __reserved;
> +            __u8 raw[];
> +    };
> +
>  :c:type:`struct fscrypt_add_key_arg` must be zeroed, then initialized
>  as follows:
>  
> @@ -677,9 +684,26 @@ as follows:
>    ``Documentation/security/keys/core.rst``).
>  
>  - ``raw_size`` must be the size of the ``raw`` key provided, in bytes.
> +  Alternatively, if ``key_id`` is nonzero, this field must be 0, since
> +  in that case the size is implied by the specified Linux keyring key.
> +
> +- ``key_id`` is 0 if the raw key is given directly in the ``raw``
> +  field.  Otherwise ``key_id`` is the ID of a Linux keyring key of
> +  type "fscrypt-provisioning" whose payload is a ``struct
> +  fscrypt_key_provisioning_payload`` whose ``raw`` field contains the
> +  raw key and whose ``type`` field matches ``key_spec.type``.  Since
> +  ``raw`` is variable-length, the total size of this key's payload
> +  must be ``sizeof(struct fscrypt_key_provisioning_payload)`` plus the
> +  raw key size.  The process must have Search permission on this key.
> +
> +  Most users should leave this 0 and specify the raw key directly.
> +  The support for specifying a Linux keyring key is intended mainly to
> +  allow re-adding keys after a filesystem is unmounted and re-mounted,
> +  without having to store the raw keys in userspace memory.
>  
>  - ``raw`` is a variable-length field which must contain the actual
> -  key, ``raw_size`` bytes long.
> +  key, ``raw_size`` bytes long.  Alternatively, if ``key_id`` is
> +  nonzero, then this field is unused.
>  
>  For v2 policy keys, the kernel keeps track of which user (identified
>  by effective user ID) added the key, and only allows the key to be
> @@ -701,11 +725,16 @@ FS_IOC_ADD_ENCRYPTION_KEY can fail with the following errors:
>  
>  - ``EACCES``: FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR was specified, but the
>    caller does not have the CAP_SYS_ADMIN capability in the initial
> -  user namespace
> +  user namespace; or the raw key was specified by Linux key ID but the
> +  process lacks Search permission on the key.
>  - ``EDQUOT``: the key quota for this user would be exceeded by adding
>    the key
>  - ``EINVAL``: invalid key size or key specifier type, or reserved bits
>    were set
> +- ``EKEYREJECTED``: the raw key was specified by Linux key ID, but the
> +  key has the wrong type
> +- ``ENOKEY``: the raw key was specified by Linux key ID, but no key
> +  exists with that ID
>  - ``ENOTTY``: this type of filesystem does not implement encryption
>  - ``EOPNOTSUPP``: the kernel was not configured with encryption
>    support for this filesystem, or the filesystem superblock has not
> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> index 040df1f5e1c8b1..ef5b171c0f1d64 100644
> --- a/fs/crypto/keyring.c
> +++ b/fs/crypto/keyring.c
> @@ -465,6 +465,103 @@ static int add_master_key(struct super_block *sb,
>  	return err;
>  }
>  
> +static int fscrypt_provisioning_key_preparse(struct key_preparsed_payload *prep)
> +{
> +	const struct fscrypt_key_provisioning_payload *payload = prep->data;
> +
> +	if (prep->datalen < sizeof(*payload) + FSCRYPT_MIN_KEY_SIZE ||
> +	    prep->datalen > sizeof(*payload) + FSCRYPT_MAX_KEY_SIZE)
> +		return -EINVAL;

<empty line>

> +	if (payload->type != FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR &&
> +	    payload->type != FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER)
> +		return -EINVAL;

<empty line>

> +	if (payload->__reserved)
> +		return -EINVAL;

<empty line>

> +	prep->payload.data[0] = kmemdup(payload, prep->datalen, GFP_KERNEL);
> +	if (!prep->payload.data[0])
> +		return -ENOMEM;

<empty line>

> +	prep->quotalen = prep->datalen;

<empty line>

> +	return 0;
> +}
> +
> +static void fscrypt_provisioning_key_free_preparse(
> +					struct key_preparsed_payload *prep)
> +{
> +	kzfree(prep->payload.data[0]);
> +}
> +
> +static void fscrypt_provisioning_key_describe(const struct key *key,
> +					      struct seq_file *m)
> +{
> +	seq_puts(m, key->description);
> +	if (key_is_positive(key)) {
> +		const struct fscrypt_key_provisioning_payload *payload =
> +			key->payload.data[0];
> +
> +		seq_printf(m, ": %u [%u]", key->datalen, payload->type);
> +	}
> +}
> +
> +static void fscrypt_provisioning_key_destroy(struct key *key)
> +{
> +	kzfree(key->payload.data[0]);
> +}
> +
> +static struct key_type key_type_fscrypt_provisioning = {
> +	.name			= "fscrypt-provisioning",
> +	.preparse		= fscrypt_provisioning_key_preparse,
> +	.free_preparse		= fscrypt_provisioning_key_free_preparse,
> +	.instantiate		= generic_key_instantiate,
> +	.describe		= fscrypt_provisioning_key_describe,
> +	.destroy		= fscrypt_provisioning_key_destroy,
> +};
> +
> +/*
> + * Retrieve the raw key from the Linux keyring key specified by 'key_id', and
> + * store it into 'secret'.
> + *
> + * The key must be of type "fscrypt-provisioning" and must have the field
> + * fscrypt_key_provisioning_payload::type set to 'type', indicating that it's
> + * only usable with fscrypt with the particular KDF version identified by
> + * 'type'.  We don't use the "logon" key type because there's no way to
> + * completely restrict the use of such keys; they can be used by any kernel API
> + * that accepts "logon" keys and doesn't require a specific service prefix.
> + *
> + * The ability to specify the key via Linux keyring key is intended for cases
> + * where userspace needs to re-add keys after the filesystem is unmounted and
> + * re-mounted.  Most users should just provide the raw key directly instead.
> + */
> +static int get_keyring_key(u32 key_id, u32 type,
> +			   struct fscrypt_master_key_secret *secret)
> +{
> +	key_ref_t ref;
> +	struct key *key;
> +	const struct fscrypt_key_provisioning_payload *payload;
> +	int err;
> +
> +	ref = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
> +	if (IS_ERR(ref))
> +		return PTR_ERR(ref);

<empty line>

> +	key = key_ref_to_ptr(ref);
> +	if (key->type != &key_type_fscrypt_provisioning)
> +		goto bad_key;

<empty line>

> +	payload = key->payload.data[0];
> +
> +	/* Don't allow fscrypt v1 keys to be used as v2 keys and vice versa. */
> +	if (payload->type != type)
> +		goto bad_key;
> +
> +	secret->size = key->datalen - sizeof(*payload);
> +	memcpy(secret->raw, payload->raw, secret->size);
> +	err = 0;
> +	goto out_put;

<empty line>

> +bad_key:
> +	err = -EKEYREJECTED;

<empty line>

> +out_put:
> +	key_ref_put(ref);
> +	return err;
> +}
> +
>  /*
>   * Add a master encryption key to the filesystem, causing all files which were
>   * encrypted with it to appear "unlocked" (decrypted) when accessed.
> @@ -503,18 +600,25 @@ int fscrypt_ioctl_add_key(struct file *filp, void __user *_uarg)
>  	if (!valid_key_spec(&arg.key_spec))
>  		return -EINVAL;
>  
> -	if (arg.raw_size < FSCRYPT_MIN_KEY_SIZE ||
> -	    arg.raw_size > FSCRYPT_MAX_KEY_SIZE)
> -		return -EINVAL;
> -
>  	if (memchr_inv(arg.__reserved, 0, sizeof(arg.__reserved)))
>  		return -EINVAL;
>  
>  	memset(&secret, 0, sizeof(secret));
> -	secret.size = arg.raw_size;
> -	err = -EFAULT;
> -	if (copy_from_user(secret.raw, uarg->raw, secret.size))
> -		goto out_wipe_secret;
> +	if (arg.key_id) {
> +		if (arg.raw_size != 0)
> +			return -EINVAL;
> +		err = get_keyring_key(arg.key_id, arg.key_spec.type, &secret);
> +		if (err)
> +			goto out_wipe_secret;
> +	} else {
> +		if (arg.raw_size < FSCRYPT_MIN_KEY_SIZE ||
> +		    arg.raw_size > FSCRYPT_MAX_KEY_SIZE)
> +			return -EINVAL;
> +		secret.size = arg.raw_size;
> +		err = -EFAULT;
> +		if (copy_from_user(secret.raw, uarg->raw, secret.size))
> +			goto out_wipe_secret;
> +	}
>  
>  	switch (arg.key_spec.type) {
>  	case FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR:
> @@ -978,8 +1082,14 @@ int __init fscrypt_init_keyring(void)
>  	if (err)
>  		goto err_unregister_fscrypt;
>  
> +	err = register_key_type(&key_type_fscrypt_provisioning);
> +	if (err)
> +		goto err_unregister_fscrypt_user;
> +
>  	return 0;
>  
> +err_unregister_fscrypt_user:
> +	unregister_key_type(&key_type_fscrypt_user);
>  err_unregister_fscrypt:
>  	unregister_key_type(&key_type_fscrypt);
>  	return err;
> diff --git a/include/uapi/linux/fscrypt.h b/include/uapi/linux/fscrypt.h
> index 1beb174ad95056..605dde7343a4e4 100644
> --- a/include/uapi/linux/fscrypt.h
> +++ b/include/uapi/linux/fscrypt.h
> @@ -109,11 +109,22 @@ struct fscrypt_key_specifier {
>  	} u;
>  };
>  
> +/*
> + * Payload for Linux keyring key of type "fscrypt-provisioning", referenced by
> + * fscrypt_add_key_arg::key_id as an alternative to fscrypt_add_key_arg::raw.
> + */
> +struct fscrypt_key_provisioning_payload {
> +	__u32 type;
> +	__u32 __reserved;
> +	__u8 raw[];
> +};
> +
>  /* Struct passed to FS_IOC_ADD_ENCRYPTION_KEY */
>  struct fscrypt_add_key_arg {
>  	struct fscrypt_key_specifier key_spec;
>  	__u32 raw_size;
> -	__u32 __reserved[9];
> +	__u32 key_id;
> +	__u32 __reserved[8];
>  	__u8 raw[];
>  };
>  
> -- 
> 2.24.0.rc1.363.gb1bccd3e3d-goog
> 

I don't see anything obviously wrong. Just would reformat it a bit.
How you tested it?

/Jarkko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-13 20:35 ` Eric Biggers
@ 2019-11-15 17:29   ` Jarkko Sakkinen
  0 siblings, 0 replies; 14+ messages in thread
From: Jarkko Sakkinen @ 2019-11-15 17:29 UTC (permalink / raw)
  To: David Howells, keyrings, linux-fscrypt, Theodore Y . Ts'o,
	Ondrej Mosnacek, linux-f2fs-devel, Paul Lawrence, linux-mtd,
	Ondrej Kozina, Jaegeuk Kim, linux-ext4, Paul Crowley, g

On Wed, Nov 13, 2019 at 12:35:51PM -0800, Eric Biggers wrote:
> On Wed, Nov 06, 2019 at 04:12:59PM -0800, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Extend the FS_IOC_ADD_ENCRYPTION_KEY ioctl to allow the raw key to be
> > specified by a Linux keyring key, rather than specified directly.
> > 
> > This is useful because fscrypt keys belong to a particular filesystem
> > instance, so they are destroyed when that filesystem is unmounted.
> > Usually this is desired.  But in some cases, userspace may need to
> > unmount and re-mount the filesystem while keeping the keys, e.g. during
> > a system update.  This requires keeping the keys somewhere else too.
> > 
> > The keys could be kept in memory in a userspace daemon.  But depending
> > on the security architecture and assumptions, it can be preferable to
> > keep them only in kernel memory, where they are unreadable by userspace.
> > 
> > We also can't solve this by going back to the original fscrypt API
> > (where for each file, the master key was looked up in the process's
> > keyring hierarchy) because that caused lots of problems of its own.
> > 
> > Therefore, add the ability for FS_IOC_ADD_ENCRYPTION_KEY to accept a
> > Linux keyring key.  This solves the problem by allowing userspace to (if
> > needed) save the keys securely in a Linux keyring for re-provisioning,
> > while still using the new fscrypt key management ioctls.
> > 
> > This is analogous to how dm-crypt accepts a Linux keyring key, but the
> > key is then stored internally in the dm-crypt data structures rather
> > than being looked up again each time the dm-crypt device is accessed.
> > 
> > Use a custom key type "fscrypt-provisioning" rather than one of the
> > existing key types such as "logon".  This is strongly desired because it
> > enforces that these keys are only usable for a particular purpose: for
> > fscrypt as input to a particular KDF.  Otherwise, the keys could also be
> > passed to any kernel API that accepts a "logon" key with any service
> > prefix, e.g. dm-crypt, UBIFS, or (recently proposed) AF_ALG.  This would
> > risk leaking information about the raw key despite it ostensibly being
> > unreadable.  Of course, this mistake has already been made for multiple
> > kernel APIs; but since this is a new API, let's do it right.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> 
> David and Jarkko, are you okay with this patch from a keyrings subsystem
> perspective?

Thanks for reminding. Still catching up with keyring. I gave some
feedback to the patch.

/Jarkko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-15 17:28 ` Jarkko Sakkinen
@ 2019-11-15 19:22   ` Eric Biggers
  2019-11-15 22:53     ` Jarkko Sakkinen
  0 siblings, 1 reply; 14+ messages in thread
From: Eric Biggers @ 2019-11-15 19:22 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-fscrypt, Theodore Y . Ts'o, Jaegeuk Kim, Paul Crowley,
	Paul Lawrence, keyrings, linux-ext4, linux-f2fs-devel, linux-mtd,
	David Howells, Ondrej Mosnacek, Ondrej Kozina

On Fri, Nov 15, 2019 at 07:28:53PM +0200, Jarkko Sakkinen wrote:
> 
> I don't see anything obviously wrong. Just would reformat it a bit.
> How you tested it?
> 

I'm not sure all the blank lines you're suggesting would be an improvement.
The ones in fscrypt_provisioning_key_preparse() might make sense though.

I'm working on an xfstest for this:

	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97

It's not quite ready, though.  I'll post it for review when it is.

Someone is also planning to update Android userspace to use this.  So if there
are any issues from that, I'll hear about it.

- Eric

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-15 19:22   ` Eric Biggers
@ 2019-11-15 22:53     ` Jarkko Sakkinen
  2019-11-15 23:04       ` Eric Biggers
  2019-11-16  0:01       ` Theodore Y. Ts'o
  0 siblings, 2 replies; 14+ messages in thread
From: Jarkko Sakkinen @ 2019-11-15 22:53 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, Theodore Y . Ts'o, Jaegeuk Kim, Paul Crowley,
	Paul Lawrence, keyrings, linux-ext4, linux-f2fs-devel, linux-mtd,
	David Howells, Ondrej Mosnacek, Ondrej Kozina

On Fri, Nov 15, 2019 at 11:22:27AM -0800, Eric Biggers wrote:
> On Fri, Nov 15, 2019 at 07:28:53PM +0200, Jarkko Sakkinen wrote:
> > 
> > I don't see anything obviously wrong. Just would reformat it a bit.
> > How you tested it?
> > 
> 
> I'm not sure all the blank lines you're suggesting would be an improvement.
> The ones in fscrypt_provisioning_key_preparse() might make sense though.

OK. Some of this aesthics comes from the feedback that I've received
during Intel SGX patch set review process (of course subsystem is
different i.e. x86). I tend to agree at least that before a new
conditional statement it is more readable if there is a blank line
before it.

> I'm working on an xfstest for this:
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97
> 
> It's not quite ready, though.  I'll post it for review when it is.
> 
> Someone is also planning to update Android userspace to use this.  So if there
> are any issues from that, I'll hear about it.

Cool. Can you combine this patch and matching test (once it is done) to
a patch set?

/Jarkko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-15 22:53     ` Jarkko Sakkinen
@ 2019-11-15 23:04       ` Eric Biggers
  2019-11-18 18:01         ` Jarkko Sakkinen
  2019-11-16  0:01       ` Theodore Y. Ts'o
  1 sibling, 1 reply; 14+ messages in thread
From: Eric Biggers @ 2019-11-15 23:04 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-fscrypt, Theodore Y . Ts'o, Jaegeuk Kim, Paul Crowley,
	Paul Lawrence, keyrings, linux-ext4, linux-f2fs-devel, linux-mtd,
	David Howells, Ondrej Mosnacek, Ondrej Kozina

On Sat, Nov 16, 2019 at 12:53:19AM +0200, Jarkko Sakkinen wrote:
> 
> > I'm working on an xfstest for this:
> > 
> > 	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97
> > 
> > It's not quite ready, though.  I'll post it for review when it is.
> > 
> > Someone is also planning to update Android userspace to use this.  So if there
> > are any issues from that, I'll hear about it.
> 
> Cool. Can you combine this patch and matching test (once it is done) to
> a patch set?
> 

xfstests is developed separately from the kernel (different git repo and
maintainer), so combining kernel and xfstests patches into the same patchset
doesn't make sense.  I can certainly send them out at the same time, though.

- Eric

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-15 22:53     ` Jarkko Sakkinen
  2019-11-15 23:04       ` Eric Biggers
@ 2019-11-16  0:01       ` Theodore Y. Ts'o
  2019-11-17 21:44         ` Darrick J. Wong
  2019-11-18 18:02         ` Jarkko Sakkinen
  1 sibling, 2 replies; 14+ messages in thread
From: Theodore Y. Ts'o @ 2019-11-16  0:01 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Eric Biggers, linux-fscrypt, Jaegeuk Kim, Paul Crowley,
	Paul Lawrence, keyrings, linux-ext4, linux-f2fs-devel, linux-mtd,
	David Howells, Ondrej Mosnacek, Ondrej Kozina

On Sat, Nov 16, 2019 at 12:53:19AM +0200, Jarkko Sakkinen wrote:
> > I'm working on an xfstest for this:
> > 
> > 	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97
> > 
> > It's not quite ready, though.  I'll post it for review when it is.
> > 
> > Someone is also planning to update Android userspace to use this.  So if there
> > are any issues from that, I'll hear about it.
> 
> Cool. Can you combine this patch and matching test (once it is done) to
> a patch set?

That's generally not done since the test goes to a different repo
(xfstests.git) which has a different review process from the kernel
change.

					- Ted

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-16  0:01       ` Theodore Y. Ts'o
@ 2019-11-17 21:44         ` Darrick J. Wong
  2019-11-18 18:02         ` Jarkko Sakkinen
  1 sibling, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2019-11-17 21:44 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: Jarkko Sakkinen, Eric Biggers, linux-fscrypt, Jaegeuk Kim,
	Paul Crowley, Paul Lawrence, keyrings, linux-ext4,
	linux-f2fs-devel, linux-mtd, David Howells, Ondrej Mosnacek,
	Ondrej Kozina

On Fri, Nov 15, 2019 at 07:01:39PM -0500, Theodore Y. Ts'o wrote:
> On Sat, Nov 16, 2019 at 12:53:19AM +0200, Jarkko Sakkinen wrote:
> > > I'm working on an xfstest for this:
> > > 
> > > 	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97
> > > 
> > > It's not quite ready, though.  I'll post it for review when it is.
> > > 
> > > Someone is also planning to update Android userspace to use this.  So if there
> > > are any issues from that, I'll hear about it.
> > 
> > Cool. Can you combine this patch and matching test (once it is done) to
> > a patch set?
> 
> That's generally not done since the test goes to a different repo
> (xfstests.git) which has a different review process from the kernel
> change.

FWIW I generally send one series per git tree (kernel, *progs, fstests)
one right after another so that they'll all land more or less together
in everybody's inboxes.

--D

> 					- Ted

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-15 23:04       ` Eric Biggers
@ 2019-11-18 18:01         ` Jarkko Sakkinen
  2019-11-18 18:14           ` Eric Biggers
  0 siblings, 1 reply; 14+ messages in thread
From: Jarkko Sakkinen @ 2019-11-18 18:01 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, Theodore Y . Ts'o, Jaegeuk Kim, Paul Crowley,
	Paul Lawrence, keyrings, linux-ext4, linux-f2fs-devel, linux-mtd,
	David Howells, Ondrej Mosnacek, Ondrej Kozina

On Fri, Nov 15, 2019 at 03:04:31PM -0800, Eric Biggers wrote:
> On Sat, Nov 16, 2019 at 12:53:19AM +0200, Jarkko Sakkinen wrote:
> > 
> > > I'm working on an xfstest for this:
> > > 
> > > 	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97
> > > 
> > > It's not quite ready, though.  I'll post it for review when it is.
> > > 
> > > Someone is also planning to update Android userspace to use this.  So if there
> > > are any issues from that, I'll hear about it.
> > 
> > Cool. Can you combine this patch and matching test (once it is done) to
> > a patch set?
> > 
> 
> xfstests is developed separately from the kernel (different git repo and
> maintainer), so combining kernel and xfstests patches into the same patchset
> doesn't make sense.  I can certainly send them out at the same time, though.

Is there instructions somewhere how to build and run these tests?

For me it is sufficient if you point a branch and have some kind
of instructions somewhere.

/Jarkko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-16  0:01       ` Theodore Y. Ts'o
  2019-11-17 21:44         ` Darrick J. Wong
@ 2019-11-18 18:02         ` Jarkko Sakkinen
  2019-11-18 18:05           ` Jarkko Sakkinen
  1 sibling, 1 reply; 14+ messages in thread
From: Jarkko Sakkinen @ 2019-11-18 18:02 UTC (permalink / raw)
  To: Theodore Y. Ts'o, g
  Cc: Eric Biggers, linux-fscrypt, Jaegeuk Kim, Paul Crowley,
	Paul Lawrence, keyrings, linux-ext4, linux-f2fs-devel, linux-mtd,
	David Howells, Ondrej Mosnacek, Ondrej Kozina

On Fri, Nov 15, 2019 at 07:01:39PM -0500, Theodore Y. Ts'o wrote:
> On Sat, Nov 16, 2019 at 12:53:19AM +0200, Jarkko Sakkinen wrote:
> > > I'm working on an xfstest for this:
> > > 
> > > 	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97
> > > 
> > > It's not quite ready, though.  I'll post it for review when it is.
> > > 
> > > Someone is also planning to update Android userspace to use this.  So if there
> > > are any issues from that, I'll hear about it.
> > 
> > Cool. Can you combine this patch and matching test (once it is done) to
> > a patch set?
> 
> That's generally not done since the test goes to a different repo
> (xfstests.git) which has a different review process from the kernel
> change.

OK, sorry, both fscrypt and xfstests are both somewhat alien to me. That
is why I'm looking into setting up test environment so that I can review
these patches with a sane judgement.

/Jarkko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-18 18:02         ` Jarkko Sakkinen
@ 2019-11-18 18:05           ` Jarkko Sakkinen
  2019-11-18 18:27             ` Eric Biggers
  0 siblings, 1 reply; 14+ messages in thread
From: Jarkko Sakkinen @ 2019-11-18 18:05 UTC (permalink / raw)
  To: Theodore Y. Ts'o, g
  Cc: Eric Biggers, linux-fscrypt, Jaegeuk Kim, Paul Crowley,
	Paul Lawrence, keyrings, linux-ext4, linux-f2fs-devel, linux-mtd,
	David Howells, Ondrej Mosnacek, Ondrej Kozina

On Mon, Nov 18, 2019 at 08:02:22PM +0200, Jarkko Sakkinen wrote:
> On Fri, Nov 15, 2019 at 07:01:39PM -0500, Theodore Y. Ts'o wrote:
> > On Sat, Nov 16, 2019 at 12:53:19AM +0200, Jarkko Sakkinen wrote:
> > > > I'm working on an xfstest for this:
> > > > 
> > > > 	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97
> > > > 
> > > > It's not quite ready, though.  I'll post it for review when it is.
> > > > 
> > > > Someone is also planning to update Android userspace to use this.  So if there
> > > > are any issues from that, I'll hear about it.
> > > 
> > > Cool. Can you combine this patch and matching test (once it is done) to
> > > a patch set?
> > 
> > That's generally not done since the test goes to a different repo
> > (xfstests.git) which has a different review process from the kernel
> > change.
> 
> OK, sorry, both fscrypt and xfstests are both somewhat alien to me. That
> is why I'm looking into setting up test environment so that I can review
> these patches with a sane judgement.

And also since I've just barely started to help David on co-maintaining
keyring it is better to put extra emphasis on testing even for the most
trivial patches. That is fastest way to learn different interactions.

/Jarkko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-18 18:01         ` Jarkko Sakkinen
@ 2019-11-18 18:14           ` Eric Biggers
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2019-11-18 18:14 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-fscrypt, Theodore Y . Ts'o, Jaegeuk Kim, Paul Crowley,
	Paul Lawrence, keyrings, linux-ext4, linux-f2fs-devel, linux-mtd,
	David Howells, Ondrej Mosnacek, Ondrej Kozina

On Mon, Nov 18, 2019 at 08:01:02PM +0200, Jarkko Sakkinen wrote:
> On Fri, Nov 15, 2019 at 03:04:31PM -0800, Eric Biggers wrote:
> > On Sat, Nov 16, 2019 at 12:53:19AM +0200, Jarkko Sakkinen wrote:
> > > 
> > > > I'm working on an xfstest for this:
> > > > 
> > > > 	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97
> > > > 
> > > > It's not quite ready, though.  I'll post it for review when it is.
> > > > 
> > > > Someone is also planning to update Android userspace to use this.  So if there
> > > > are any issues from that, I'll hear about it.
> > > 
> > > Cool. Can you combine this patch and matching test (once it is done) to
> > > a patch set?
> > > 
> > 
> > xfstests is developed separately from the kernel (different git repo and
> > maintainer), so combining kernel and xfstests patches into the same patchset
> > doesn't make sense.  I can certainly send them out at the same time, though.
> 
> Is there instructions somewhere how to build and run these tests?
> 
> For me it is sufficient if you point a branch and have some kind
> of instructions somewhere.
> 

There are many ways to run xfstests, but I usually use kvm-xfstests.  See the
command to run the encryption tests here:

https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html#tests

More details about kvm-xfstests are here:

https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-xfstests.md

But if you want to run tests which aren't included in the prebuilt kvm-xfstests
test appliance yet (such as the test for this patch), it's not quite as
straightforward since you'll also need to build your own test appliance:

https://github.com/tytso/xfstests-bld/blob/master/Documentation/building-rootfs.md

Also note that this test will require both xfstests and xfsprogs updates:

https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/log/?h=fscrypt-provisioning
https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfsprogs-dev.git/log/?h=fscrypt-provisioning

- Eric

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY
  2019-11-18 18:05           ` Jarkko Sakkinen
@ 2019-11-18 18:27             ` Eric Biggers
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2019-11-18 18:27 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Theodore Y. Ts'o, g, linux-fscrypt, Jaegeuk Kim,
	Paul Crowley, Paul Lawrence, keyrings, linux-ext4,
	linux-f2fs-devel, linux-mtd, David Howells, Ondrej Mosnacek,
	Ondrej Kozina

On Mon, Nov 18, 2019 at 08:05:16PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 18, 2019 at 08:02:22PM +0200, Jarkko Sakkinen wrote:
> > On Fri, Nov 15, 2019 at 07:01:39PM -0500, Theodore Y. Ts'o wrote:
> > > On Sat, Nov 16, 2019 at 12:53:19AM +0200, Jarkko Sakkinen wrote:
> > > > > I'm working on an xfstest for this:
> > > > > 
> > > > > 	https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/xfstests-dev.git/commit/?h=fscrypt-provisioning&id=24ab6abb7cf6a80be44b7c72b73f0519ccaa5a97
> > > > > 
> > > > > It's not quite ready, though.  I'll post it for review when it is.
> > > > > 
> > > > > Someone is also planning to update Android userspace to use this.  So if there
> > > > > are any issues from that, I'll hear about it.
> > > > 
> > > > Cool. Can you combine this patch and matching test (once it is done) to
> > > > a patch set?
> > > 
> > > That's generally not done since the test goes to a different repo
> > > (xfstests.git) which has a different review process from the kernel
> > > change.
> > 
> > OK, sorry, both fscrypt and xfstests are both somewhat alien to me. That
> > is why I'm looking into setting up test environment so that I can review
> > these patches with a sane judgement.
> 
> And also since I've just barely started to help David on co-maintaining
> keyring it is better to put extra emphasis on testing even for the most
> trivial patches. That is fastest way to learn different interactions.
> 

I gave some tips about kvm-xfstests in my other reply:
https://lkml.kernel.org/linux-fscrypt/20191118181359.GA184560@gmail.com/

However, please note that xfstests is really about filesystem testing (including
fscrypt), not about testing the keyrings subsystem itself.  So while you're
certainly welcome to run the fscrypt tests, for most patches you'll encounter as
a keyrings maintainer the keyutils testsuite will be more useful.

- Eric

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-07  0:12 [PATCH] fscrypt: support passing a keyring key to FS_IOC_ADD_ENCRYPTION_KEY Eric Biggers
2019-11-13 20:35 ` Eric Biggers
2019-11-15 17:29   ` Jarkko Sakkinen
2019-11-15 17:28 ` Jarkko Sakkinen
2019-11-15 19:22   ` Eric Biggers
2019-11-15 22:53     ` Jarkko Sakkinen
2019-11-15 23:04       ` Eric Biggers
2019-11-18 18:01         ` Jarkko Sakkinen
2019-11-18 18:14           ` Eric Biggers
2019-11-16  0:01       ` Theodore Y. Ts'o
2019-11-17 21:44         ` Darrick J. Wong
2019-11-18 18:02         ` Jarkko Sakkinen
2019-11-18 18:05           ` Jarkko Sakkinen
2019-11-18 18:27             ` Eric Biggers

Linux-FSCrypt Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fscrypt/0 linux-fscrypt/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fscrypt linux-fscrypt/ https://lore.kernel.org/linux-fscrypt \
		linux-fscrypt@vger.kernel.org
	public-inbox-index linux-fscrypt

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fscrypt


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git