Linux Security Modules development
 help / color / mirror / Atom feed

* [RFC 5/7] KEYS: encrypted: Allow TEE based trusted master keys
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
  To: keyrings, linux-integrity, linux-security-module
  Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
	jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
	linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>

Allow search for TEE based trusted keys to act as master keys in case
TPM device is not present.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 security/keys/encrypted-keys/masterkey_trusted.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
index c68528a..cfac27f 100644
--- a/security/keys/encrypted-keys/masterkey_trusted.c
+++ b/security/keys/encrypted-keys/masterkey_trusted.c
@@ -23,6 +23,9 @@
  * Trusted keys are sealed to PCRs and other metadata. Although userspace
  * manages both trusted/encrypted key-types, like the encrypted key type
  * data, trusted key type data is not visible decrypted from userspace.
+ *
+ * Also, check for alternate trusted keys provided via TEE in case there
+ * is no TPM available.
  */
 struct key *request_trusted_key(const char *trusted_desc,
 				const u8 **master_key, size_t *master_keylen)
@@ -31,8 +34,11 @@ struct key *request_trusted_key(const char *trusted_desc,
 	struct key *tkey;
 
 	tkey = request_key(&key_type_trusted, trusted_desc, NULL);
-	if (IS_ERR(tkey))
-		goto error;
+	if (IS_ERR(tkey)) {
+		tkey = request_key(&key_type_tee_trusted, trusted_desc, NULL);
+		if (IS_ERR(tkey))
+			goto error;
+	}
 
 	down_read(&tkey->sem);
 	tpayload = tkey->payload.data[0];
-- 
2.7.4


^ permalink raw reply related

* [RFC 4/7] KEYS: trusted: Introduce TEE based Trusted Keys
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
  To: keyrings, linux-integrity, linux-security-module
  Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
	jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
	linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>

Add support for TEE based trusted keys where TEE provides the functionality
to seal and unseal trusted keys using hardware unique key.

Refer to Documentation/tee.txt for detailed information about TEE.

Approach taken in this patch acts as an alternative to a TPM device in case
platform doesn't possess one.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 include/keys/tee_trusted.h  |  84 ++++++++
 include/keys/trusted-type.h |   1 +
 security/keys/Kconfig       |   3 +
 security/keys/Makefile      |   3 +
 security/keys/tee_trusted.c | 506 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 597 insertions(+)
 create mode 100644 include/keys/tee_trusted.h
 create mode 100644 security/keys/tee_trusted.c

diff --git a/include/keys/tee_trusted.h b/include/keys/tee_trusted.h
new file mode 100644
index 0000000..e5c0042
--- /dev/null
+++ b/include/keys/tee_trusted.h
@@ -0,0 +1,84 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019 Linaro Ltd.
+ *
+ * Author:
+ * Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#ifndef __TEE_TRUSTED_KEY_H
+#define __TEE_TRUSTED_KEY_H
+
+#include <linux/tee_drv.h>
+
+#define DRIVER_NAME "tee-trusted-key"
+
+/*
+ * Get random data for symmetric key
+ *
+ * [out]     memref[0]        Random data
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_GET_RANDOM	0x0
+
+/*
+ * Seal trusted key using hardware unique key
+ *
+ * [in]      memref[0]        Plain key
+ * [out]     memref[1]        Sealed key datablob
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_SEAL		0x1
+
+/*
+ * Unseal trusted key using hardware unique key
+ *
+ * [in]      memref[0]        Sealed key datablob
+ * [out]     memref[1]        Plain key
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_UNSEAL		0x2
+
+/**
+ * struct trusted_key_private - TEE Trusted key private data
+ * @dev:		TEE based Trusted key device.
+ * @ctx:		TEE context handler.
+ * @session_id:		Trusted key TA session identifier.
+ * @shm_pool:		Memory pool shared with TEE device.
+ */
+struct trusted_key_private {
+	struct device *dev;
+	struct tee_context *ctx;
+	u32 session_id;
+	u32 data_rate;
+	struct tee_shm *shm_pool;
+};
+
+#define TEE_KEY_DEBUG 0
+
+#if TEE_KEY_DEBUG
+static inline void dump_tee_payload(struct trusted_key_payload *p)
+{
+	pr_info("trusted_key: key_len %d\n", p->key_len);
+	print_hex_dump(KERN_INFO, "key ", DUMP_PREFIX_NONE,
+		       16, 1, p->key, p->key_len, 0);
+	pr_info("trusted_key: bloblen %d\n", p->blob_len);
+	print_hex_dump(KERN_INFO, "blob ", DUMP_PREFIX_NONE,
+		       16, 1, p->blob, p->blob_len, 0);
+}
+#else
+static inline void dump_tee_payload(struct trusted_key_payload *p)
+{
+}
+#endif
+
+#endif
diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index a94c03a..363ec83 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -41,5 +41,6 @@ struct trusted_key_options {
 };
 
 extern struct key_type key_type_trusted;
+extern struct key_type key_type_tee_trusted;
 
 #endif /* _KEYS_TRUSTED_TYPE_H */
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index ee502e4..b206a20 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -70,6 +70,9 @@ config TRUSTED_KEYS
 	  if the boot PCRs and other criteria match.  Userspace will only ever
 	  see encrypted blobs.
 
+	  It also provides support for alternative TEE based Trusted keys
+	  generation and sealing in case TPM isn't present.
+
 	  If you are unsure as to whether this is required, answer N.
 
 config ENCRYPTED_KEYS
diff --git a/security/keys/Makefile b/security/keys/Makefile
index 9cef540..07ad3e2 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -30,3 +30,6 @@ obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += keyctl_pkey.o
 obj-$(CONFIG_BIG_KEYS) += big_key.o
 obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
 obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys/
+ifdef CONFIG_TEE
+obj-$(CONFIG_TRUSTED_KEYS) += tee_trusted.o
+endif
diff --git a/security/keys/tee_trusted.c b/security/keys/tee_trusted.c
new file mode 100644
index 0000000..081e45e
--- /dev/null
+++ b/security/keys/tee_trusted.c
@@ -0,0 +1,506 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Linaro Ltd.
+ *
+ * Author:
+ * Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#include <linux/err.h>
+#include <linux/key-type.h>
+#include <linux/module.h>
+#include <linux/parser.h>
+#include <linux/rcupdate.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/tpm.h>
+#include <linux/uaccess.h>
+#include <linux/uuid.h>
+
+#include <keys/trusted-type.h>
+#include <keys/user-type.h>
+#include <keys/tee_trusted.h>
+
+static struct trusted_key_private pvt_data;
+
+/*
+ * Have the TEE seal(encrypt) the symmetric key
+ */
+static int tee_key_seal(struct trusted_key_payload *p)
+{
+	int ret = 0;
+	struct tee_ioctl_invoke_arg inv_arg;
+	struct tee_param param[4];
+	struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
+
+	memset(&inv_arg, 0, sizeof(inv_arg));
+	memset(&param, 0, sizeof(param));
+
+	reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
+				      p->key_len, TEE_SHM_DMA_BUF |
+				      TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm_in)) {
+		dev_err(pvt_data.dev, "key shm register failed\n");
+		return PTR_ERR(reg_shm_in);
+	}
+
+	reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
+				       sizeof(p->blob), TEE_SHM_DMA_BUF |
+				       TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm_out)) {
+		dev_err(pvt_data.dev, "blob shm register failed\n");
+		ret = PTR_ERR(reg_shm_out);
+		goto out;
+	}
+
+	inv_arg.func = TA_CMD_SEAL;
+	inv_arg.session = pvt_data.session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+	param[0].u.memref.shm = reg_shm_in;
+	param[0].u.memref.size = p->key_len;
+	param[0].u.memref.shm_offs = 0;
+	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[1].u.memref.shm = reg_shm_out;
+	param[1].u.memref.size = sizeof(p->blob);
+	param[1].u.memref.shm_offs = 0;
+
+	ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(pvt_data.dev, "TA_CMD_SEAL invoke err: %x\n",
+			inv_arg.ret);
+		ret = -EFAULT;
+	} else {
+		p->blob_len = param[1].u.memref.size;
+	}
+
+out:
+	if (reg_shm_out)
+		tee_shm_free(reg_shm_out);
+	if (reg_shm_in)
+		tee_shm_free(reg_shm_in);
+
+	return ret;
+}
+
+/*
+ * Have the TEE unseal(decrypt) the symmetric key
+ */
+static int tee_key_unseal(struct trusted_key_payload *p)
+{
+	int ret = 0;
+	struct tee_ioctl_invoke_arg inv_arg;
+	struct tee_param param[4];
+	struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
+
+	memset(&inv_arg, 0, sizeof(inv_arg));
+	memset(&param, 0, sizeof(param));
+
+	reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
+				      p->blob_len, TEE_SHM_DMA_BUF |
+				      TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm_in)) {
+		dev_err(pvt_data.dev, "blob shm register failed\n");
+		return PTR_ERR(reg_shm_in);
+	}
+
+	reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
+				       sizeof(p->key), TEE_SHM_DMA_BUF |
+				       TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm_out)) {
+		dev_err(pvt_data.dev, "key shm register failed\n");
+		ret = PTR_ERR(reg_shm_out);
+		goto out;
+	}
+
+	inv_arg.func = TA_CMD_UNSEAL;
+	inv_arg.session = pvt_data.session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+	param[0].u.memref.shm = reg_shm_in;
+	param[0].u.memref.size = p->blob_len;
+	param[0].u.memref.shm_offs = 0;
+	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[1].u.memref.shm = reg_shm_out;
+	param[1].u.memref.size = sizeof(p->key);
+	param[1].u.memref.shm_offs = 0;
+
+	ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(pvt_data.dev, "TA_CMD_UNSEAL invoke err: %x\n",
+			inv_arg.ret);
+		ret = -EFAULT;
+	} else {
+		p->key_len = param[1].u.memref.size;
+	}
+
+out:
+	if (reg_shm_out)
+		tee_shm_free(reg_shm_out);
+	if (reg_shm_in)
+		tee_shm_free(reg_shm_in);
+
+	return ret;
+}
+
+/*
+ * Have the TEE generate random symmetric key
+ */
+static int tee_get_random(unsigned char *key, unsigned int key_len)
+{
+	int ret = 0;
+	struct tee_ioctl_invoke_arg inv_arg;
+	struct tee_param param[4];
+	struct tee_shm *reg_shm = NULL;
+
+	memset(&inv_arg, 0, sizeof(inv_arg));
+	memset(&param, 0, sizeof(param));
+
+	reg_shm = tee_shm_register(pvt_data.ctx, (unsigned long)key, key_len,
+				   TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm)) {
+		dev_err(pvt_data.dev, "random key shm register failed\n");
+		return PTR_ERR(reg_shm);
+	}
+
+	inv_arg.func = TA_CMD_GET_RANDOM;
+	inv_arg.session = pvt_data.session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[0].u.memref.shm = reg_shm;
+	param[0].u.memref.size = key_len;
+	param[0].u.memref.shm_offs = 0;
+
+	ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(pvt_data.dev, "TA_CMD_GET_RANDOM invoke err: %x\n",
+			inv_arg.ret);
+		ret = -EFAULT;
+	} else {
+		ret = param[0].u.memref.size;
+	}
+
+	tee_shm_free(reg_shm);
+
+	return ret;
+}
+
+enum {
+	Opt_err,
+	Opt_new, Opt_load
+};
+
+static const match_table_t key_tokens = {
+	{Opt_new, "new"},
+	{Opt_load, "load"},
+	{Opt_err, NULL}
+};
+
+/*
+ * datablob_parse - parse the keyctl data and fill in the
+ *		    payload structure
+ *
+ * On success returns 0, otherwise -EINVAL.
+ */
+static int datablob_parse(char *datablob, struct trusted_key_payload *p)
+{
+	substring_t args[MAX_OPT_ARGS];
+	long keylen;
+	int ret = -EINVAL;
+	int key_cmd;
+	char *c;
+
+	/* main command */
+	c = strsep(&datablob, " \t");
+	if (!c)
+		return -EINVAL;
+
+	key_cmd = match_token(c, key_tokens, args);
+	switch (key_cmd) {
+	case Opt_new:
+		/* first argument is key size */
+		c = strsep(&datablob, " \t");
+		if (!c)
+			return -EINVAL;
+		ret = kstrtol(c, 10, &keylen);
+		if (ret < 0 || keylen < MIN_KEY_SIZE || keylen > MAX_KEY_SIZE)
+			return -EINVAL;
+		p->key_len = keylen;
+		ret = Opt_new;
+		break;
+	case Opt_load:
+		/* first argument is sealed blob */
+		c = strsep(&datablob, " \t");
+		if (!c)
+			return -EINVAL;
+		p->blob_len = strlen(c) / 2;
+		if (p->blob_len > MAX_BLOB_SIZE)
+			return -EINVAL;
+		ret = hex2bin(p->blob, c, p->blob_len);
+		if (ret < 0)
+			return -EINVAL;
+		ret = Opt_load;
+		break;
+	case Opt_err:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
+{
+	struct trusted_key_payload *p = NULL;
+	int ret;
+
+	ret = key_payload_reserve(key, sizeof(*p));
+	if (ret < 0)
+		return p;
+
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+
+	return p;
+}
+
+/*
+ * trusted_instantiate - create a new trusted key
+ *
+ * Unseal an existing trusted blob or, for a new key, get a
+ * random key, then seal and create a trusted key-type key,
+ * adding it to the specified keyring.
+ *
+ * On success, return 0. Otherwise return errno.
+ */
+static int trusted_instantiate(struct key *key,
+			       struct key_preparsed_payload *prep)
+{
+	struct trusted_key_payload *payload = NULL;
+	size_t datalen = prep->datalen;
+	char *datablob;
+	int ret = 0;
+	int key_cmd;
+	size_t key_len;
+
+	if (datalen <= 0 || datalen > 32767 || !prep->data)
+		return -EINVAL;
+
+	datablob = kmalloc(datalen + 1, GFP_KERNEL);
+	if (!datablob)
+		return -ENOMEM;
+	memcpy(datablob, prep->data, datalen);
+	datablob[datalen] = '\0';
+
+	payload = trusted_payload_alloc(key);
+	if (!payload) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	key_cmd = datablob_parse(datablob, payload);
+	if (key_cmd < 0) {
+		ret = key_cmd;
+		goto out;
+	}
+
+	dump_tee_payload(payload);
+
+	switch (key_cmd) {
+	case Opt_load:
+		ret = tee_key_unseal(payload);
+		dump_tee_payload(payload);
+		if (ret < 0)
+			dev_err(pvt_data.dev, "key_unseal failed (%d)\n", ret);
+		break;
+	case Opt_new:
+		key_len = payload->key_len;
+		ret = tee_get_random(payload->key, key_len);
+		if (ret != key_len) {
+			dev_err(pvt_data.dev, "key_create failed (%d)\n", ret);
+			goto out;
+		}
+
+		ret = tee_key_seal(payload);
+		if (ret < 0)
+			dev_err(pvt_data.dev, "key_seal failed (%d)\n", ret);
+		dump_tee_payload(payload);
+		break;
+	default:
+		ret = -EINVAL;
+		goto out;
+	}
+out:
+	kzfree(datablob);
+	if (!ret)
+		rcu_assign_keypointer(key, payload);
+	else
+		kzfree(payload);
+	return ret;
+}
+
+static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
+{
+	dev_info(pvt_data.dev, "trusted key update method not supported\n");
+
+	return -EINVAL;
+}
+
+/*
+ * trusted_read - copy the sealed blob data to userspace in hex.
+ * On success, return to userspace the trusted key datablob size.
+ */
+static long trusted_read(const struct key *key, char __user *buffer,
+			 size_t buflen)
+{
+	const struct trusted_key_payload *p;
+	char *ascii_buf;
+	char *bufp;
+	int i;
+
+	p = dereference_key_locked(key);
+	if (!p)
+		return -EINVAL;
+
+	if (buffer && buflen >= 2 * p->blob_len) {
+		ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
+		if (!ascii_buf)
+			return -ENOMEM;
+
+		bufp = ascii_buf;
+		for (i = 0; i < p->blob_len; i++)
+			bufp = hex_byte_pack(bufp, p->blob[i]);
+		if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
+			kzfree(ascii_buf);
+			return -EFAULT;
+		}
+		kzfree(ascii_buf);
+	}
+	return 2 * p->blob_len;
+}
+
+/*
+ * trusted_destroy - clear and free the key's payload
+ */
+static void trusted_destroy(struct key *key)
+{
+	kzfree(key->payload.data[0]);
+}
+
+struct key_type key_type_tee_trusted = {
+	.name = "trusted",
+	.instantiate = trusted_instantiate,
+	.update = trusted_update,
+	.destroy = trusted_destroy,
+	.describe = user_describe,
+	.read = trusted_read,
+};
+EXPORT_SYMBOL_GPL(key_type_tee_trusted);
+
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	if (ver->impl_id == TEE_IMPL_ID_OPTEE)
+		return 1;
+	else
+		return 0;
+}
+
+static int trusted_key_probe(struct device *dev)
+{
+	struct tee_client_device *rng_device = to_tee_client_device(dev);
+	int ret = 0, err = -ENODEV;
+	struct tee_ioctl_open_session_arg sess_arg;
+
+	memset(&sess_arg, 0, sizeof(sess_arg));
+
+	/* Open context with TEE driver */
+	pvt_data.ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
+					       NULL);
+	if (IS_ERR(pvt_data.ctx))
+		return -ENODEV;
+
+	/* Open session with hwrng Trusted App */
+	memcpy(sess_arg.uuid, rng_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
+	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+	sess_arg.num_params = 0;
+
+	ret = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL);
+	if ((ret < 0) || (sess_arg.ret != 0)) {
+		dev_err(dev, "tee_client_open_session failed, err: %x\n",
+			sess_arg.ret);
+		err = -EINVAL;
+		goto out_ctx;
+	}
+	pvt_data.session_id = sess_arg.session;
+
+	ret = register_key_type(&key_type_tee_trusted);
+	if (ret < 0)
+		goto out_sess;
+
+	pvt_data.dev = dev;
+
+	return 0;
+
+out_sess:
+	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+out_ctx:
+	tee_client_close_context(pvt_data.ctx);
+
+	return err;
+}
+
+static int trusted_key_remove(struct device *dev)
+{
+	unregister_key_type(&key_type_tee_trusted);
+	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+	tee_client_close_context(pvt_data.ctx);
+
+	return 0;
+}
+
+static const struct tee_client_device_id trusted_key_id_table[] = {
+	{UUID_INIT(0xf04a0fe7, 0x1f5d, 0x4b9b,
+		   0xab, 0xf7, 0x61, 0x9b, 0x85, 0xb4, 0xce, 0x8c)},
+	{}
+};
+
+MODULE_DEVICE_TABLE(tee, trusted_key_id_table);
+
+static struct tee_client_driver trusted_key_driver = {
+	.id_table	= trusted_key_id_table,
+	.driver		= {
+		.name		= DRIVER_NAME,
+		.bus		= &tee_bus_type,
+		.probe		= trusted_key_probe,
+		.remove		= trusted_key_remove,
+	},
+};
+
+static int __init init_tee_trusted(void)
+{
+	struct tpm_chip *chip;
+
+	/*
+	 * Check for TPM availability as that is default source for trusted
+	 * keys. If not present, then register driver for TEE based device
+	 * providing support for trusted keys.
+	 */
+	chip = tpm_default_chip();
+	if (chip)
+		return 0;
+
+	return driver_register(&trusted_key_driver.driver);
+}
+
+static void __exit cleanup_tee_trusted(void)
+{
+	driver_unregister(&trusted_key_driver.driver);
+}
+
+late_initcall(init_tee_trusted);
+module_exit(cleanup_tee_trusted);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Sumit Garg <sumit.garg@linaro.org>");
+MODULE_DESCRIPTION("TEE based trusted keys");
-- 
2.7.4


