[PATCH] Security: Keys: Added derived keytype

[PATCH] Security: Keys: Added derived keytype

[Kirill Marinushkin]

For details see
Documentation/security/keys-derived.txt

Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
---
 Documentation/security/keys-derived.txt |  82 +++++
 include/keys/derived-type.h             |  33 ++
 security/keys/Kconfig                   |  11 +
 security/keys/Makefile                  |   1 +
 security/keys/derived.c                 | 577 ++++++++++++++++++++++++++++++++
 5 files changed, 704 insertions(+)
 create mode 100644 Documentation/security/keys-derived.txt
 create mode 100644 include/keys/derived-type.h
 create mode 100644 security/keys/derived.c

diff --git a/Documentation/security/keys-derived.txt b/Documentation/security/keys-derived.txt
new file mode 100644
index 0000000..3c1d65c
--- /dev/null
+++ b/Documentation/security/keys-derived.txt
@@ -0,0 +1,82 @@
+			Derived Keys
+
+Derived is a keytype of the kernel keyring facility.
+The key secret is derived from the secret value given by user.
+Optionally user may specify:
+	- hash function used for derivation;
+	- salt value;
+	- number of iterations.
+Both secret value and salt value may be given in one of the formats:
+	- plain data;
+	- hex string;
+	- size of data to generate randomly.
+If no optional parameters are specified, the key is derived from
+the plain secret value with sha256, no salt, 1 iteration.
+Derived keys store as a payload:
+	- derived key;
+	- salt;
+	- number of iterations;
+	- name of derivation algorithm;
+	- name of RNG algorithm.
+From userspace only the derived key value is returned on read.
+
+Usage:
+	keyctl add derived name "key [options]" ring
+
+	mandatory parameter:
+		key					- key secret value
+
+	options:
+		kf=, keyformat=		- key secret value format, see dataformat below
+		s=,  salt=			- salt value,
+								default is empty (no salt)
+		sf=, saltformat=	- salt value format, see dataformat below
+		i=,  iterations=	- number of itaretions,
+								default is 1, maximum is 0x000FFFFF
+		a=,  algorithm=		- name of crypto API hash derivation algorithm,
+								default is sha256
+		r=,  rng=			- name of crypto API RNG algorithm,
+								default is stdrng
+
+	dataformat:
+		plain				- data is a plain value, used by default
+		hex					- data is a hex string
+		rand				- data is a size of random value to be generated
+
+Examples:
+
+Create a simple derived key
+
+	$ keyctl add derived key0 secret0 @u
+	925448848
+
+	$ keyctl read 925448848
+	32 bytes of data in key:
+	97699b7c c0a0ed83 b78b2002 f0e57046 ee561be6 942bec25 6fe201ab ba552a9e
+
+Create a derived key from plain secret, hex salt
+
+	$ keyctl add derived key0 "secret0 s=65a4fe09 sf=hex" @u
+	847728695
+
+	$ keyctl read 847728695
+	32 bytes of data in key:
+	1c64cbb9 cc4dffff a94f8efe dce813d0 5def4a28 97c02336 6c95737b f2b152be
+
+Create a derived key from hex secret value, 32-byte random salt, 65536 iterations
+
+	$keyctl add derived key0 "09afde6781ff kf=hex s=32 sf=rand i=65536" @u
+	604146072
+
+	$ keyctl read 604146072
+	32 bytes of data in key:
+	a5b494b3 b6e3e26c bb9511b1 b16ce60e 99edf63e d8fbc3c2 ba38b195 229e3f43
+
+Create a derived key with sha1 algorithm
+
+	$ keyctl add derived key0 "secret0 a=sha1" @u
+	56670858
+
+	$ keyctl read 56670858
+	20 bytes of data in key:
+	d16cd26f bc3d44a6 16b8d0b2 ce8b5ddc c93e964d
diff --git a/include/keys/derived-type.h b/include/keys/derived-type.h
new file mode 100644
index 0000000..24772a1
--- /dev/null
+++ b/include/keys/derived-type.h
@@ -0,0 +1,33 @@
+/*
+ * Derived key type
+ *
+ * For details see
+ * Documentation/security/keys-derived.txt
+ *
+ * Copyright (C) 2016
+ * Written by Kirill Marinushkin (kmarinushkin@gmail.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ *
+ */
+
+#ifndef INCLUDE_KEYS_DERIVED_TYPE_H_
+#define INCLUDE_KEYS_DERIVED_TYPE_H_
+
+#include <linux/key.h>
+
+extern struct key_type key_type_derived;
+
+extern int derived_instantiate(struct key *key,
+		struct key_preparsed_payload *prep);
+extern int derived_update(struct key *key,
+		struct key_preparsed_payload *prep);
+extern long derived_read(const struct key *key,
+		char __user *buffer, size_t buflen);
+extern void derived_revoke(struct key *key);
+extern void derived_destroy(struct key *key);
+
+#endif /* INCLUDE_KEYS_DERIVED_TYPE_H_ */
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index fe4d74e..261994f 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -81,3 +81,14 @@ config ENCRYPTED_KEYS
 	  Userspace only ever sees/stores encrypted blobs.
 
 	  If you are unsure as to whether this is required, answer N.