^ permalink raw reply related

* [RFC 3/7] tee: add private login method for kernel clients
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
  To: keyrings, linux-integrity, linux-security-module
  Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
	jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
	linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>

There are use-cases where user-space shouldn't be allowed to communicate
directly with a TEE device which is dedicated to provide a specific
service for a kernel client. So add a private login method for kernel
clients and disallow user-space to open-session using this login method.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tee/tee_core.c   | 6 ++++++
 include/uapi/linux/tee.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 0f16d9f..4581bd1 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -334,6 +334,12 @@ static int tee_ioctl_open_session(struct tee_context *ctx,
 			goto out;
 	}
 
+	if (arg.clnt_login == TEE_IOCTL_LOGIN_REE_KERNEL) {
+		pr_err("login method not allowed for user-space client\n");
+		rc = -EPERM;
+		goto out;
+	}
+
 	rc = ctx->teedev->desc->ops->open_session(ctx, &arg, params);
 	if (rc)
 		goto out;
diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
index 4b9eb06..f33c69c 100644
--- a/include/uapi/linux/tee.h
+++ b/include/uapi/linux/tee.h
@@ -172,6 +172,8 @@ struct tee_ioctl_buf_data {
 #define TEE_IOCTL_LOGIN_APPLICATION		4
 #define TEE_IOCTL_LOGIN_USER_APPLICATION	5
 #define TEE_IOCTL_LOGIN_GROUP_APPLICATION	6
+/* Private login method for REE kernel clients */
+#define TEE_IOCTL_LOGIN_REE_KERNEL		0x80000000
 
 /**
  * struct tee_ioctl_param - parameter
-- 
2.7.4


^ permalink raw reply related

* [RFC 2/7] tee: enable support to register kernel memory
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
  To: keyrings, linux-integrity, linux-security-module
  Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
	jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
	linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>

Enable support to register kernel memory reference with TEE. This change
will allow TEE bus drivers to register memory references.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tee/tee_shm.c   | 16 ++++++++++++++--
 include/linux/tee_drv.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 2da026f..5c69b89 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -9,6 +9,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/tee_drv.h>
+#include <linux/uio.h>
 #include "tee_private.h"
 
 static void tee_shm_release(struct tee_shm *shm)
@@ -224,13 +225,14 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
 {
 	struct tee_device *teedev = ctx->teedev;
 	const u32 req_flags = TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED;
+	const u32 req_ker_flags = TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED;
 	struct tee_shm *shm;
 	void *ret;
 	int rc;
 	int num_pages;
 	unsigned long start;
 
-	if (flags != req_flags)
+	if (flags != req_flags && flags != req_ker_flags)
 		return ERR_PTR(-ENOTSUPP);
 
 	if (!tee_device_get(teedev))
@@ -264,7 +266,17 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
 		goto err;
 	}
 
-	rc = get_user_pages_fast(start, num_pages, FOLL_WRITE, shm->pages);
+	if (flags & TEE_SHM_USER_MAPPED) {
+		rc = get_user_pages_fast(start, num_pages, FOLL_WRITE,
+					 shm->pages);
+	} else {
+		const struct kvec kiov = {
+			.iov_base = (void *)start,
+			.iov_len = PAGE_SIZE
+		};
+
+		rc = get_kernel_pages(&kiov, num_pages, 0, shm->pages);
+	}
 	if (rc > 0)
 		shm->num_pages = rc;
 	if (rc != num_pages) {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 7a03f68..dedf8fa 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -26,6 +26,7 @@
 #define TEE_SHM_REGISTER	BIT(3)  /* Memory registered in secure world */
 #define TEE_SHM_USER_MAPPED	BIT(4)  /* Memory mapped in user space */
 #define TEE_SHM_POOL		BIT(5)  /* Memory allocated from pool */
+#define TEE_SHM_KERNEL_MAPPED	BIT(6)  /* Memory mapped in kernel space */
 
 struct device;
 struct tee_device;
-- 
2.7.4


^ permalink raw reply related

* [RFC 1/7] tee: optee: allow kernel pages to register as shm
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
  To: keyrings, linux-integrity, linux-security-module
  Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
	jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
	linux-kernel, tee-dev, Sumit Garg
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>

Kernel pages are marked as normal type memory only so allow kernel pages
to be registered as shared memory with OP-TEE.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tee/optee/call.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index aa94270..bce45b1 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -553,6 +553,13 @@ static int check_mem_type(unsigned long start, size_t num_pages)
 	struct mm_struct *mm = current->mm;
 	int rc;
 
+	/*
+	 * Allow kernel address to register with OP-TEE as kernel
+	 * pages are configured as normal memory only.
+	 */
+	if (virt_addr_valid(start))
+		return 0;
+
 	down_read(&mm->mmap_sem);
 	rc = __check_mem_type(find_vma(mm, start),
 			      start + num_pages * PAGE_SIZE);
-- 
2.7.4


^ permalink raw reply related

* [RFC 0/7] Introduce TEE based Trusted Keys support
From: Sumit Garg @ 2019-06-13 10:30 UTC (permalink / raw)
  To: keyrings, linux-integrity, linux-security-module
  Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
	jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
	linux-kernel, tee-dev, Sumit Garg

Add support for TEE based trusted keys where TEE provides the functionality
to seal and unseal trusted keys using hardware unique key. Also, this is
an alternative in case platform doesn't possess a TPM device.

This series also adds some TEE features like:

Patch #1, #2 enables support for registered kernel shared memory with TEE.

Patch #3 enables support for private kernel login method required for
cases like trusted keys where we don't wan't user-space to directly access
TEE service to retrieve trusted key contents.

Rest of the patches from #4 to #7 adds support for TEE based trusted keys.

This patch-set has been tested with OP-TEE based pseudo TA which can be
found here [1].

Looking forward to your valuable feedback/suggestions.

[1] https://github.com/OP-TEE/optee_os/pull/3082

Sumit Garg (7):
  tee: optee: allow kernel pages to register as shm
  tee: enable support to register kernel memory
  tee: add private login method for kernel clients
  KEYS: trusted: Introduce TEE based Trusted Keys
  KEYS: encrypted: Allow TEE based trusted master keys
  doc: keys: Document usage of TEE based Trusted Keys
  MAINTAINERS: Add entry for TEE based Trusted Keys

 Documentation/security/keys/tee-trusted.rst      |  93 +++++
 MAINTAINERS                                      |   9 +
 drivers/tee/optee/call.c                         |   7 +
 drivers/tee/tee_core.c                           |   6 +
 drivers/tee/tee_shm.c                            |  16 +-
 include/keys/tee_trusted.h                       |  84 ++++
 include/keys/trusted-type.h                      |   1 +
 include/linux/tee_drv.h                          |   1 +
 include/uapi/linux/tee.h                         |   2 +
 security/keys/Kconfig                            |   3 +
 security/keys/Makefile                           |   3 +
 security/keys/encrypted-keys/masterkey_trusted.c |  10 +-
 security/keys/tee_trusted.c                      | 506 +++++++++++++++++++++++
 13 files changed, 737 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/security/keys/tee-trusted.rst
 create mode 100644 include/keys/tee_trusted.h
 create mode 100644 security/keys/tee_trusted.c

-- 
2.7.4


^ permalink raw reply

* Re: [PATCH -next] ima: Make arch_policy_entry static
From: Mimi Zohar @ 2019-06-13 15:38 UTC (permalink / raw)
  To: YueHaibing, dmitry.kasatkin, jmorris, serge
  Cc: linux-kernel, linux-security-module, linux-integrity
In-Reply-To: <20190611134032.14656-1-yuehaibing@huawei.com>

On Tue, 2019-06-11 at 21:40 +0800, YueHaibing wrote:
> Fix sparse warning:
> 
> security/integrity/ima/ima_policy.c:202:23: warning:
>  symbol 'arch_policy_entry' was not declared. Should it be static?
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>

Thanks, this patch has been queued to be upstreamed.

Mimi

> ---
>  security/integrity/ima/ima_policy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 1cc822a..cd1b728 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -199,7 +199,7 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
>  };
>  
>  /* An array of architecture specific rules */
> -struct ima_rule_entry *arch_policy_entry __ro_after_init;
> +static struct ima_rule_entry *arch_policy_entry __ro_after_init;
>  
>  static LIST_HEAD(ima_default_rules);
>  static LIST_HEAD(ima_policy_rules);


^ permalink raw reply

* Re: [PATCH v3 0/2] ima/evm fixes for v5.2
From: Roberto Sassu @ 2019-06-13  8:51 UTC (permalink / raw)
  To: Janne Karhunen
  Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity,
	linux-security-module, silviu.vlasceanu
In-Reply-To: <CAE=NcraD_DcSqog8XbisA+0YdNqwj0v_jZhzjR2Na0eZ-2XgJQ@mail.gmail.com>

On 6/13/2019 10:04 AM, Janne Karhunen wrote:
> On Thu, Jun 13, 2019 at 10:50 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> 
>>> Would the appraise actually need any changes, just keep the
>>> IMA_NEW_FILE in ima_check_last_writer()? Of course it's not that easy
>>> (it never is) as the iint could go away and things like that, but with
>>> some tweaks?
>>
>> I think the problem would be that the code that sets the status to
>> INTEGRITY_PASS is not executed, because the file gets security.ima after
>> the first write.
> 
> We have a patchset coming shortly that starts tracking the inode
> changes as we go, so first time we fix it is when the file is created
> before it has any content (!);
> 
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 5fb7127bbe68..da4f0afe0348 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -236,8 +236,10 @@ int ima_appraise_measurement(enum ima_hooks func,
>                          iint->flags |= IMA_NEW_FILE;
>                  if ((iint->flags & IMA_NEW_FILE) &&
>                      (!(iint->flags & IMA_DIGSIG_REQUIRED) ||
> -                    (inode->i_size == 0)))
> +                    (inode->i_size == 0))) {
> +                       ima_fix_xattr(dentry, iint);
>                          status = INTEGRITY_PASS;

Some time ago I developed this patch:

http://kernsec.org/pipermail/linux-security-module-archive/2017-November/004569.html

Since the appraisal flags are not cleared, ima_appraise_measurement() is
not executed again and the problem with EVM does not arise.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply

* Re: [PATCH V8 3/3] Call ima_kexec_cmdline to measure the cmdline args
From: Dave Young @ 2019-06-13  8:26 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Prakhar Srivastava, linux-integrity, linux-security-module,
	linux-kernel, roberto.sassu, Eric W. Biederman, vgoyal, kexec
In-Reply-To: <1560378703.4578.91.camel@linux.ibm.com>

On 06/12/19 at 06:31pm, Mimi Zohar wrote:
> [Cc: kexec mailing list]
> 
> Hi Eric, Dave,
> 
> On Wed, 2019-06-12 at 15:15 -0700, Prakhar Srivastava wrote:
> > During soft reboot(kexec_file_load) boot cmdline args
> > are not measured.Thus the new kernel on load boots with
> > an assumption of cold reboot.
> > 
> > This patch makes a call to the ima hook ima_kexec_cmdline,
> > added in "Define a new IMA hook to measure the boot command
> > line arguments"
> > to measure the boot cmdline args into the ima log.
> > 
> > - call ima_kexec_cmdline from kexec_file_load.
> > - move the call ima_add_kexec_buffer after the cmdline
> > args have been measured.
> > 
> > Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Dave Young <dyoung@redhat.com>
> 
> Any chance we could get some Acks?

The ima_* is blackbox functions to me, looks like this patch is trying
to measure kexec cmdline buffer and save in some ima logs and then add all the
measure results including those for kernel/initrd to a kexec_buf and pass to 2nd
kernel.

It should be good and only take effect when IMA enabled. If all the
assumptions are right:

Acked-by: Dave Young <dyoung@redhat.com>
> 
> thanks,
> 
> Mimi
> 
> > ---
> >  kernel/kexec_file.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 072b6ee55e3f..b0c724e5d86c 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -198,9 +198,6 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
> >  		return ret;
> >  	image->kernel_buf_len = size;
> >  
> > -	/* IMA needs to pass the measurement list to the next kernel. */
> > -	ima_add_kexec_buffer(image);
> > -
> >  	/* Call arch image probe handlers */
> >  	ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
> >  					    image->kernel_buf_len);
> > @@ -241,8 +238,14 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
> >  			ret = -EINVAL;
> >  			goto out;
> >  		}
> > +
> > +		ima_kexec_cmdline(image->cmdline_buf,
> > +				  image->cmdline_buf_len - 1);
> >  	}
> >  
> > +	/* IMA needs to pass the measurement list to the next kernel. */
> > +	ima_add_kexec_buffer(image);
> > +
> >  	/* Call arch image load handlers */
> >  	ldata = arch_kexec_kernel_image_load(image);
> >  
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

^ permalink raw reply

* Re: [PATCH v3 0/2] ima/evm fixes for v5.2
From: Janne Karhunen @ 2019-06-13  8:04 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity,
	linux-security-module, silviu.vlasceanu
In-Reply-To: <3911846b-f836-592a-81e1-a2fd25470d98@huawei.com>

On Thu, Jun 13, 2019 at 10:50 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:

> > Would the appraise actually need any changes, just keep the
> > IMA_NEW_FILE in ima_check_last_writer()? Of course it's not that easy
> > (it never is) as the iint could go away and things like that, but with
> > some tweaks?
>
> I think the problem would be that the code that sets the status to
> INTEGRITY_PASS is not executed, because the file gets security.ima after
> the first write.

We have a patchset coming shortly that starts tracking the inode
changes as we go, so first time we fix it is when the file is created
before it has any content (!);

diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..da4f0afe0348 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -236,8 +236,10 @@ int ima_appraise_measurement(enum ima_hooks func,
                        iint->flags |= IMA_NEW_FILE;
                if ((iint->flags & IMA_NEW_FILE) &&
                    (!(iint->flags & IMA_DIGSIG_REQUIRED) ||
-                    (inode->i_size == 0)))
+                    (inode->i_size == 0))) {
+                       ima_fix_xattr(dentry, iint);
                        status = INTEGRITY_PASS;
+               }
                goto out;
        }



--
Janne

^ permalink raw reply related

* Re: [PATCH v3 0/2] ima/evm fixes for v5.2
From: Roberto Sassu @ 2019-06-13  7:50 UTC (permalink / raw)
  To: Janne Karhunen
  Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity,
	linux-security-module, silviu.vlasceanu
In-Reply-To: <CAE=NcrZgQSENPOtRdU=u1y6kqy0ouaaj-gioKHaUxZUcbUHwqA@mail.gmail.com>

On 6/13/2019 9:39 AM, Janne Karhunen wrote:
> On Thu, Jun 13, 2019 at 9:57 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> 
>>> Ok, I see the use case. Now, if you pull a urandom key that early on
>>> during the boot, the state of the system entropy is at all time low,
>>> and you are not really protecting against any sort of offline attack
>>> since the file is created during that boot cycle. Is there really use
>>> for using such key? Wouldn't it be possible to create a new config
>>> option, say IMA_ALLOW_EARLY_WRITERS, that would hold the NEW_FILE flag
>>> until the persistent key becomes available? In other words, it would
>>> start the measuring at the point when the key becomes online?
>>
>> I also thought about similar solutions. Another is for example to keep
>> the appraisal flags at file close, if security.ima is successfully
>> added to the file.
>>
>> Initializing EVM with a key is not a trivial change, but it seemed
>> better to me as it does not introduce exceptions in the IMA behavior.
> 
> Would the appraise actually need any changes, just keep the
> IMA_NEW_FILE in ima_check_last_writer()? Of course it's not that easy
> (it never is) as the iint could go away and things like that, but with
> some tweaks?

I think the problem would be that the code that sets the status to
INTEGRITY_PASS is not executed, because the file gets security.ima after
the first write.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply

* Re: [PATCH v3 0/2] ima/evm fixes for v5.2
From: Janne Karhunen @ 2019-06-13  7:39 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity,
	linux-security-module, silviu.vlasceanu
In-Reply-To: <144bf319-ea0c-f6b6-5737-0aac34f37186@huawei.com>

On Thu, Jun 13, 2019 at 9:57 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:

> > Ok, I see the use case. Now, if you pull a urandom key that early on
> > during the boot, the state of the system entropy is at all time low,
> > and you are not really protecting against any sort of offline attack
> > since the file is created during that boot cycle. Is there really use
> > for using such key? Wouldn't it be possible to create a new config
> > option, say IMA_ALLOW_EARLY_WRITERS, that would hold the NEW_FILE flag
> > until the persistent key becomes available? In other words, it would
> > start the measuring at the point when the key becomes online?
>
> I also thought about similar solutions. Another is for example to keep
> the appraisal flags at file close, if security.ima is successfully
> added to the file.
>
> Initializing EVM with a key is not a trivial change, but it seemed
> better to me as it does not introduce exceptions in the IMA behavior.

Would the appraise actually need any changes, just keep the
IMA_NEW_FILE in ima_check_last_writer()? Of course it's not that easy
(it never is) as the iint could go away and things like that, but with
some tweaks?


--
Janne

^ permalink raw reply

* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Dr. Greg @ 2019-06-13  7:25 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Stephen Smalley, Cedric Xing, linux-security-module, selinux,
	linux-kernel, linux-sgx, jarkko.sakkinen, luto, jmorris, serge,
	paul, eparis, jethro, dave.hansen, tglx, torvalds, akpm, nhorman,
	pmccallum, serge.ayoun, shay.katz-zamir, haitao.huang,
	andriy.shevchenko, kai.svahn, bp, josh, kai.huang, rientjes,
	william.c.roberts, philip.b.tricca
In-Reply-To: <20190612142557.GB20308@linux.intel.com>

On Wed, Jun 12, 2019 at 07:25:57AM -0700, Sean Christopherson wrote:

Good morning, we hope the week continues to go well for everyone.

> On Wed, Jun 12, 2019 at 04:32:21AM -0500, Dr. Greg wrote:
> > With SGX2 we will, by necessity, have to admit the notion that a
> > platform owner will not have any effective visibility into code that
> > is loaded and executed, since it can come in over a secured network
> > connection in an enclave security context.  This advocates for the
> > simplest approach possible to providing some type of regulation to any
> > form of WX page access.

> I believe we're all on the same page in the sense that we all want
> the "simplest approach possible", but there's a sliding scale of
> complexity between the kernel and userspace.  We can make life
> simple for userspace at the cost of additional complexity in the
> kernel, and vice versa.  The disagreement is over where to shove the
> extra complexity.

Yes, we are certainly cognizant of and sympathetic to the engineering
tensions involved.

The purpose of our e-mail was to leaven the discussion with the notion
that the most important question is how much complexity should be
shoved in either direction.  With respect to SGX as a technology, the
most important engineering metric is how much effective security is
actually being achieved.

Given an admission that enclave dynamic memory management (EDMM/SGX2)
is the goal in all of this, there are only two effective security
questions to be answered:

1.) Should a corpus of known memory with executable permissions be
copied into to an enclave.

2.) Should a corpus of executable memory with unknown content be
available to an enclave.

Given the functionality that SGX implements, both questions ultimately
devolve to whether or not a platform owner trusts an enclave author.
Security relevant trust is conveyed through cryptographically mediated
mechanisms.

The decision has been made to take full hardware mediated
cryptographic trust off the table for the mainstream Linux
implementation.  Given that, the most pragmatic engineering solution
would seem to be to implement the least complex implementation that
allows a platform owner to answer the two questions above.

See below.