+
+config DERIVED_KEYS
+	tristate "Derived keys"
+	depends on KEYS
+	select CRYPTO
+	select CRYPTO_SHA256
+	select CRYPTO_RNG
+	help
+	  This option provides support for derived key type.
+
+	  If you are unsure as to whether this is required, answer N.
diff --git a/security/keys/Makefile b/security/keys/Makefile
index dfb3a7b..fbe954d 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -26,3 +26,4 @@ obj-$(CONFIG_PERSISTENT_KEYRINGS) += persistent.o
 obj-$(CONFIG_BIG_KEYS) += big_key.o
 obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
 obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys/
+obj-$(CONFIG_DERIVED_KEYS) += derived.o
diff --git a/security/keys/derived.c b/security/keys/derived.c
new file mode 100644
index 0000000..18085ce
--- /dev/null
+++ b/security/keys/derived.c
@@ -0,0 +1,577 @@
+/*
+ * Derived key type
+ *
+ * For details see
+ * Documentation/security/keys-derived.txt
+ *
+ * Copyright (C) 2016
+ * Written by Kirill Marinushkin (kmarinushkin@gmail.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/seq_file.h>
+#include <linux/err.h>
+#include <linux/parser.h>
+#include <linux/key.h>
+#include <linux/string.h>
+#include <linux/uaccess.h>
+#include <keys/user-type.h>
+#include <keys/derived-type.h>
+#include <crypto/hash.h>
+#include <crypto/rng.h>
+#include "internal.h"
+
+/* KERN_ERR prefix */
+#define PREFIX	"derived: "
+
+/* Limits */
+#define ITER_MAX_VAL		0x000FFFFF
+#define SALT_MAX_SIZE		1024
+#define RAND_MAX_SIZE		1024
+
+/* Default values */
+#define ITER_DEFAULT		1
+#define ALG_NAME_DEFAULT	"sha256"
+#define RNG_NAME_DEFAULT	"stdrng"
+
+/* Options */
+enum {
+	OPT_SHORT_SALT,
+	OPT_LONG_SALT,
+	OPT_SHORT_ITER,
+	OPT_LONG_ITER,
+	OPT_SHORT_ALG,
+	OPT_LONG_ALG,
+	OPT_SHORT_RNG,
+	OPT_LONG_RNG,
+	OPT_SHORT_KEY_F,
+	OPT_LONG_KEY_F,
+	OPT_SHORT_SALT_F,
+	OPT_LONG_SALT_F
+};
+
+/* Options data formats */
+enum derived_opt_format {
+	OPT_FORMAT_ERR = -1,
+	OPT_FORMAT_PLAIN,
+	OPT_FORMAT_HEX,
+	OPT_FORMAT_RAND
+};
+
+/* Options data index */
+enum {
+	OPT_IND_KEY = 0,
+	OPT_IND_SALT,
+	OPT_IND_NUM /* number of indexes */
+};
+
+struct derived_blob {
+	u8 *data;
+	size_t *lenp;
+};
+
+struct derived_f_blob {
+	enum derived_opt_format format;
+	struct derived_blob *b;
+};
+
+struct derived_key_payload {
+	struct rcu_head rcu;	/* RCU destructor */
+	char *alg_name;			/* null-terminated digest algorithm name */
+	char *rng_name;			/* null-terminated random generator algorithm name */
+	u64 iter;				/* number of iterations */
+	unsigned int saltlen;	/* length of salt */
+	unsigned char *salt;	/* salt */
+	unsigned int datalen;	/* length of derived data */
+	unsigned char *data;	/* derived data */
+};
+
+/* Get option data format specified by user */
+static enum derived_opt_format get_opt_format(const char *arg)
+{
+	if (!strcmp(arg, "plain"))
+		return OPT_FORMAT_PLAIN;
+	if (!strcmp(arg, "hex"))
+		return OPT_FORMAT_HEX;
+	if (!strcmp(arg, "rand"))
+		return OPT_FORMAT_RAND;
+	return OPT_FORMAT_ERR;
+}
+
+/* Generate random data */
+static int gen_random(const char *rnd_name, u8 *buf, unsigned int len)
+{
+	int ret = -EINVAL;
+	struct crypto_rng *rng = NULL;
+
+	rng = crypto_alloc_rng(rnd_name, 0, 0);
+	if (IS_ERR(rng)) {
+		pr_err(PREFIX "RNG alloc failed");
+		return -EINVAL;
+	}
+
+	ret = crypto_rng_get_bytes(rng, buf, len);
+	if (ret < 0) {
+		pr_err(PREFIX "RNG get bytes failed");
+		ret = -EFAULT;
+	}
+
+	if (rng)
+		crypto_free_rng(rng);
+	return ret;
+}
+
+/* Parse options specified by user */
+static int parse_options(char **args_str,
+		struct derived_key_payload *payload, struct derived_blob *ukey)
+{
+	int ret = -EINVAL;
+	substring_t args[MAX_OPT_ARGS];
+	char *p = *args_str;
+	int token;
+	int i;
+	unsigned short templen;
+	unsigned int tempu;
+	u64 tempul;
+	struct derived_blob usalt = {NULL};
+	struct derived_f_blob v[OPT_IND_NUM] = {
+			{OPT_FORMAT_PLAIN, NULL}
+	};
+	const match_table_t key_tokens = {
+			{OPT_SHORT_SALT, "s=%s"},
+			{OPT_LONG_SALT, "salt=%s"},
+			{OPT_SHORT_ITER, "i=%u"},
+			{OPT_LONG_ITER, "iterations=%u"},
+			{OPT_SHORT_ALG, "a=%s"},
+			{OPT_LONG_ALG, "algorithm=%s"},
+			{OPT_SHORT_RNG, "r=%s"},
+			{OPT_LONG_RNG, "rng=%s"},
+			{OPT_SHORT_KEY_F, "kf=%s"},
+			{OPT_LONG_KEY_F, "keyformat=%s"},
+			{OPT_SHORT_SALT_F, "sf=%s"},
+			{OPT_LONG_SALT_F, "saltformat=%s"}
+	};
+
+	/* set defaults */
+	payload->iter = ITER_DEFAULT;
+	payload->alg_name = kstrdup(ALG_NAME_DEFAULT, GFP_KERNEL);
+	if (!payload->alg_name) {
+		pr_err(PREFIX "default algorithm name alloc failed");
+		return -ENOMEM;
+	}
+	payload->rng_name = kstrdup(RNG_NAME_DEFAULT, GFP_KERNEL);
+	if (!payload->rng_name) {
+		pr_err(PREFIX "default RNG name alloc failed");
+		return -ENOMEM;
+	}
+
+	/* parse key */
+	ukey->data = strsep(args_str, " \t");
+	if (!ukey->data) {
+		pr_err(PREFIX "input string separation failed");
+		return -EINVAL;
+	}
+	ukey->lenp = kmalloc(sizeof(*ukey->lenp), GFP_KERNEL);
+	if (!ukey->lenp) {
+		pr_err(PREFIX "input key secret alloc failed");
+		return -ENOMEM;
+	}
+	*ukey->lenp = strlen(ukey->data);
+
+	/* prepare format blob array */
+	v[OPT_IND_KEY].b = ukey;
+	v[OPT_IND_SALT].b = &usalt;
+
+	/* parse options */
+	while ((p = strsep(args_str, " \t"))) {
+		if (*p == '\0' || *p == ' ' || *p == '\t')
+			continue;
+
+		token = match_token(p, key_tokens, args);
+
+		switch (token) {
+
+		case OPT_SHORT_SALT: /* salt */
+		case OPT_LONG_SALT:
+			templen = args[0].to - args[0].from;
+			if (templen < 0 || templen > SALT_MAX_SIZE) {
+				pr_err(PREFIX "invalid salt length");
+				return -EINVAL;
+			}
+			payload->salt = kstrndup(args[0].from, templen, GFP_KERNEL);
+			if (!payload->salt) {
+				pr_err(PREFIX "salt alloc failed");
+				return -ENOMEM;
+			}
+			payload->saltlen = templen;
+			usalt.data = payload->salt;
+			usalt.lenp = &payload->saltlen;
+			break;
+
+		case OPT_SHORT_ITER: /* iterations */
+		case OPT_LONG_ITER:
+			if (kstrtou64(args[0].from, 0, &tempul)
+					|| tempul == 0
+					|| tempul > ITER_MAX_VAL) {
+				pr_err(PREFIX "invalid iterations number");
+				return -EINVAL;
+			}
+			payload->iter = tempul;
+			break;
+
+		case OPT_SHORT_ALG: /* alg name */
+		case OPT_LONG_ALG:
+			payload->alg_name = kstrdup(args[0].from, GFP_KERNEL);
+			if (!payload->alg_name) {
+				pr_err(PREFIX "algorithm name alloc failed");
+				return -ENOMEM;
+			}
+			break;
+
+		case OPT_SHORT_RNG: /* rng name */
+		case OPT_LONG_RNG:
+			payload->rng_name = kstrdup(args[0].from, GFP_KERNEL);
+			if (!payload->rng_name) {
+				pr_err(PREFIX "RNG name alloc failed");
+				return -ENOMEM;
+			}
+			break;
+
+		case OPT_SHORT_KEY_F: /* key format */
+		case OPT_LONG_KEY_F:
+			v[OPT_IND_KEY].format = get_opt_format(args[0].from);
+			if (v[OPT_IND_KEY].format == OPT_FORMAT_ERR) {
+				pr_err(PREFIX "invalid key format");
+				return -EINVAL;
+			}
+			break;
+
+		case OPT_SHORT_SALT_F: /* salt format */
+		case OPT_LONG_SALT_F:
+			v[OPT_IND_SALT].format = get_opt_format(args[0].from);
+			if (v[OPT_IND_SALT].format == OPT_FORMAT_ERR) {
+				pr_err(PREFIX "invalid salt format");
+				return -EINVAL;
+			}
+			break;
+
+		default:
+			pr_err(PREFIX "unsupported option");
+			return -EINVAL;
+		}
+	}
+
+	/* modify options according to format */
+	for (i = 0; i < OPT_IND_NUM; i++) {
+		if (!v[i].b || !v[i].b->data)
+			continue;
+
+		switch (v[i].format) {
+
+		case OPT_FORMAT_HEX:
+			if (*v[i].b->lenp % 2) {
+				pr_err(PREFIX "invalid hex string");
+				return -EINVAL;
+			}
+			*v[i].b->lenp /= 2;
+			ret = hex2bin(v[i].b->data, v[i].b->data, *v[i].b->lenp);
+			if (ret) {
+				pr_err(PREFIX "invalid hex string");
+				return -EINVAL;
+			}
+			break;
+
+		case OPT_FORMAT_RAND:
+			if (kstrtouint(v[i].b->data, 0, &tempu)
+					|| tempu == 0
+					|| tempu > RAND_MAX_SIZE) {
+				pr_err(PREFIX "invalid random size");
+				return -EINVAL;
+			}
+			v[i].b->data = kmalloc(tempu, GFP_KERNEL);
+			if (!v[i].b->data) {
+				pr_err(PREFIX "random data alloc failed");
+				return -ENOMEM;
+			}
+			*v[i].b->lenp = tempu;
+			ret = gen_random(payload->rng_name, v[i].b->data, *v[i].b->lenp);
+			if (ret)
+				return ret;
+			break;
+
+		default:
+			break;
+		}
+	}
+
+	return 0;
+}
+
+/* Free and zero payload fields */
+static void free_payload_content(struct derived_key_payload *payload)
+{
+	if (payload->alg_name)
+		kzfree(payload->alg_name);
+	if (payload->rng_name)
+		kzfree(payload->rng_name);
+	if (payload->data)
+		kzfree(payload->data);
+	if (payload->salt)
+		kzfree(payload->salt);
+}
+
+/* Fill derived key payload with data specified by user */
+static int fill_payload(struct derived_key_payload *payload,
+		struct key_preparsed_payload *prep)
+{
+	int ret = -EINVAL;
+	char *args_str = NULL;
+	struct derived_blob ukey = {NULL};
+	struct crypto_shash *sh = NULL;
+	struct shash_desc *sdesc = NULL;
+	unsigned int i;
+
+	if (!payload || prep->datalen <= 0 || prep->datalen > 32767 || !prep->data) {
+		pr_err(PREFIX "invalid data for payload");
+		return -EINVAL;
+	}
+
+	args_str = kstrndup(prep->data, prep->datalen, GFP_KERNEL);
+	if (!args_str) {
+		pr_err(PREFIX "input arguments alloc failed");
+		return -EINVAL;
+	}
+
+	ret = parse_options(&args_str, payload, &ukey);
+	if (ret)
+		return ret;
+	if (!ukey.data || !ukey.lenp) {
+		pr_err(PREFIX "invalid key input parsed");
+		return -EINVAL;
+	}
+
+	/* start derivation */
+	sh = crypto_alloc_shash(payload->alg_name, 0, 0);
+	if (IS_ERR(sh)) {
+		pr_err(PREFIX "shash alloc failed");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	sdesc = kzalloc(sizeof(struct shash_desc) + crypto_shash_descsize(sh), GFP_KERNEL);
+	if (!sdesc) {
+		pr_err(PREFIX "sdesc alloc failed");
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	sdesc->tfm = sh;
+	sdesc->flags = 0;
+
+	payload->datalen = crypto_shash_digestsize(sh);
+	if (payload->data)
+		kzfree(payload->data);
+	payload->data = kmalloc(payload->datalen, GFP_KERNEL);
+	if (!payload->data) {
+		pr_err(PREFIX "payload data alloc failed");
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < payload->iter; i++) {
+		ret = crypto_shash_init(sdesc);
+		if (ret) {
+			pr_err(PREFIX "shash init failed");
+			goto out;
+		}
+
+		if (i == 0) {
+			/* first iteration */
+			ret = crypto_shash_update(sdesc, ukey.data, *ukey.lenp);
+			if (ret) {
+				pr_err(PREFIX "shash update failed");
+				goto out;
+			}
+
+			ret = crypto_shash_update(sdesc, payload->salt, payload->saltlen);
+			if (ret) {
+				pr_err(PREFIX "shash update failed");
+				goto out;
+			}
+		} else {
+			/* next iterations */
+			ret = crypto_shash_update(sdesc, payload->data, payload->datalen);
+			if (ret) {
+				pr_err(PREFIX "shash update failed");
+				goto out;
+			}
+		}
+
+		ret = crypto_shash_final(sdesc, payload->data);
+		if (ret) {
+			pr_err(PREFIX "shash final failed");
+			goto out;
+		}
+
+	}
+
+out:
+	if (sdesc)
+		kzfree(sdesc);
+	if (!IS_ERR(sh))
+		crypto_free_shash(sh);
+	if (args_str)
+		kzfree(args_str);
+	return ret;
+}
+
+/* Reserve payload for derived key */
+static int reserve_derived_payload(struct key *key,
+		struct derived_key_payload *payload)
+{
+	return key_payload_reserve(key, sizeof(*payload)
+			+ payload->datalen + payload->saltlen
+			+ strlen(payload->alg_name) + strlen(payload->rng_name) + 2);
+}
+
+/* Derived key instantiate */
+int derived_instantiate(struct key *key, struct key_preparsed_payload *prep)
+{
+	int ret = -EINVAL;
+	struct derived_key_payload *payload = NULL;
+
+	if (prep->datalen <= 0 || prep->datalen > 32767 || !prep->data) {
+		pr_err(PREFIX "invalid input data");
+		return -EINVAL;
+	}
+
+	payload = kzalloc(sizeof(*payload), GFP_KERNEL);
+	if (!payload) {
+		pr_err(PREFIX "payload alloc failed");
+		return -ENOMEM;
+	}
+
+	/* fill payload */
+	ret = fill_payload(payload, prep);
+	if (!ret)
+		ret = reserve_derived_payload(key, payload);
+
+	/* assign key if succeed */
+	if (!ret)
+		rcu_assign_keypointer(key, payload);
+	else
+		kzfree(key->payload.data);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(derived_instantiate);
+
+/* Derived key update */
+int derived_update(struct key *key, struct key_preparsed_payload *prep)
+{
+	int ret  = -EINVAL;
+	struct derived_key_payload *payload =
+			(struct derived_key_payload *)key->payload.data;
+
+	/* free current payload */
+	free_payload_content(payload);
+	memset(payload, 0x00, sizeof(*payload));
+
+	ret = fill_payload(payload, prep);
+	if (!ret)
+		ret = reserve_derived_payload(key, payload);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(derived_update);
+
+/* Derived key read */
+long derived_read(const struct key *key, char __user *buffer, size_t buflen)
+{
+	long len = -1;
+	struct derived_key_payload *payload = rcu_dereference_key(key);
+
+	if (!payload) {
+		pr_err(PREFIX "invalid key payload");
+		return -EINVAL;
+	}
+
+	len = payload->datalen;
+	if (buffer && buflen > 0) {
+		/* copy to buffer */
+		if (buflen < payload->datalen
+				|| copy_to_user(buffer, payload->data, payload->datalen)) {
+			pr_err(PREFIX "read key data failed");
+			return -EFAULT;
+		}
+	} /* else return without copy */
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(derived_read);
+
+/* Derived key revoke */
+void derived_revoke(struct key *key)
+{
+	struct derived_key_payload *payload =
+			(struct derived_key_payload *)key->payload.data;
+
+	/* clear the quota */
+	key_payload_reserve(key, 0);
+
+	if (payload) {
+		rcu_assign_keypointer(key, NULL);
+		kfree_rcu(payload, rcu);
+	}
+}
+EXPORT_SYMBOL(derived_revoke);
+
+/* Derived key destroy */
+void derived_destroy(struct key *key)
+{
+	struct derived_key_payload *payload =
+			(struct derived_key_payload *)key->payload.data;
+
+	if (!payload)
+		return;
+
+	free_payload_content(payload);
+
+	kzfree(payload);
+}
+EXPORT_SYMBOL_GPL(derived_destroy);
+
+struct key_type key_type_derived = {
+	.name			= "derived",
+	.instantiate	= derived_instantiate,
+	.update			= derived_update,
+	.destroy		= derived_destroy,
+	.revoke			= derived_revoke,
+	.describe		= user_describe,
+	.read			= derived_read,
+};
+EXPORT_SYMBOL_GPL(key_type_derived);
+
+static int __init init_derived(void)
+{
+	return register_key_type(&key_type_derived);
+}
+
+static void __exit cleanup_derived(void)
+{
+	unregister_key_type(&key_type_derived);
+}
+
+late_initcall(init_derived);
+module_exit(cleanup_derived);
+
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH] Security: Keys: Added derived keytype

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

Hi Kirill,

[auto build test WARNING on v4.5-rc7]
[also build test WARNING on next-20160321]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Kirill-Marinushkin/Security-Keys-Added-derived-keytype/20160322-084809
config: sparc64-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All warnings (new ones prefixed by >>):

   security/keys/derived.c: In function 'parse_options':
>> security/keys/derived.c:217:15: warning: assignment from incompatible pointer type
       usalt.lenp = &payload->saltlen;
                  ^

vim +217 security/keys/derived.c

   201			switch (token) {
   202	
   203			case OPT_SHORT_SALT: /* salt */
   204			case OPT_LONG_SALT:
   205				templen = args[0].to - args[0].from;
   206				if (templen < 0 || templen > SALT_MAX_SIZE) {
   207					pr_err(PREFIX "invalid salt length");
   208					return -EINVAL;
   209				}
   210				payload->salt = kstrndup(args[0].from, templen, GFP_KERNEL);
   211				if (!payload->salt) {
   212					pr_err(PREFIX "salt alloc failed");
   213					return -ENOMEM;
   214				}
   215				payload->saltlen = templen;
   216				usalt.data = payload->salt;
 > 217				usalt.lenp = &payload->saltlen;
   218				break;
   219	
   220			case OPT_SHORT_ITER: /* iterations */
   221			case OPT_LONG_ITER:
   222				if (kstrtou64(args[0].from, 0, &tempul)
   223						|| tempul == 0
   224						|| tempul > ITER_MAX_VAL) {
   225					pr_err(PREFIX "invalid iterations number");

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 45105 bytes --]

Re: [PATCH] Security: Keys: Added derived keytype

[David Howells]

Kirill Marinushkin <k.marinushkin@gmail.com> wrote:

> For details see
> Documentation/security/keys-derived.txt

Usage?

David

Re: [PATCH] Security: Keys: Added derived keytype

[David Howells]

Kirill Marinushkin <k.marinushkin@gmail.com> wrote:

> For details see
> Documentation/security/keys-derived.txt

Please include at least a summary in the patch description, not just a pointer
to the documentation file.

> +			Derived Keys
> +
> +Derived is a keytype of the kernel keyring facility.
> +The key secret is derived from the secret value given by user.

I'm not keen on the type name "derived" as it's totally non-obvious.  How
about "secret", "shared-secret" or "salted" or something like that.

> +		i=,  iterations=	- number of itaretions,

"iterations"

> +#ifndef INCLUDE_KEYS_DERIVED_TYPE_H_
> +#define INCLUDE_KEYS_DERIVED_TYPE_H_

I would drop the initial "INCLUDE_" from that.

> +extern int derived_instantiate(struct key *key,
> +		struct key_preparsed_payload *prep);
> +extern int derived_update(struct key *key,
> +		struct key_preparsed_payload *prep);
> +extern long derived_read(const struct key *key,
> +		char __user *buffer, size_t buflen);
> +extern void derived_revoke(struct key *key);
> +extern void derived_destroy(struct key *key);

Is there a reason you're exporting all the methods?

> +struct derived_key_payload {

Should this struct go in your type header?

> +	struct rcu_head rcu;	/* RCU destructor */
> +	char *alg_name;			/* null-terminated digest algorithm name */
> +	char *rng_name;			/* null-terminated random generator algorithm name */
> +	u64 iter;				/* number of iterations */

Isn't the max value for this 0x000FFFFF?  If so, why is it u64?

> +	unsigned int saltlen;	/* length of salt */
> +	unsigned char *salt;	/* salt */
> +	unsigned int datalen;	/* length of derived data */
> +	unsigned char *data;	/* derived data */

Reorder these to put saltlen and datalen next to each other, thereby
eliminating two holes in the struct on a 64-bit machine.


> +static int gen_random(const char *rnd_name, u8 *buf, unsigned int len)

Prefix with "derived_" please.

> +		case OPT_FORMAT_RAND:
> +			if (kstrtouint(v[i].b->data, 0, &tempu)
> +					|| tempu == 0
> +					|| tempu > RAND_MAX_SIZE) {
> +				pr_err(PREFIX "invalid random size");
> +				return -EINVAL;
> +			}
> +			v[i].b->data = kmalloc(tempu, GFP_KERNEL);
> +			if (!v[i].b->data) {
> +				pr_err(PREFIX "random data alloc failed");
> +				return -ENOMEM;
> +			}
> +			*v[i].b->lenp = tempu;
> +			ret = gen_random(payload->rng_name, v[i].b->data, *v[i].b->lenp);

I would move the kmalloc() inside the gen_random() function.

> +static void free_payload_content(struct derived_key_payload *payload)
> +{
> +	if (payload->alg_name)
> +		kzfree(payload->alg_name);
> +	if (payload->rng_name)
> +		kzfree(payload->rng_name);
> +	if (payload->data)
> +		kzfree(payload->data);
> +	if (payload->salt)
> +		kzfree(payload->salt);
> +}

kzfree() can handle a NULL pointer.  You've got more instances of this.

Your functions should all be prefixed with "derived_".

> +	sdesc = kzalloc(sizeof(struct shash_desc) + crypto_shash_descsize(sh), GFP_KERNEL);

Do you need some wrappers on this to get the alignment correct?

> +	if (!sdesc) {
> +		pr_err(PREFIX "sdesc alloc failed");

Don't print an error here.

> +		ret = -ENOMEM;
> +		goto out;
> +	}

You should stick a label about four lines below "out:" and go there instead.
Then you can get rid of the conditionalisation in the following:

	+	if (!IS_ERR(sh))
	+		crypto_free_shash(sh);

> +	payload = kzalloc(sizeof(*payload), GFP_KERNEL);
> +	if (!payload) {
> +		pr_err(PREFIX "payload alloc failed");
> +		return -ENOMEM;
> +	}
> +
> +	/* fill payload */
> +	ret = fill_payload(payload, prep);

Move the kzalloc() call into fill_payload().

> +int derived_update(struct key *key, struct key_preparsed_payload *prep)
> +{
> +	int ret  = -EINVAL;
> +	struct derived_key_payload *payload =
> +			(struct derived_key_payload *)key->payload.data;
> +
> +	/* free current payload */
> +	free_payload_content(payload);
> +	memset(payload, 0x00, sizeof(*payload));
> +
> +	ret = fill_payload(payload, prep);
> +	if (!ret)
> +		ret = reserve_derived_payload(key, payload);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(derived_update);

This is *not* RCU safe.

You should implement the ->preparse() method and do the argument parsing and
creation and filling in of struct derived_key_payload there.  Take a look at
user_preparse().  I would start by renaming fill_payload() to
derived_preparse() - it's almost exactly what you want.

You will also need to implement ->free_preparse().

You can then get rid of reserve_derived_payload() and just put the quota
amount into prep->quotalen and the payload into prep->payload.data[0].

derived_instantiate() can then be replaced with generic_key_instantiate.

Since derived_preparse() would be called prior to derived_update(), the latter
can just replace where prep->payload.data[0] points using
rcu_assign_keypointer() and then call_rcu() on the old payload.

David

Re: [PATCH] Security: Keys: Added derived keytype

[David Howells]

Kirill Marinushkin <k.marinushkin@gmail.com> wrote:

> kernel space:
>     derive keys from "trusted" (with possibility to access from user space if proper permissions are set);
> user space:
>     store passwords within keyrings;
>     randomly generated keys, keys with payload given as hex string.
> 
> What's your opinion on having derived keytype?

Ummm...  I'm not keen on the name; it doesn't really capture what the key is
for.

Apart from that, let me have another look through the patch.

David

Re: [PATCH] Security: Keys: Added derived keytype

[Kirill Marinushkin]

>> For details see
>> Documentation/security/keys-derived.txt
>
> Usage?
>
> David

You maybe didn't receive my answer sent from gmail web-interface, so I repeat it now from mutt.

Usages of derived keys that I can see:

kernel space:
    derive keys from "trusted" (with possibility to access from user space if proper permissions are set);
user space:
    store passwords within keyrings;
    randomly generated keys, keys with payload given as hex string.

What's your opinion on having derived keytype?

--
Best Regards,
Kirill Marinushkin