> > Current state of the art, and there doesn't appear to be a reason to
> > change this, is to package an enclave in the form of an ELF shared
> > library.  It seems straight forward to inherit and act on page
> > privileges from the privileges specified on the ELF sections that are
> > loaded.  Loaders will have a file descriptor available so an mmap of
> > the incoming page with the specified privileges should trigger the
> > required LSM interventions and tie them to a specific enclave.
> > 
> > The current enclave 'standard' also uses layout metadata, stored in a
> > special .notes section of the shared image, to direct a loader with
> > respect to construction of the enclave stack, heap, TCS and other
> > miscellaneous regions not directly coded by the ELF TEXT sections.  It
> > seems straight forward to extend this paradigm to declare region(s) of
> > an enclave that are eligible to be generated at runtime (EAUG'ed) with
> > the RWX protections needed to support dynamically loaded code.
> > 
> > If an enclave wishes to support this functionality, it would seem
> > straight forward to require an enclave to provide a single zero page
> > which the loader will mmap with those protections in order to trigger
> > the desired LSM checks against that specific enclave.

> This is effectively #1, e.g. would require userspace to pre-declare its
> intent to make regions W->X.

Yes, we understood that when we wrote our original e-mail.

This model effectively allows the two relevant security questions to
be easily answered and is most consistent with current enclave
formats, software practices and runtimes.  It is also largely
consistent with existing LSM practices.

There hasn't been any discussion with respect to backports of this
driver but we believe it it safe to conclude that the industry is
going to be at least two years away from any type of realistic
deployments of this driver.  By that time there will be over a half a
decade of software deployment of existing API's and enclave formats.

Expecting a 'flag day' to be successful would seem to be contrary to
all known history of software practice and would thus disadvantage
Linux as an effective platform for this technology.

Best wishes for a productive remainder of the week to everyone.

Dr. Greg

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.           Specializing in information infra-structure
Fargo, ND  58102            development.
PH: 701-281-1686            EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"Sometimes I sing and dance around the house in my underwear,
 doesn't make me Madonna, never will.
                                -- Cyn
                                   Working Girl

^ permalink raw reply

* Re: [RFC 0/7] Introduce TEE based Trusted Keys support
From: Casey Schaufler @ 2019-06-13 16:40 UTC (permalink / raw)
  To: Sumit Garg, keyrings, linux-integrity, linux-security-module
  Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
	jmorris, serge, ard.biesheuvel, daniel.thompson, linux-doc,
	linux-kernel, tee-dev
In-Reply-To: <1560421833-27414-1-git-send-email-sumit.garg@linaro.org>

On 6/13/2019 3:30 AM, Sumit Garg wrote:
> Add support for TEE based trusted keys where TEE provides the functionality
> to seal and unseal trusted keys using hardware unique key. Also, this is
> an alternative in case platform doesn't possess a TPM device.
>
> This series also adds some TEE features like:

Please expand the acronym TEE on first use. That will
help people who don't work with it on a daily basis
understand what you're going on about.

>
> Patch #1, #2 enables support for registered kernel shared memory with TEE.
>
> Patch #3 enables support for private kernel login method required for
> cases like trusted keys where we don't wan't user-space to directly access
> TEE service to retrieve trusted key contents.
>
> Rest of the patches from #4 to #7 adds support for TEE based trusted keys.
>
> This patch-set has been tested with OP-TEE based pseudo TA which can be
> found here [1].
>
> Looking forward to your valuable feedback/suggestions.
>
> [1] https://github.com/OP-TEE/optee_os/pull/3082
>
> Sumit Garg (7):
>   tee: optee: allow kernel pages to register as shm
>   tee: enable support to register kernel memory
>   tee: add private login method for kernel clients
>   KEYS: trusted: Introduce TEE based Trusted Keys
>   KEYS: encrypted: Allow TEE based trusted master keys
>   doc: keys: Document usage of TEE based Trusted Keys
>   MAINTAINERS: Add entry for TEE based Trusted Keys
>
>  Documentation/security/keys/tee-trusted.rst      |  93 +++++
>  MAINTAINERS                                      |   9 +
>  drivers/tee/optee/call.c                         |   7 +
>  drivers/tee/tee_core.c                           |   6 +
>  drivers/tee/tee_shm.c                            |  16 +-
>  include/keys/tee_trusted.h                       |  84 ++++
>  include/keys/trusted-type.h                      |   1 +
>  include/linux/tee_drv.h                          |   1 +
>  include/uapi/linux/tee.h                         |   2 +
>  security/keys/Kconfig                            |   3 +
>  security/keys/Makefile                           |   3 +
>  security/keys/encrypted-keys/masterkey_trusted.c |  10 +-
>  security/keys/tee_trusted.c                      | 506 +++++++++++++++++++++++
>  13 files changed, 737 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/security/keys/tee-trusted.rst
>  create mode 100644 include/keys/tee_trusted.h
>  create mode 100644 security/keys/tee_trusted.c
>

^ permalink raw reply

* Re: [PATCH v3 0/2] ima/evm fixes for v5.2
From: Roberto Sassu @ 2019-06-13  6:57 UTC (permalink / raw)
  To: Janne Karhunen
  Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity,
	linux-security-module, silviu.vlasceanu
In-Reply-To: <CAE=NcrZiyWjZUuxdLgA9Bq89Cpt1W6MLAzPkLHVgfOqSo2i1hQ@mail.gmail.com>

On 6/13/2019 8:01 AM, Janne Karhunen wrote:
> On Wed, Jun 12, 2019 at 7:33 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> 
>>> That's a pretty big change for the userland IMHO. Quite a few
>>> configurations out there will break, including mine I believe, so I
>>> hope there is a solid reason asking people to change their stuff. I'm
>>> fine holding off all writing until it is safe to do so for now..
>>
>> The goal of appraisal is to allow access only to files with a valid
>> signature or HMAC. With the current behavior, that cannot be guaranteed.
>>
>> Unfortunately, dracut-state.sh is created very early. It could be
>> possible to unseal the key before, but this probably means modifying
>> systemd.
> 
> Ok, I see the use case. Now, if you pull a urandom key that early on
> during the boot, the state of the system entropy is at all time low,
> and you are not really protecting against any sort of offline attack
> since the file is created during that boot cycle. Is there really use
> for using such key? Wouldn't it be possible to create a new config
> option, say IMA_ALLOW_EARLY_WRITERS, that would hold the NEW_FILE flag
> until the persistent key becomes available? In other words, it would
> start the measuring at the point when the key becomes online?

I also thought about similar solutions. Another is for example to keep
the appraisal flags at file close, if security.ima is successfully
added to the file.

Initializing EVM with a key is not a trivial change, but it seemed
better to me as it does not introduce exceptions in the IMA behavior.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply

* Re: [PATCH v3 0/2] ima/evm fixes for v5.2
From: Janne Karhunen @ 2019-06-13  6:01 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: Mimi Zohar, dmitry.kasatkin, mjg59, linux-integrity,
	linux-security-module, silviu.vlasceanu
In-Reply-To: <c13c6b4f-1302-35fb-f077-00b7f84fea08@huawei.com>

On Wed, Jun 12, 2019 at 7:33 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:

> > That's a pretty big change for the userland IMHO. Quite a few
> > configurations out there will break, including mine I believe, so I
> > hope there is a solid reason asking people to change their stuff. I'm
> > fine holding off all writing until it is safe to do so for now..
>
> The goal of appraisal is to allow access only to files with a valid
> signature or HMAC. With the current behavior, that cannot be guaranteed.
>
> Unfortunately, dracut-state.sh is created very early. It could be
> possible to unseal the key before, but this probably means modifying
> systemd.

Ok, I see the use case. Now, if you pull a urandom key that early on
during the boot, the state of the system entropy is at all time low,
and you are not really protecting against any sort of offline attack
since the file is created during that boot cycle. Is there really use
for using such key? Wouldn't it be possible to create a new config
option, say IMA_ALLOW_EARLY_WRITERS, that would hold the NEW_FILE flag
until the persistent key becomes available? In other words, it would
start the measuring at the point when the key becomes online?


--
Janne

^ permalink raw reply

* RE: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address
From: Xing, Cedric @ 2019-06-13 16:47 UTC (permalink / raw)
  To: Jarkko Sakkinen, Andy Lutomirski
  Cc: Andy Lutomirski, Christopherson, Sean J, Stephen Smalley,
	James Morris, Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
	selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
	Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
	linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
	npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
	Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
	Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
	Tricca, Philip B
In-Reply-To: <20190613134800.GA12791@linux.intel.com>

> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Thursday, June 13, 2019 6:48 AM
> 
> On Thu, Jun 06, 2019 at 06:37:10PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jun 05, 2019 at 01:14:04PM -0700, Andy Lutomirski wrote:
> > >
> > >
> > > > On Jun 5, 2019, at 8:17 AM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > >> On Tue, Jun 04, 2019 at 10:10:22PM +0000, Xing, Cedric wrote:
> > > >> A bit off topic here. This mmap()/mprotect() discussion reminds
> > > >> me a question (guess for Jarkko): Now that
> > > >> vma->vm_file->private_data keeps a pointer to the enclave, why do
> we store it again in vma->vm_private?
> > > >> It isn't a big deal but non-NULL vm_private does prevent
> > > >> mprotect() from merging adjacent VMAs.
> > > >
> > > > Same semantics as with a regular mmap i.e. you can close the file
> > > > and still use the mapping.
> > > >
> > > >
> > >
> > > The file should be properly refcounted — vm_file should not go away
> while it’s mapped.
> 
> mm already takes care of that so vm_file does not go away. Still we need
> an internal refcount for enclaves to synchronize with the swapper. In
> summary nothing needs to be done.

I don't get this. The swapper takes a read lock on mm->mmap_sem, which locks the vma, which in turn reference counts vma->vm_file. Why is the internal refcount still needed? 

> 
> > Right, makes sense. It is easy one to change essentially just removing
> > internal refcount from sgx_encl and using file for the same. I'll
> > update this to my tree along with the changes to remove LKM/ACPI bits
> ASAP.
> 
> /Jarkko

^ permalink raw reply

* Re: [GIT PULL] SELinux fixes for v5.2 (#2)
From: pr-tracker-bot @ 2019-06-13  3:15 UTC (permalink / raw)
  To: Paul Moore; +Cc: Linus Torvalds, selinux, linux-security-module, linux-kernel
In-Reply-To: <CAHC9VhT8tYyUt_gtUR-jD-33LMW2RmzSXwP_OgPrh5ujQSiuUA@mail.gmail.com>

The pull request you sent on Wed, 12 Jun 2019 16:59:05 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20190612

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/b076173a309e2ceae84257d1d52cd3cc53b00e39

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

^ permalink raw reply

* [RFC PATCH v4 0/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-06-13  1:06 UTC (permalink / raw)
  To: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel
  Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka,
	jaskarankhurana

This patch set adds in-kernel pkcs7 signature checking for the roothash of
the dm-verity hash tree.
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.

Why we are doing validation in the Kernel?

The reason is to still be secure in cases where the attacker is able to
compromise the user mode application in which case the user mode validation
could not have been trusted.
The root hash signature validation in the kernel along with existing
dm-verity implementation gives a higher level of confidence in the
executable code or the protected data. Before allowing the creation of
the device mapper block device the kernel code will check that the detached
pkcs7 signature passed to it validates the roothash and the signature is
trusted by builtin keys set at kernel creation. The kernel should be
secured using Verified boot, UEFI Secure Boot or similar technologies so we
can trust it.

What about attacker mounting non dm-verity volumes to run executable
code?

This verification can be used to have a security architecture where a LSM
can enforce this verification for all the volumes and by doing this it can
ensure that all executable code runs from signed and trusted dm-verity
volumes.

Further patches will be posted that build on this and enforce this
verification based on policy for all the volumes on the system.

How are these changes tested?

To generate and sign the roothash just dump the roothash returned by veritysetup
format in a text file and then sign using the tool in the topic branch here:
(fsverity uses the tool for signing, I just added a parameter there for testing)

https://github.com/jaskarankhurana/fsverity-sign/tree/fs_verity_detached_pkcs7_for_dm_verity

fsverity sign-dm-verity <ROOTHASH_IN_A_FILE>  <OUTSIG> --key=<KEYFILE>
--cert=<CERTFILE>

veritysetup part of cryptsetup library was modified to take a optional
root-hash-sig parameter.

Commandline used to test the changes:

veritysetup open  <data_device> <name> <hash_device> <root_hash>
 --root-hash-sig=<root_hash_pkcs7_detached_sig>

The changes for veritysetup are in a topic branch for now at:
https://github.com/jaskarankhurana/veritysetup/tree/veritysetup_add_sig

Changelog:

v4:
  - Code review feedback given by Milan Broz.
  - Add documentation about the root hash signature parameter.
  - Bump up the dm-verity target version.
  - Provided way to sign and test with veritysetup in cover letter.

v3:
  - Code review feedback given by Sasha Levin.
  - Removed EXPORT_SYMBOL_GPL since this was not required.
  - Removed "This file is released under the GPLv2" since we have SPDX
    identifier.
  - Inside verity_verify_root_hash changed EINVAL to ENOKEY when the key
    descriptor is not specified but due to force option being set it is
    expected.
  - Moved CONFIG check to inside verity_verify_get_sig_from_key.
     (Did not move the sig_opts_cleanup to inside verity_dtr as the
     sig_opts do not need to be allocated for the entire duration the block
     device is active unlike the verity structure, note verity_dtr is called
     only if verity_ctr fails or after the lifetime of the block device.)

v2:
  - Code review feedback to pass the signature binary blob as a key that can be
    looked up in the kernel and be used to verify the roothash.
    [Suggested by Milan Broz]
  - Made the code related change suggested in review of v1.
    [Suggested by Balbir Singh]

v1:
  - Add kconfigs to control dm-verity root has signature verification and
    use the signature if specified to verify the root hash.

Jaskaran Khurana (1):
  Adds in-kernel pkcs7 sig checking the roothash of the dm-verity hash
    tree

 Documentation/device-mapper/verity.txt |   7 ++
 drivers/md/Kconfig                     |  23 +++++
 drivers/md/Makefile                    |   2 +-
 drivers/md/dm-verity-target.c          |  36 ++++++-
 drivers/md/dm-verity-verify-sig.c      | 132 +++++++++++++++++++++++++
 drivers/md/dm-verity-verify-sig.h      |  30 ++++++
 6 files changed, 224 insertions(+), 6 deletions(-)
 create mode 100644 drivers/md/dm-verity-verify-sig.c
 create mode 100644 drivers/md/dm-verity-verify-sig.h

-- 
2.17.1


^ permalink raw reply

* [RFC PATCH v4 1/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-06-13  1:06 UTC (permalink / raw)
  To: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel
  Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka,
	jaskarankhurana
In-Reply-To: <20190613010610.4364-1-jaskarankhurana@linux.microsoft.com>

The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.

The signature being provided for verification must verify the root hash and
must be trusted by the builtin keyring for verification to succeed.

The hash is added as a key of type "user" and the description is passed to
the kernel so it can look it up and use it for verification.

Adds DM_VERITY_VERIFY_ROOTHASH_SIG: roothash verification
against the roothash signature file *if* specified, if signature file is
specified verification must succeed prior to creation of device mapper
block device.

Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
specified for all dm verity volumes and verification must succeed prior
to creation of device mapper block device.

Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
---
 Documentation/device-mapper/verity.txt |   7 ++
 drivers/md/Kconfig                     |  23 +++++
 drivers/md/Makefile                    |   2 +-
 drivers/md/dm-verity-target.c          |  36 ++++++-
 drivers/md/dm-verity-verify-sig.c      | 132 +++++++++++++++++++++++++
 drivers/md/dm-verity-verify-sig.h      |  30 ++++++
 6 files changed, 224 insertions(+), 6 deletions(-)
 create mode 100644 drivers/md/dm-verity-verify-sig.c
 create mode 100644 drivers/md/dm-verity-verify-sig.h

diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
index b3d2e4a42255..df7ef1d553cc 100644
--- a/Documentation/device-mapper/verity.txt
+++ b/Documentation/device-mapper/verity.txt
@@ -121,6 +121,13 @@ check_at_most_once
     blocks, and a hash block will not be verified any more after all the data
     blocks it covers have been verified anyway.
 
+root_hash_sig_key_desc <key_description>
+    This is the description of the USER_KEY that the kernel will lookup to get
+    the pkcs7 signature of the roothash. The pkcs7 signature is used to validate
+    the root hash during the creation of the device mapper block device.
+    Verification of roothash depends on the config DM_VERITY_VERIFY_ROOTHASH_SIG
+    being set in the kernel.
+
 Theory of operation
 ===================
 
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index db269a348b20..da4115753f25 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -489,6 +489,29 @@ config DM_VERITY
 
 	  If unsure, say N.
 
+config DM_VERITY_VERIFY_ROOTHASH_SIG
+	def_bool n
+	bool "Verity data device root hash signature verification support"
+	depends on DM_VERITY
+	select SYSTEM_DATA_VERIFICATION
+	  help
+	  The device mapper target created by DM-VERITY can be validated if the
+	  pre-generated tree of cryptographic checksums passed has a pkcs#7
+	  signature file that can validate the roothash of the tree.
+
+	  If unsure, say N.
+
+config DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE
+	def_bool n
+	bool "Forces all dm verity data device root hash should be signed"
+	depends on DM_VERITY_VERIFY_ROOTHASH_SIG
+	  help
+	  The device mapper target created by DM-VERITY will succeed only if the
+	  pre-generated tree of cryptographic checksums passed also has a pkcs#7
+	  signature file that can validate the roothash of the tree.
+
+	  If unsure, say N.
+
 config DM_VERITY_FEC
 	bool "Verity forward error correction support"
 	depends on DM_VERITY
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index be7a6eb92abc..8a8c142bcfe1 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -61,7 +61,7 @@ obj-$(CONFIG_DM_LOG_USERSPACE)	+= dm-log-userspace.o
 obj-$(CONFIG_DM_ZERO)		+= dm-zero.o
 obj-$(CONFIG_DM_RAID)	+= dm-raid.o
 obj-$(CONFIG_DM_THIN_PROVISIONING)	+= dm-thin-pool.o
-obj-$(CONFIG_DM_VERITY)		+= dm-verity.o
+obj-$(CONFIG_DM_VERITY)		+= dm-verity.o dm-verity-verify-sig.o
 obj-$(CONFIG_DM_CACHE)		+= dm-cache.o
 obj-$(CONFIG_DM_CACHE_SMQ)	+= dm-cache-smq.o
 obj-$(CONFIG_DM_ERA)		+= dm-era.o
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index f4c31ffaa88e..adf7f376be7d 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -16,7 +16,7 @@
 
 #include "dm-verity.h"
 #include "dm-verity-fec.h"
-
+#include "dm-verity-verify-sig.h"
 #include <linux/module.h>
 #include <linux/reboot.h>
 
@@ -34,7 +34,8 @@
 #define DM_VERITY_OPT_IGN_ZEROES	"ignore_zero_blocks"
 #define DM_VERITY_OPT_AT_MOST_ONCE	"check_at_most_once"
 
-#define DM_VERITY_OPTS_MAX		(2 + DM_VERITY_OPTS_FEC)
+#define DM_VERITY_OPTS_MAX		(2 + DM_VERITY_OPTS_FEC + \
+					 DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
 
 static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
 
@@ -855,7 +856,8 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
 	return r;
 }
 
-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+				 struct dm_verity_sig_opts *verify_args)
 {
 	int r;
 	unsigned argc;
@@ -904,6 +906,14 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
 			if (r)
 				return r;
 			continue;
+		} else if (verity_verify_is_sig_opt_arg(arg_name)) {
+			r = verity_verify_sig_parse_opt_args(as, v,
+							     verify_args,
+							     &argc, arg_name);
+			if (r)
+				return r;
+			continue;
+
 		}
 
 		ti->error = "Unrecognized verity feature request";
@@ -930,6 +940,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
 static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 {
 	struct dm_verity *v;
+	struct dm_verity_sig_opts verify_args = {0};
 	struct dm_arg_set as;
 	unsigned int num;
 	unsigned long long num_ll;
@@ -937,6 +948,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	int i;
 	sector_t hash_position;
 	char dummy;
+	char *root_hash_digest_to_validate;
 
 	v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
 	if (!v) {
@@ -1070,6 +1082,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		r = -EINVAL;
 		goto bad;
 	}
+	root_hash_digest_to_validate = argv[8];
 
 	if (strcmp(argv[9], "-")) {
 		v->salt_size = strlen(argv[9]) / 2;
@@ -1095,11 +1108,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		as.argc = argc;
 		as.argv = argv;
 
-		r = verity_parse_opt_args(&as, v);
+		r = verity_parse_opt_args(&as, v, &verify_args);
 		if (r < 0)
 			goto bad;
 	}
 
+	/* Root hash signature is  a optional parameter*/
+	r = verity_verify_root_hash(root_hash_digest_to_validate,
+				    strlen(root_hash_digest_to_validate),
+				    verify_args.sig,
+				    verify_args.sig_size);
+	if (r < 0) {
+		ti->error = "Root hash verification failed";
+		goto bad;
+	}
 	v->hash_per_block_bits =
 		__fls((1 << v->hash_dev_block_bits) / v->digest_size);
 
@@ -1165,9 +1187,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	ti->per_io_data_size = roundup(ti->per_io_data_size,
 				       __alignof__(struct dm_verity_io));
 
+	verity_verify_sig_opts_cleanup(&verify_args);
+
 	return 0;
 
 bad:
+
+	verity_verify_sig_opts_cleanup(&verify_args);
 	verity_dtr(ti);
 
 	return r;
@@ -1175,7 +1201,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 static struct target_type verity_target = {
 	.name		= "verity",
-	.version	= {1, 4, 0},
+	.version	= {1, 5, 0},
 	.module		= THIS_MODULE,
 	.ctr		= verity_ctr,
 	.dtr		= verity_dtr,
diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
new file mode 100644
index 000000000000..1a889be76ede
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author:  Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#include <linux/device-mapper.h>
+#include <linux/verification.h>
+#include <keys/user-type.h>
+#include "dm-verity.h"
+#include "dm-verity-verify-sig.h"
+
+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
+
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+	return (!strcasecmp(arg_name,
+			    DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY));
+}
+
+static int verity_verify_get_sig_from_key(const char *key_desc,
+					struct dm_verity_sig_opts *sig_opts)
+{
+	struct key *key;
+	const struct user_key_payload *ukp;
+	int ret = 0;
+
+	if (!IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG))
+		return 0;
+
+	key = request_key(&key_type_user,
+			key_desc, NULL);
+	if (IS_ERR(key))
+		return PTR_ERR(key);
+
+	down_read(&key->sem);
+
+	ukp = user_key_payload_locked(key);
+	if (!ukp) {
+		ret = -EKEYREVOKED;
+		goto end;
+	}
+
+	sig_opts->sig = kmalloc(ukp->datalen, GFP_KERNEL);
+	if (!sig_opts->sig) {
+		ret = -ENOMEM;
+		goto end;
+	}
+	sig_opts->sig_size = ukp->datalen;
+
+	memcpy(sig_opts->sig, ukp->data, sig_opts->sig_size);
+
+end:
+	up_read(&key->sem);
+	key_put(key);
+
+	return ret;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
+				     struct dm_verity *v,
+				     struct dm_verity_sig_opts *sig_opts,
+				     unsigned int *argc,
+				     const char *arg_name)
+{
+	struct dm_target *ti = v->ti;
+	int ret = 0;
+	const char *sig_key = NULL;
+
+	if (!*argc) {
+		ti->error = DM_VERITY_VERIFY_ERR("Signature key not specified");
+		return -EINVAL;
+	}
+
+	sig_key = dm_shift_arg(as);
+	(*argc)--;
+
+	ret = verity_verify_get_sig_from_key(sig_key, sig_opts);
+	if (ret < 0)
+		ti->error = DM_VERITY_VERIFY_ERR("Invalid key specified");
+
+	return ret;
+}
+
+#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG
+/*
+ * verify_verify_roothash - Verify the root hash of the verity hash device
+ *			     using builtin trusted keys.
+ *
+ * @root_hash: For verity, the roothash/data to be verified.
+ * @root_hash_len: Size of the roothash/data to be verified.
+ * @sig_data: The trusted signature that verifies the roothash/data.
+ * @sig_len: Size of the signature.
+ *
+ */
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+			    const void *sig_data, size_t sig_len)
+{
+	int ret;
+
+	if (!root_hash || root_hash_len == 0)
+		return -EINVAL;
+
+	if (!sig_data  || sig_len == 0) {
+		if (IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE))
+			return -ENOKEY;
+		else
+			return 0;
+	}
+
+	ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
+				sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
+				NULL, NULL);
+
+	return ret;
+}
+#else
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+			    const void *sig_data, size_t sig_len)
+{
+	return 0;
+}
+#endif
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+	kfree(sig_opts->sig);
+	sig_opts->sig = NULL;
+	sig_opts->sig_size = 0;
+}
diff --git a/drivers/md/dm-verity-verify-sig.h b/drivers/md/dm-verity-verify-sig.h
new file mode 100644
index 000000000000..339818e6b527
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.h
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author:  Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#ifndef DM_VERITY_SIG_VERIFICATION_H
+#define DM_VERITY_SIG_VERIFICATION_H
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION "DM Verity Sig Verification"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY "root_hash_sig_key_desc"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 2
+
+struct dm_verity_sig_opts {
+	unsigned int sig_size;
+	u8 *sig;
+};
+int verity_verify_root_hash(const void *data, size_t data_len,
+			    const void *sig_data, size_t sig_len);
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name);
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+				    struct dm_verity_sig_opts *sig_opts,
+				    unsigned int *argc, const char *arg_name);
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts);
+
+#endif /* DM_VERITY_SIG_VERIFICATION_H */
-- 
2.17.1


^ permalink raw reply related

* RE: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Xing, Cedric @ 2019-06-13  1:02 UTC (permalink / raw)
  To: Christopherson, Sean J, Andy Lutomirski
  Cc: Stephen Smalley, LSM List, selinux@vger.kernel.org, LKML,
	linux-sgx@vger.kernel.org, Jarkko Sakkinen, James Morris,
	Serge E. Hallyn, Paul Moore, Eric Paris, Jethro Beekman,
	Hansen, Dave, Thomas Gleixner, Linus Torvalds, Andrew Morton,
	nhorman@redhat.com, Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao,
	Andy Shevchenko, Svahn, Kai, Borislav Petkov, Josh Triplett,
	Huang, Kai, David Rientjes, Roberts, William C, Tricca, Philip B
In-Reply-To: <20190612220242.GJ20308@linux.intel.com>

> From: Christopherson, Sean J
> Sent: Wednesday, June 12, 2019 3:03 PM
> 
> > I think this model works quite well in an SGX1 world.  The main thing
> > that makes me uneasy about this model is that, in SGX2, it requires
> > that an SGX2-compatible enclave loader must pre-declare to the kernel
> > whether it intends for its dynamically allocated memory to be
> > ALLOW_EXEC.  If ALLOW_EXEC is set but not actually needed, it will
> > still fail if DENY_X_IF_ALLOW_WRITE ends up being set.  The other
> > version below does not have this limitation.
> 
> I'm not convinced this will be a meaningful limitation in practice,
> though that's probably obvious from my RFCs :-).  That being said, the
> UAPI quirk is essentially a dealbreaker for multiple people, so let's
> drop #1.
> 
> I discussed the options with Cedric offline, and he is ok with option #2
> *if* the idea actually translates to acceptable code and doesn't present
> problems for userspace and/or future SGX features.
> 
> So, I'll work on an RFC series to implement #2 as described below.  If
> it works out, yay!  If not, i.e. option #2 is fundamentally broken, I'll
> shift my focus to Cedric's code (option #3).
> 
> > >   2. Pre-check LSM permissions and dynamically track mappings to
> enclave
> > >      pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
> > >      based on the pre-checked permissions.
> > >
> > >      Pros: Does not impact SGX UAPI, medium kernel complexity
> > >      Cons: Auditing is complex/weird, requires taking enclave-
> specific
> > >            lock during mprotect() to query/update tracking.
> >
> > Here's how this looks in my mind.  It's quite similar, except that
> > ALLOW_READ, ALLOW_WRITE, and ALLOW_EXEC are replaced with a little
> > state machine.
> >
> > EADD does not take any special flags.  It calls this LSM hook:
> >
> >   int security_enclave_load(struct vm_area_struct *source);
> >
> > This hook can return -EPERM.  Otherwise it 0 or
> > ALLOC_EXEC_IF_UNMODIFIED (i.e. 1).  This hook enforces permissions (a)
> and (b).
> >
> > The driver tracks a state for each page, and the possible states are:
> >
> >  - CLEAN_MAYEXEC /* no W or X VMAs have existed, but X is okay */
> >  - CLEAN_NOEXEC /* no W or X VMAs have existed, and X is not okay */
> >  - CLEAN_EXEC /* no W VMA has existed, but an X VMA has existed */
> >  - DIRTY /* a W VMA has existed */
> >
> > The initial state for a page is CLEAN_MAYEXEC if the hook said
> > ALLOW_EXEC_IF_UNMODIFIED and CLEAN_NOEXEC otherwise.
> >
> > The future EAUG does not call a hook at all and puts pages into the
> > state CLEAN_NOEXEC.  If SGX3 or later ever adds EAUG-but-don't-clear,
> > it can call security_enclave_load() and add CLEAN_MAYEXEC pages if
> appropriate.
> >
> > EINIT takes a sigstruct pointer.  SGX calls a new hook:
> >
> >   unsigned int security_enclave_init(struct sigstruct *sigstruct,
> > struct vm_area_struct *source, unsigned int flags);
> >
> > This hook can return -EPERM.  Otherwise it returns 0 or a combination
> > of flags DENY_WX and DENY_X_DIRTY.  The driver saves this value.
> > These represent permissions (c) and (d).
> >
> > If we want to have a permission for "execute code supplied from
> > outside the enclave that was not measured", we could have a flag like
> > HAS_UNMEASURED_CLEAN_EXEC_PAGE that the LSM could consider.
> >
> > mmap() and mprotect() enforce the following rules:
> >
> >  - If VM_EXEC is requested and (either the page is DIRTY or VM_WRITE
> is
> >    requested) and DENY_X_DIRTY, then deny.
> >
> >  - If VM_WRITE and VM_EXEC are both requested and DENY_WX, then deny.
> >
> >  - If VM_WRITE is requested, we need to update the state.  If it was
> >    CLEAN_EXEC, then we reject if DENY_X_DIRTY.  Otherwise we change
> the
> >    state to DIRTY.
> >
> >  - If VM_EXEC is requested and the page is CLEAN_NOEXEC, then deny.
> >
> > mprotect() and mmap() do *not* call SGX-specific LSM hooks to ask for
> > permission, although they can optionally call an LSM hook if they hit
> > one of the -EPERM cases for auditing purposes.
> >
> > Before the SIGSTRUCT is provided to the driver, the driver acts as
> > though DENY_X_DIRTY and DENY_WX are both set.

I think we've been discussing 2 topics simultaneously, one is the state machine that accepts/rejects mmap/mprotect requests, while the other is where is the best place to put it. I think we have an agreement on the former, and IMO option #2 and #3 differ only in the latter.

Option #2 keeps the state machine inside SGX subsystem, so it could reuse existing data structures for page tracking/locking to some extent. Sean may have smarter ideas, but it looks to me like the existing 'struct sgx_encl_page' tracks individual enclave pages while the FSM states apply to ranges. So in order *not* to test page by page in mmap/mprotect, I guess some new range oriented structures are still necessary. But I don't think it very important anyway. 

My major concern is more from the architecture/modularity perspective. Specifically, the state machine is defined by LSM but SGX does the state transitions. That's a brittle relationship that'd break easily if the state machine changes in future, or if different LSM modules want to define different FSMs (comprised of different set of states and/or triggers). After all, what's needed by the SGX subsystem is just the decision, not the FSM definition. I think we should take a closer look at this area once Sean's patch comes out.


^ permalink raw reply

* [RFC PATCH v4 1/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-06-13  0:43 UTC (permalink / raw)
  To: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel
  Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka
In-Reply-To: <20190613004328.4274-1-jaskarankhurana@linux.microsoft.com>

The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.

The signature being provided for verification must verify the root hash and
must be trusted by the builtin keyring for verification to succeed.

The hash is added as a key of type "user" and the description is passed to
the kernel so it can look it up and use it for verification.

Adds DM_VERITY_VERIFY_ROOTHASH_SIG: roothash verification
against the roothash signature file *if* specified, if signature file is
specified verification must succeed prior to creation of device mapper
block device.

Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
specified for all dm verity volumes and verification must succeed prior
to creation of device mapper block device.

Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
---
 Documentation/device-mapper/verity.txt |   7 ++
 drivers/md/Kconfig                     |  23 +++++
 drivers/md/Makefile                    |   2 +-
 drivers/md/dm-verity-target.c          |  36 ++++++-
 drivers/md/dm-verity-verify-sig.c      | 132 +++++++++++++++++++++++++
 drivers/md/dm-verity-verify-sig.h      |  30 ++++++
 6 files changed, 224 insertions(+), 6 deletions(-)
 create mode 100644 drivers/md/dm-verity-verify-sig.c
 create mode 100644 drivers/md/dm-verity-verify-sig.h

diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
index b3d2e4a42255..df7ef1d553cc 100644
--- a/Documentation/device-mapper/verity.txt
+++ b/Documentation/device-mapper/verity.txt
@@ -121,6 +121,13 @@ check_at_most_once
     blocks, and a hash block will not be verified any more after all the data
     blocks it covers have been verified anyway.
 
+root_hash_sig_key_desc <key_description>
+    This is the description of the USER_KEY that the kernel will lookup to get
+    the pkcs7 signature of the roothash. The pkcs7 signature is used to validate
+    the root hash during the creation of the device mapper block device.
+    Verification of roothash depends on the config DM_VERITY_VERIFY_ROOTHASH_SIG
+    being set in the kernel.
+
 Theory of operation
 ===================
 
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index db269a348b20..da4115753f25 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -489,6 +489,29 @@ config DM_VERITY
 
 	  If unsure, say N.
 
+config DM_VERITY_VERIFY_ROOTHASH_SIG
+	def_bool n
+	bool "Verity data device root hash signature verification support"
+	depends on DM_VERITY
+	select SYSTEM_DATA_VERIFICATION
+	  help
+	  The device mapper target created by DM-VERITY can be validated if the
+	  pre-generated tree of cryptographic checksums passed has a pkcs#7
+	  signature file that can validate the roothash of the tree.
+
+	  If unsure, say N.
+
+config DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE
+	def_bool n
+	bool "Forces all dm verity data device root hash should be signed"
+	depends on DM_VERITY_VERIFY_ROOTHASH_SIG
+	  help
+	  The device mapper target created by DM-VERITY will succeed only if the
+	  pre-generated tree of cryptographic checksums passed also has a pkcs#7
+	  signature file that can validate the roothash of the tree.
+
+	  If unsure, say N.
+
 config DM_VERITY_FEC
 	bool "Verity forward error correction support"
 	depends on DM_VERITY
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index be7a6eb92abc..8a8c142bcfe1 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -61,7 +61,7 @@ obj-$(CONFIG_DM_LOG_USERSPACE)	+= dm-log-userspace.o
 obj-$(CONFIG_DM_ZERO)		+= dm-zero.o
 obj-$(CONFIG_DM_RAID)	+= dm-raid.o
 obj-$(CONFIG_DM_THIN_PROVISIONING)	+= dm-thin-pool.o
-obj-$(CONFIG_DM_VERITY)		+= dm-verity.o
+obj-$(CONFIG_DM_VERITY)		+= dm-verity.o dm-verity-verify-sig.o
 obj-$(CONFIG_DM_CACHE)		+= dm-cache.o
 obj-$(CONFIG_DM_CACHE_SMQ)	+= dm-cache-smq.o
 obj-$(CONFIG_DM_ERA)		+= dm-era.o
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index f4c31ffaa88e..adf7f376be7d 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -16,7 +16,7 @@
 
 #include "dm-verity.h"
 #include "dm-verity-fec.h"
-
+#include "dm-verity-verify-sig.h"
 #include <linux/module.h>
 #include <linux/reboot.h>
 
@@ -34,7 +34,8 @@
 #define DM_VERITY_OPT_IGN_ZEROES	"ignore_zero_blocks"
 #define DM_VERITY_OPT_AT_MOST_ONCE	"check_at_most_once"
 
-#define DM_VERITY_OPTS_MAX		(2 + DM_VERITY_OPTS_FEC)
+#define DM_VERITY_OPTS_MAX		(2 + DM_VERITY_OPTS_FEC + \
+					 DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
 
 static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
 
@@ -855,7 +856,8 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
 	return r;
 }
 
-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+				 struct dm_verity_sig_opts *verify_args)
 {
 	int r;
 	unsigned argc;
@@ -904,6 +906,14 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
 			if (r)
 				return r;
 			continue;
+		} else if (verity_verify_is_sig_opt_arg(arg_name)) {
+			r = verity_verify_sig_parse_opt_args(as, v,
+							     verify_args,
+							     &argc, arg_name);
+			if (r)
+				return r;
+			continue;
+
 		}
 
 		ti->error = "Unrecognized verity feature request";
@@ -930,6 +940,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
 static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 {
 	struct dm_verity *v;
+	struct dm_verity_sig_opts verify_args = {0};
 	struct dm_arg_set as;
 	unsigned int num;
 	unsigned long long num_ll;
@@ -937,6 +948,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	int i;
 	sector_t hash_position;
 	char dummy;
+	char *root_hash_digest_to_validate;
 
 	v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
 	if (!v) {
@@ -1070,6 +1082,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		r = -EINVAL;
 		goto bad;
 	}
+	root_hash_digest_to_validate = argv[8];
 
 	if (strcmp(argv[9], "-")) {
 		v->salt_size = strlen(argv[9]) / 2;
@@ -1095,11 +1108,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		as.argc = argc;
 		as.argv = argv;
 
-		r = verity_parse_opt_args(&as, v);
+		r = verity_parse_opt_args(&as, v, &verify_args);
 		if (r < 0)
 			goto bad;
 	}
 
+	/* Root hash signature is  a optional parameter*/
+	r = verity_verify_root_hash(root_hash_digest_to_validate,
+				    strlen(root_hash_digest_to_validate),
+				    verify_args.sig,
+				    verify_args.sig_size);
+	if (r < 0) {
+		ti->error = "Root hash verification failed";
+		goto bad;
+	}
 	v->hash_per_block_bits =
 		__fls((1 << v->hash_dev_block_bits) / v->digest_size);
 
@@ -1165,9 +1187,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	ti->per_io_data_size = roundup(ti->per_io_data_size,
 				       __alignof__(struct dm_verity_io));
 
+	verity_verify_sig_opts_cleanup(&verify_args);
+
 	return 0;
 
 bad:
+
+	verity_verify_sig_opts_cleanup(&verify_args);
 	verity_dtr(ti);
 
 	return r;
@@ -1175,7 +1201,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 static struct target_type verity_target = {
 	.name		= "verity",
-	.version	= {1, 4, 0},
+	.version	= {1, 5, 0},
 	.module		= THIS_MODULE,
 	.ctr		= verity_ctr,
 	.dtr		= verity_dtr,
diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
new file mode 100644
index 000000000000..1a889be76ede
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author:  Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#include <linux/device-mapper.h>
+#include <linux/verification.h>
+#include <keys/user-type.h>
+#include "dm-verity.h"
+#include "dm-verity-verify-sig.h"
+
+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
+
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+	return (!strcasecmp(arg_name,
+			    DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY));
+}
+
+static int verity_verify_get_sig_from_key(const char *key_desc,
+					struct dm_verity_sig_opts *sig_opts)
+{
+	struct key *key;
+	const struct user_key_payload *ukp;
+	int ret = 0;
+
+	if (!IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG))
+		return 0;
+
+	key = request_key(&key_type_user,
+			key_desc, NULL);
+	if (IS_ERR(key))
+		return PTR_ERR(key);
+
+	down_read(&key->sem);
+
+	ukp = user_key_payload_locked(key);
+	if (!ukp) {
+		ret = -EKEYREVOKED;
+		goto end;
+	}
+
+	sig_opts->sig = kmalloc(ukp->datalen, GFP_KERNEL);
+	if (!sig_opts->sig) {
+		ret = -ENOMEM;
+		goto end;
+	}
+	sig_opts->sig_size = ukp->datalen;
+
+	memcpy(sig_opts->sig, ukp->data, sig_opts->sig_size);
+
+end:
+	up_read(&key->sem);
+	key_put(key);
+
+	return ret;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
+				     struct dm_verity *v,
+				     struct dm_verity_sig_opts *sig_opts,
+				     unsigned int *argc,
+				     const char *arg_name)
+{
+	struct dm_target *ti = v->ti;
+	int ret = 0;
+	const char *sig_key = NULL;
+
+	if (!*argc) {
+		ti->error = DM_VERITY_VERIFY_ERR("Signature key not specified");
+		return -EINVAL;
+	}
+
+	sig_key = dm_shift_arg(as);
+	(*argc)--;
+
+	ret = verity_verify_get_sig_from_key(sig_key, sig_opts);
+	if (ret < 0)
+		ti->error = DM_VERITY_VERIFY_ERR("Invalid key specified");
+
+	return ret;
+}
+
+#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG
+/*
+ * verify_verify_roothash - Verify the root hash of the verity hash device
+ *			     using builtin trusted keys.
+ *
+ * @root_hash: For verity, the roothash/data to be verified.
+ * @root_hash_len: Size of the roothash/data to be verified.
+ * @sig_data: The trusted signature that verifies the roothash/data.
+ * @sig_len: Size of the signature.
+ *
+ */
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+			    const void *sig_data, size_t sig_len)
+{
+	int ret;
+
+	if (!root_hash || root_hash_len == 0)
+		return -EINVAL;
+
+	if (!sig_data  || sig_len == 0) {
+		if (IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE))
+			return -ENOKEY;
+		else
+			return 0;
+	}
+
+	ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
+				sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
+				NULL, NULL);
+
+	return ret;
+}
+#else
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+			    const void *sig_data, size_t sig_len)
+{
+	return 0;
+}
+#endif
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+	kfree(sig_opts->sig);
+	sig_opts->sig = NULL;
+	sig_opts->sig_size = 0;
+}
diff --git a/drivers/md/dm-verity-verify-sig.h b/drivers/md/dm-verity-verify-sig.h
new file mode 100644
index 000000000000..339818e6b527
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.h
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author:  Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#ifndef DM_VERITY_SIG_VERIFICATION_H
+#define DM_VERITY_SIG_VERIFICATION_H
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION "DM Verity Sig Verification"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY "root_hash_sig_key_desc"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 2
+
+struct dm_verity_sig_opts {
+	unsigned int sig_size;
+	u8 *sig;
+};
+int verity_verify_root_hash(const void *data, size_t data_len,
+			    const void *sig_data, size_t sig_len);
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name);
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+				    struct dm_verity_sig_opts *sig_opts,
+				    unsigned int *argc, const char *arg_name);
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts);
+
+#endif /* DM_VERITY_SIG_VERIFICATION_H */
-- 
2.17.1


^ permalink raw reply related

* [RFC PATCH v4 0/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-06-13  0:43 UTC (permalink / raw)
  To: linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel
  Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka

This patch set adds in-kernel pkcs7 signature checking for the roothash of
the dm-verity hash tree.
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.

Why we are doing validation in the Kernel?

The reason is to still be secure in cases where the attacker is able to
compromise the user mode application in which case the user mode validation
could not have been trusted.
The root hash signature validation in the kernel along with existing
dm-verity implementation gives a higher level of confidence in the
executable code or the protected data. Before allowing the creation of
the device mapper block device the kernel code will check that the detached
pkcs7 signature passed to it validates the roothash and the signature is
trusted by builtin keys set at kernel creation. The kernel should be
secured using Verified boot, UEFI Secure Boot or similar technologies so we
can trust it.

What about attacker mounting non dm-verity volumes to run executable
code?

This verification can be used to have a security architecture where a LSM
can enforce this verification for all the volumes and by doing this it can
ensure that all executable code runs from signed and trusted dm-verity
volumes.

Further patches will be posted that build on this and enforce this
verification based on policy for all the volumes on the system.

How are these changes tested?

To generate and sign the roothash just dump the roothash returned by veritysetup
format in a text file and then sign using the tool in the topic branch here:
(fsverity uses the tool for signing, I just added a parameter there for testing)

https://github.com/jaskarankhurana/fsverity-sign/tree/fs_verity_detached_pkcs7_for_dm_verity

fsverity sign-dm-verity <ROOTHASH_IN_A_FILE>  <OUTSIG> --key=<KEYFILE>
--cert=<CERTFILE>

veritysetup part of cryptsetup library was modified to take a optional
root-hash-sig parameter.

Commandline used to test the changes:

veritysetup open  <data_device> <name> <hash_device> <root_hash>
 --root-hash-sig=<root_hash_pkcs7_detached_sig>

The changes for veritysetup are in a topic branch for now at:
https://github.com/jaskarankhurana/veritysetup/tree/veritysetup_add_sig

Changelog:

v4:
  - Code review feedback given by Milan Broz.
  - Add documentation about the root hash signature parameter.
  - Bump up the dm-verity target version.
  - Provided way to sign and test with veritysetup in cover letter.

v3:
  - Code review feedback given by Sasha Levin.
  - Removed EXPORT_SYMBOL_GPL since this was not required.
  - Removed "This file is released under the GPLv2" since we have SPDX
    identifier.
  - Inside verity_verify_root_hash changed EINVAL to ENOKEY when the key
    descriptor is not specified but due to force option being set it is
    expected.
  - Moved CONFIG check to inside verity_verify_get_sig_from_key.
     (Did not move the sig_opts_cleanup to inside verity_dtr as the
     sig_opts do not need to be allocated for the entire duration the block
     device is active unlike the verity structure, note verity_dtr is called
     only if verity_ctr fails or after the lifetime of the block device.)

v2:
  - Code review feedback to pass the signature binary blob as a key that can be
    looked up in the kernel and be used to verify the roothash.
    [Suggested by Milan Broz]
  - Made the code related change suggested in review of v1.
    [Suggested by Balbir Singh]

v1:
  - Add kconfigs to control dm-verity root has signature verification and
    use the signature if specified to verify the root hash.

Jaskaran Khurana (1):
  Adds in-kernel pkcs7 sig checking the roothash of the dm-verity hash
    tree

 Documentation/device-mapper/verity.txt |   7 ++
 drivers/md/Kconfig                     |  23 +++++
 drivers/md/Makefile                    |   2 +-
 drivers/md/dm-verity-target.c          |  36 ++++++-
 drivers/md/dm-verity-verify-sig.c      | 132 +++++++++++++++++++++++++
 drivers/md/dm-verity-verify-sig.h      |  30 ++++++
 6 files changed, 224 insertions(+), 6 deletions(-)
 create mode 100644 drivers/md/dm-verity-verify-sig.c
 create mode 100644 drivers/md/dm-verity-verify-sig.h

-- 
2.17.1


^ permalink raw reply

* RE: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Xing, Cedric @ 2019-06-13  0:10 UTC (permalink / raw)
  To: Christopherson, Sean J, Andy Lutomirski
  Cc: Stephen Smalley, LSM List, selinux@vger.kernel.org, LKML,
	linux-sgx@vger.kernel.org, Jarkko Sakkinen, James Morris,
	Serge E. Hallyn, Paul Moore, Eric Paris, Jethro Beekman,
	Hansen, Dave, Thomas Gleixner, Linus Torvalds, Andrew Morton,
	nhorman@redhat.com, pmccallum@redhat.com, Ayoun, Serge,
	Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
	Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes,
	Roberts, William C, Tricca, Philip B
In-Reply-To: <20190612220242.GJ20308@linux.intel.com>

> From: Christopherson, Sean J
> Sent: Wednesday, June 12, 2019 3:03 PM
> 
> > I think this model works quite well in an SGX1 world.  The main thing
> > that makes me uneasy about this model is that, in SGX2, it requires
> > that an SGX2-compatible enclave loader must pre-declare to the kernel
> > whether it intends for its dynamically allocated memory to be
> > ALLOW_EXEC.  If ALLOW_EXEC is set but not actually needed, it will
> > still fail if DENY_X_IF_ALLOW_WRITE ends up being set.  The other
> > version below does not have this limitation.
> 
> I'm not convinced this will be a meaningful limitation in practice,
> though that's probably obvious from my RFCs :-).  That being said, the
> UAPI quirk is essentially a dealbreaker for multiple people, so let's
> drop #1.
> 
> I discussed the options with Cedric offline, and he is ok with option #2
> *if* the idea actually translates to acceptable code and doesn't present
> problems for userspace and/or future SGX features.
> 
> So, I'll work on an RFC series to implement #2 as described below.  If
> it works out, yay!  If not, i.e. option #2 is fundamentally broken, I'll
> shift my focus to Cedric's code (option #3).
> 
> > >   2. Pre-check LSM permissions and dynamically track mappings to
> enclave
> > >      pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
> > >      based on the pre-checked permissions.
> > >
> > >      Pros: Does not impact SGX UAPI, medium kernel complexity
> > >      Cons: Auditing is complex/weird, requires taking enclave-
> specific
> > >            lock during mprotect() to query/update tracking.
> >
> > Here's how this looks in my mind.  It's quite similar, except that
> > ALLOW_READ, ALLOW_WRITE, and ALLOW_EXEC are replaced with a little
> > state machine.
> >
> > EADD does not take any special flags.  It calls this LSM hook:
> >
> >   int security_enclave_load(struct vm_area_struct *source);
> >
> > This hook can return -EPERM.  Otherwise it 0 or
> > ALLOC_EXEC_IF_UNMODIFIED (i.e. 1).  This hook enforces permissions (a)
> and (b).
> >
> > The driver tracks a state for each page, and the possible states are:
> >
> >  - CLEAN_MAYEXEC /* no W or X VMAs have existed, but X is okay */
> >  - CLEAN_NOEXEC /* no W or X VMAs have existed, and X is not okay */
> >  - CLEAN_EXEC /* no W VMA has existed, but an X VMA has existed */
> >  - DIRTY /* a W VMA has existed */
> >
> > The initial state for a page is CLEAN_MAYEXEC if the hook said
> > ALLOW_EXEC_IF_UNMODIFIED and CLEAN_NOEXEC otherwise.
> >
> > The future EAUG does not call a hook at all and puts pages into the
> > state CLEAN_NOEXEC.  If SGX3 or later ever adds EAUG-but-don't-clear,
> > it can call security_enclave_load() and add CLEAN_MAYEXEC pages if
> appropriate.
> >
> > EINIT takes a sigstruct pointer.  SGX calls a new hook:
> >
> >   unsigned int security_enclave_init(struct sigstruct *sigstruct,
> > struct vm_area_struct *source, unsigned int flags);
> >
> > This hook can return -EPERM.  Otherwise it returns 0 or a combination
> > of flags DENY_WX and DENY_X_DIRTY.  The driver saves this value.
> > These represent permissions (c) and (d).
> >
> > If we want to have a permission for "execute code supplied from
> > outside the enclave that was not measured", we could have a flag like
> > HAS_UNMEASURED_CLEAN_EXEC_PAGE that the LSM could consider.
> >
> > mmap() and mprotect() enforce the following rules:
> >
> >  - If VM_EXEC is requested and (either the page is DIRTY or VM_WRITE
> is
> >    requested) and DENY_X_DIRTY, then deny.
> >
> >  - If VM_WRITE and VM_EXEC are both requested and DENY_WX, then deny.
> >
> >  - If VM_WRITE is requested, we need to update the state.  If it was
> >    CLEAN_EXEC, then we reject if DENY_X_DIRTY.  Otherwise we change
> the
> >    state to DIRTY.
> >
> >  - If VM_EXEC is requested and the page is CLEAN_NOEXEC, then deny.
> >
> > mprotect() and mmap() do *not* call SGX-specific LSM hooks to ask for
> > permission, although they can optionally call an LSM hook if they hit
> > one of the -EPERM cases for auditing purposes.
> >
> > Before the SIGSTRUCT is provided to the driver, the driver acts as
> > though DENY_X_DIRTY and DENY_WX are both set.

I think we've been discussing 2 topics simultaneously, one is the state machine that accepts/rejects mmap/mprotect requests, while the other is where is the best place to put it. I think we have an agreement on the former, and IMO option #2 and #3 differ only in the latter.

Option #2 keeps the state machine inside SGX subsystem, so it could reuse existing data structures for page tracking/locking to some extent. Sean may have smarter ideas, but it looks to me like the existing 'struct sgx_encl_page' tracks individual enclave pages while the FSM states apply to ranges. So in order *not* to test page by page in mmap/mprotect, I guess some new range oriented structures are still necessary. But I don't think it very important anyway. 

My major concern is more from the architecture/modularity perspective. Specifically, the state machine is defined by LSM but SGX does the state transitions. That's a brittle relationship that'd break easily if the state machine changes in future, or if different LSM modules want to define different FSMs (comprised of different set of states and/or triggers). After all, what's needed by the SGX subsystem is just the decision, not the FSM definition. I think we should take a closer look at this area once Sean's patch comes out.


^ permalink raw reply

* Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Stephen Smalley @ 2019-06-13 17:02 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Cedric Xing, linux-security-module, selinux, linux-kernel,
	linux-sgx, jarkko.sakkinen, luto, jmorris, serge, paul, eparis,
	jethro, dave.hansen, tglx, torvalds, akpm, nhorman, pmccallum,
	serge.ayoun, shay.katz-zamir, haitao.huang, andriy.shevchenko,
	kai.svahn, bp, josh, kai.huang, rientjes, william.c.roberts,
	philip.b.tricca
In-Reply-To: <20190611220243.GB3416@linux.intel.com>

On 6/11/19 6:02 PM, Sean Christopherson wrote:
> On Tue, Jun 11, 2019 at 09:40:25AM -0400, Stephen Smalley wrote:
>> I haven't looked at this code closely, but it feels like a lot of
>> SGX-specific logic embedded into SELinux that will have to be repeated or
>> reused for every security module.  Does SGX not track this state itself?
> 
> SGX does track equivalent state.
> 
> There are three proposals on the table (I think):
> 
>    1. Require userspace to explicitly specificy (maximal) enclave page
>       permissions at build time.  The enclave page permissions are provided
>       to, and checked by, LSMs at enclave build time.
> 
>       Pros: Low-complexity kernel implementation, straightforward auditing
>       Cons: Sullies the SGX UAPI to some extent, may increase complexity of
>             SGX2 enclave loaders.
> 
>    2. Pre-check LSM permissions and dynamically track mappings to enclave
>       pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
>       based on the pre-checked permissions.
> 
>       Pros: Does not impact SGX UAPI, medium kernel complexity
>       Cons: Auditing is complex/weird, requires taking enclave-specific
>             lock during mprotect() to query/update tracking.
> 
>    3. Implement LSM hooks in SGX to allow LSMs to track enclave regions
>       from cradle to grave, but otherwise defer everything to LSMs.
> 
>       Pros: Does not impact SGX UAPI, maximum flexibility, precise auditing
>       Cons: Most complex and "heaviest" kernel implementation of the three,
>             pushes more SGX details into LSMs.
> 
> My RFC series[1] implements #1.  My understanding is that Andy (Lutomirski)
> prefers #2.  Cedric's RFC series implements #3.
> 
> Perhaps the easiest way to make forward progress is to rule out the
> options we absolutely *don't* want by focusing on the potentially blocking
> issue with each option:
> 
>    #1 - SGX UAPI funkiness
> 
>    #2 - Auditing complexity, potential enclave lock contention
> 
>    #3 - Pushing SGX details into LSMs and complexity of kernel implementation
> 
> 
> [1] https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com

Given the complexity tradeoff, what is the clear motivating example for 
why #1 isn't the obvious choice? That the enclave loader has no way of 
knowing a priori whether the enclave will require W->X or WX?  But 
aren't we better off requiring enclaves to be explicitly marked as 
needing such so that we can make a more informed decision about whether 
to load them in the first place?

^ permalink raw reply

page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox