[op-tee] [PATCH 0/3] tee: add support for session's client UUID generation

[op-tee] [PATCH 0/3] tee: add support for session's client UUID generation

[Vesa Jääskeläinen]

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

TEE Client API defines that from user space only information needed for
specified login operations is group identifier for group based logins.

REE kernel is expected to formulate trustworthy client UUID and pass that
to TEE environment. REE kernel is required to verify that provided group
identifier for group based logins matches calling processes group
memberships.

TEE specification only defines that the information passed from REE
environment to TEE environment is encoded into on UUID.

In order to guarantee trustworthiness of client UUID user space is not
allowed to freely pass client UUID.

Vesa Jääskeläinen (3):
  tee: add support for session's client UUID generation
  tee: optee: Add support for session login client UUID generation
  [RFC] tee: add support for app id for client UUID generation

 drivers/tee/Kconfig      |   1 +
 drivers/tee/optee/call.c |   6 +-
 drivers/tee/tee_core.c   | 188 +++++++++++++++++++++++++++++++++++++++
 include/linux/tee_drv.h  |  16 ++++
 4 files changed, 210 insertions(+), 1 deletion(-)

-- 
2.17.1

Notes:

This patcheset has been designed so that it can be iteratively intergrated
meaning that the application ID (RFC patch) part can be left for later when
there is agreed solution for that.

TEE specification leaves Linux behavior undefined. It does not define any
UUID value for name space. UUID in here is randomly generated with uuidgen
tool.

I have also include amdtee people as this method probably should also be
applied in there.

Using op-tee(a)lists.trustedfirmware.org instead of tee-dev(a)lists.linaro.org as
latter is deprecated old list.

Original issue in OP-TEE OS tracker:
https://github.com/OP-TEE/optee_os/issues/3642

Related reviews and demonstration for the concept:
https://github.com/linaro-swg/linux/pull/74
https://github.com/OP-TEE/optee_client/pull/195
https://github.com/OP-TEE/optee_test/pull/406

[PATCH 0/3] tee: add support for session's client UUID generation

[Vesa Jääskeläinen]

TEE Client API defines that from user space only information needed for
specified login operations is group identifier for group based logins.

REE kernel is expected to formulate trustworthy client UUID and pass that
to TEE environment. REE kernel is required to verify that provided group
identifier for group based logins matches calling processes group
memberships.

TEE specification only defines that the information passed from REE
environment to TEE environment is encoded into on UUID.

In order to guarantee trustworthiness of client UUID user space is not
allowed to freely pass client UUID.

Vesa Jääskeläinen (3):
  tee: add support for session's client UUID generation
  tee: optee: Add support for session login client UUID generation
  [RFC] tee: add support for app id for client UUID generation

 drivers/tee/Kconfig      |   1 +
 drivers/tee/optee/call.c |   6 +-
 drivers/tee/tee_core.c   | 188 +++++++++++++++++++++++++++++++++++++++
 include/linux/tee_drv.h  |  16 ++++
 4 files changed, 210 insertions(+), 1 deletion(-)

-- 
2.17.1

Notes:

This patcheset has been designed so that it can be iteratively intergrated
meaning that the application ID (RFC patch) part can be left for later when
there is agreed solution for that.

TEE specification leaves Linux behavior undefined. It does not define any
UUID value for name space. UUID in here is randomly generated with uuidgen
tool.

I have also include amdtee people as this method probably should also be
applied in there.

Using op-tee@lists.trustedfirmware.org instead of tee-dev@lists.linaro.org as
latter is deprecated old list.

Original issue in OP-TEE OS tracker:
https://github.com/OP-TEE/optee_os/issues/3642

Related reviews and demonstration for the concept:
https://github.com/linaro-swg/linux/pull/74
https://github.com/OP-TEE/optee_client/pull/195
https://github.com/OP-TEE/optee_test/pull/406

[op-tee] [PATCH 1/3] tee: add support for session's client UUID generation

[Vesa Jääskeläinen]

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

TEE Client API defines that from user space only information needed for
specified login operations is group identifier for group based logins.

REE kernel is expected to formulate trustworthy client UUID and pass that
to TEE environment. REE kernel is required to verify that provided group
identifier for group based logins matches calling processes group
memberships.

TEE specification only defines that the information passed from REE
environment to TEE environment is encoded into on UUID.

In order to guarantee trustworthiness of client UUID user space is not
allowed to freely pass client UUID.

UUIDv5 form is used encode variable amount of information needed for
different login types.

Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>
---
 drivers/tee/Kconfig     |   1 +
 drivers/tee/tee_core.c  | 143 ++++++++++++++++++++++++++++++++++++++++
 include/linux/tee_drv.h |  16 +++++
 3 files changed, 160 insertions(+)

diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
index 8da63f38e6bd..806eb87d4da0 100644
--- a/drivers/tee/Kconfig
+++ b/drivers/tee/Kconfig
@@ -3,6 +3,7 @@
 config TEE
 	tristate "Trusted Execution Environment support"
 	depends on HAVE_ARM_SMCCC || COMPILE_TEST || CPU_SUP_AMD
+	select CRYPTO_SHA1
 	select DMA_SHARED_BUFFER
 	select GENERIC_ALLOCATOR
 	help
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 6aec502c495c..872272bf9dec 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -6,18 +6,33 @@
 #define pr_fmt(fmt) "%s: " fmt, __func__
 
 #include <linux/cdev.h>
+#include <linux/cred.h>
 #include <linux/fs.h>
 #include <linux/idr.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/tee_drv.h>
 #include <linux/uaccess.h>
+#include <crypto/hash.h>
+#include <crypto/sha.h>
 #include "tee_private.h"
 
 #define TEE_NUM_DEVICES	32
 
 #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x))
 
+#define TEE_UUID_NS_NAME_SIZE	128
+
+/*
+ * TEE Client UUID name space identifier (UUIDv4)
+ *
+ * Value here is random UUID that is allocated as name space identifier for
+ * forming Client UUID's for TEE environment using UUIDv5 scheme.
+ */
+static const uuid_t tee_client_uuid_ns = UUID_INIT(0x58ac9ca0, 0x2086, 0x4683,
+						   0xa1, 0xb8, 0xec, 0x4b,
+						   0xc0, 0x8e, 0x01, 0xb6);
+
 /*
  * Unprivileged devices in the lower half range and privileged devices in
  * the upper half range.
@@ -110,6 +125,134 @@ static int tee_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+/**
+ * uuid_v5() - Calculate UUIDv5
+ * @uuid: Resulting UUID
+ * @ns: Name space ID for UUIDv5 function
+ * @name: Name for UUIDv5 function
+ * @size: Size of name
+ *
+ * UUIDv5 is specific in RFC 4122.
+ *
+ * This implements section (for SHA-1):
+ * 4.3.  Algorithm for Creating a Name-Based UUID
+ */
+static int uuid_v5(uuid_t *uuid, const uuid_t *ns, const void *name,
+		   size_t size)
+{
+	unsigned char hash[SHA1_DIGEST_SIZE];
+	struct crypto_shash *shash = NULL;
+	struct shash_desc *desc = NULL;
+	int rc;
+
+	shash = crypto_alloc_shash("sha1", 0, 0);
+	if (IS_ERR(shash)) {
+		rc = PTR_ERR(shash);
+		pr_err("shash(sha1) allocation failed\n");
+		return rc;
+	}
+
+	desc = kzalloc(sizeof(*desc) + crypto_shash_descsize(shash),
+		       GFP_KERNEL);
+	if (IS_ERR(desc)) {
+		rc = PTR_ERR(desc);
+		goto out;
+	}
+
+	desc->tfm = shash;
+
+	rc = crypto_shash_init(desc);
+	if (rc < 0)
+		goto out2;
+
+	rc = crypto_shash_update(desc, (const u8 *)ns, sizeof(*ns));
+	if (rc < 0)
+		goto out2;
+
+	rc = crypto_shash_update(desc, (const u8 *)name, size);
+	if (rc < 0)
+		goto out2;
+
+	rc = crypto_shash_final(desc, hash);
+	if (rc < 0)
+		goto out2;
+
+	memcpy(uuid->b, hash, UUID_SIZE);
+
+	/* Tag for version 5 */
+	uuid->b[6] = (hash[6] & 0x0F) | 0x50;
+	uuid->b[8] = (hash[8] & 0x3F) | 0x80;
+
+out2:
+	kfree(desc);
+
+out:
+	crypto_free_shash(shash);
+	return rc;
+}
+
+int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
+				 const u8 connection_data[TEE_IOCTL_UUID_LEN])
+{
+	const char *application_id = NULL;
+	gid_t ns_grp = (gid_t)-1;
+	kgid_t grp = INVALID_GID;
+	char *name = NULL;
+	int rc;
+
+	if (connection_method == TEE_IOCTL_LOGIN_PUBLIC) {
+		/* Nil UUID to be passed to TEE environment */
+		uuid_copy(uuid, &uuid_null);
+		return 0;
+	}
+
+	/*
+	 * In Linux environment client UUID is based on UUIDv5.
+	 *
+	 * Determine client UUID with following semantics for 'name':
+	 *
+	 * For TEEC_LOGIN_USER:
+	 * uid=<uid>
+	 *
+	 * For TEEC_LOGIN_GROUP:
+	 * gid=<gid>
+	 *
+	 */
+
+	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+
+	switch (connection_method) {
+	case TEE_IOCTL_LOGIN_USER:
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
+			  current_euid().val);
+		break;
+
+	case TEE_IOCTL_LOGIN_GROUP:
+		memcpy(&ns_grp, connection_data, sizeof(gid_t));
+		grp = make_kgid(current_user_ns(), ns_grp);
+		if (!gid_valid(grp) || !in_egroup_p(grp)) {
+			rc = -EPERM;
+			goto out;
+		}
+
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "gid=%x", grp.val);
+		break;
+
+	default:
+		rc = -EINVAL;
+		goto out;
+	}
+
+	rc = uuid_v5(uuid, &tee_client_uuid_ns, name, strlen(name));
+out:
+	kfree(name);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tee_session_calc_client_uuid);
+
 static int tee_ioctl_version(struct tee_context *ctx,
 			     struct tee_ioctl_version_data __user *uvers)
 {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 1412e9cc79ce..8471b790e858 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -165,6 +165,22 @@ int tee_device_register(struct tee_device *teedev);
  */
 void tee_device_unregister(struct tee_device *teedev);
 
+/**
+ * tee_session_calc_client_uuid() - Calculates client UUID for session
+ * @uuid:		Resulting UUID
+ * @connection_method:	Connection method for session (TEE_IOCTL_LOGIN_*)
+ * @connectuon_data:	Connection data for opening session
+ *
+ * Based on connection method calculates UUIDv5 based client UUID.
+ *
+ * For group based logins verifies that calling process has specified
+ * credentials.
+ *
+ * @return < 0 on failure
+ */
+int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
+				 const u8 connection_data[TEE_IOCTL_UUID_LEN]);
+
 /**
  * struct tee_shm - shared memory object
  * @ctx:	context using the object
-- 
2.17.1

[PATCH 1/3] tee: add support for session's client UUID generation

[Vesa Jääskeläinen]

TEE Client API defines that from user space only information needed for
specified login operations is group identifier for group based logins.

REE kernel is expected to formulate trustworthy client UUID and pass that
to TEE environment. REE kernel is required to verify that provided group
identifier for group based logins matches calling processes group
memberships.

TEE specification only defines that the information passed from REE
environment to TEE environment is encoded into on UUID.

In order to guarantee trustworthiness of client UUID user space is not
allowed to freely pass client UUID.

UUIDv5 form is used encode variable amount of information needed for
different login types.

Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>
---
 drivers/tee/Kconfig     |   1 +
 drivers/tee/tee_core.c  | 143 ++++++++++++++++++++++++++++++++++++++++
 include/linux/tee_drv.h |  16 +++++
 3 files changed, 160 insertions(+)

diff --git a/drivers/tee/Kconfig b/drivers/tee/Kconfig
index 8da63f38e6bd..806eb87d4da0 100644
--- a/drivers/tee/Kconfig
+++ b/drivers/tee/Kconfig
@@ -3,6 +3,7 @@
 config TEE
 	tristate "Trusted Execution Environment support"
 	depends on HAVE_ARM_SMCCC || COMPILE_TEST || CPU_SUP_AMD
+	select CRYPTO_SHA1
 	select DMA_SHARED_BUFFER
 	select GENERIC_ALLOCATOR
 	help
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 6aec502c495c..872272bf9dec 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -6,18 +6,33 @@
 #define pr_fmt(fmt) "%s: " fmt, __func__
 
 #include <linux/cdev.h>
+#include <linux/cred.h>
 #include <linux/fs.h>
 #include <linux/idr.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/tee_drv.h>
 #include <linux/uaccess.h>
+#include <crypto/hash.h>
+#include <crypto/sha.h>
 #include "tee_private.h"
 
 #define TEE_NUM_DEVICES	32
 
 #define TEE_IOCTL_PARAM_SIZE(x) (sizeof(struct tee_param) * (x))
 
+#define TEE_UUID_NS_NAME_SIZE	128
+
+/*
+ * TEE Client UUID name space identifier (UUIDv4)
+ *
+ * Value here is random UUID that is allocated as name space identifier for
+ * forming Client UUID's for TEE environment using UUIDv5 scheme.
+ */
+static const uuid_t tee_client_uuid_ns = UUID_INIT(0x58ac9ca0, 0x2086, 0x4683,
+						   0xa1, 0xb8, 0xec, 0x4b,
+						   0xc0, 0x8e, 0x01, 0xb6);
+
 /*
  * Unprivileged devices in the lower half range and privileged devices in
  * the upper half range.
@@ -110,6 +125,134 @@ static int tee_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+/**
+ * uuid_v5() - Calculate UUIDv5
+ * @uuid: Resulting UUID
+ * @ns: Name space ID for UUIDv5 function
+ * @name: Name for UUIDv5 function
+ * @size: Size of name
+ *
+ * UUIDv5 is specific in RFC 4122.
+ *
+ * This implements section (for SHA-1):
+ * 4.3.  Algorithm for Creating a Name-Based UUID
+ */
+static int uuid_v5(uuid_t *uuid, const uuid_t *ns, const void *name,
+		   size_t size)
+{
+	unsigned char hash[SHA1_DIGEST_SIZE];
+	struct crypto_shash *shash = NULL;
+	struct shash_desc *desc = NULL;
+	int rc;
+
+	shash = crypto_alloc_shash("sha1", 0, 0);
+	if (IS_ERR(shash)) {
+		rc = PTR_ERR(shash);
+		pr_err("shash(sha1) allocation failed\n");
+		return rc;
+	}
+
+	desc = kzalloc(sizeof(*desc) + crypto_shash_descsize(shash),
+		       GFP_KERNEL);
+	if (IS_ERR(desc)) {
+		rc = PTR_ERR(desc);
+		goto out;
+	}
+
+	desc->tfm = shash;
+
+	rc = crypto_shash_init(desc);
+	if (rc < 0)
+		goto out2;
+
+	rc = crypto_shash_update(desc, (const u8 *)ns, sizeof(*ns));
+	if (rc < 0)
+		goto out2;
+
+	rc = crypto_shash_update(desc, (const u8 *)name, size);
+	if (rc < 0)
+		goto out2;
+
+	rc = crypto_shash_final(desc, hash);
+	if (rc < 0)
+		goto out2;
+
+	memcpy(uuid->b, hash, UUID_SIZE);
+
+	/* Tag for version 5 */
+	uuid->b[6] = (hash[6] & 0x0F) | 0x50;
+	uuid->b[8] = (hash[8] & 0x3F) | 0x80;
+
+out2:
+	kfree(desc);
+
+out:
+	crypto_free_shash(shash);
+	return rc;
+}
+
+int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
+				 const u8 connection_data[TEE_IOCTL_UUID_LEN])
+{
+	const char *application_id = NULL;
+	gid_t ns_grp = (gid_t)-1;
+	kgid_t grp = INVALID_GID;
+	char *name = NULL;
+	int rc;
+
+	if (connection_method == TEE_IOCTL_LOGIN_PUBLIC) {
+		/* Nil UUID to be passed to TEE environment */
+		uuid_copy(uuid, &uuid_null);
+		return 0;
+	}
+
+	/*
+	 * In Linux environment client UUID is based on UUIDv5.
+	 *
+	 * Determine client UUID with following semantics for 'name':
+	 *
+	 * For TEEC_LOGIN_USER:
+	 * uid=<uid>
+	 *
+	 * For TEEC_LOGIN_GROUP:
+	 * gid=<gid>
+	 *
+	 */
+
+	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+
+	switch (connection_method) {
+	case TEE_IOCTL_LOGIN_USER:
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
+			  current_euid().val);
+		break;
+
+	case TEE_IOCTL_LOGIN_GROUP:
+		memcpy(&ns_grp, connection_data, sizeof(gid_t));
+		grp = make_kgid(current_user_ns(), ns_grp);
+		if (!gid_valid(grp) || !in_egroup_p(grp)) {
+			rc = -EPERM;
+			goto out;
+		}
+
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "gid=%x", grp.val);
+		break;
+
+	default:
+		rc = -EINVAL;
+		goto out;
+	}
+
+	rc = uuid_v5(uuid, &tee_client_uuid_ns, name, strlen(name));
+out:
+	kfree(name);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tee_session_calc_client_uuid);
+
 static int tee_ioctl_version(struct tee_context *ctx,
 			     struct tee_ioctl_version_data __user *uvers)
 {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 1412e9cc79ce..8471b790e858 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -165,6 +165,22 @@ int tee_device_register(struct tee_device *teedev);
  */
 void tee_device_unregister(struct tee_device *teedev);
 
+/**
+ * tee_session_calc_client_uuid() - Calculates client UUID for session
+ * @uuid:		Resulting UUID
+ * @connection_method:	Connection method for session (TEE_IOCTL_LOGIN_*)
+ * @connectuon_data:	Connection data for opening session
+ *
+ * Based on connection method calculates UUIDv5 based client UUID.
+ *
+ * For group based logins verifies that calling process has specified
+ * credentials.
+ *
+ * @return < 0 on failure
+ */
+int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
+				 const u8 connection_data[TEE_IOCTL_UUID_LEN]);
+
 /**
  * struct tee_shm - shared memory object
  * @ctx:	context using the object
-- 
2.17.1

Re: [op-tee] [PATCH 1/3] tee: add support for session's client UUID generation

[Dan Carpenter]

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

On Thu, Apr 23, 2020 at 06:16:59PM +0300, Vesa Jääskeläinen wrote:
> +static int uuid_v5(uuid_t *uuid, const uuid_t *ns, const void *name,
> +		   size_t size)
> +{
> +	unsigned char hash[SHA1_DIGEST_SIZE];
> +	struct crypto_shash *shash = NULL;
> +	struct shash_desc *desc = NULL;
> +	int rc;
> +
> +	shash = crypto_alloc_shash("sha1", 0, 0);
> +	if (IS_ERR(shash)) {
> +		rc = PTR_ERR(shash);
> +		pr_err("shash(sha1) allocation failed\n");
> +		return rc;
> +	}
> +
> +	desc = kzalloc(sizeof(*desc) + crypto_shash_descsize(shash),
> +		       GFP_KERNEL);
> +	if (IS_ERR(desc)) {


kzalloc() returns NULL on error.

> +		rc = PTR_ERR(desc);
> +		goto out;

Please choose a label name so that we can tell what the goto will do
like "goto free_shash;".

> +	}
> +
> +	desc->tfm = shash;
> +
> +	rc = crypto_shash_init(desc);
> +	if (rc < 0)
> +		goto out2;
> +
> +	rc = crypto_shash_update(desc, (const u8 *)ns, sizeof(*ns));
> +	if (rc < 0)
> +		goto out2;
> +
> +	rc = crypto_shash_update(desc, (const u8 *)name, size);
> +	if (rc < 0)
> +		goto out2;
> +
> +	rc = crypto_shash_final(desc, hash);
> +	if (rc < 0)
> +		goto out2;
> +
> +	memcpy(uuid->b, hash, UUID_SIZE);
> +
> +	/* Tag for version 5 */
> +	uuid->b[6] = (hash[6] & 0x0F) | 0x50;
> +	uuid->b[8] = (hash[8] & 0x3F) | 0x80;
> +
> +out2:
> +	kfree(desc);
> +
> +out:
> +	crypto_free_shash(shash);
> +	return rc;
> +}
> +
> +int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
> +				 const u8 connection_data[TEE_IOCTL_UUID_LEN])
> +{
> +	const char *application_id = NULL;
> +	gid_t ns_grp = (gid_t)-1;
> +	kgid_t grp = INVALID_GID;
> +	char *name = NULL;
> +	int rc;
> +
> +	if (connection_method == TEE_IOCTL_LOGIN_PUBLIC) {
> +		/* Nil UUID to be passed to TEE environment */
> +		uuid_copy(uuid, &uuid_null);
> +		return 0;
> +	}
> +
> +	/*
> +	 * In Linux environment client UUID is based on UUIDv5.
> +	 *
> +	 * Determine client UUID with following semantics for 'name':
> +	 *
> +	 * For TEEC_LOGIN_USER:
> +	 * uid=<uid>
> +	 *
> +	 * For TEEC_LOGIN_GROUP:
> +	 * gid=<gid>
> +	 *
> +	 */
> +
> +	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	switch (connection_method) {
> +	case TEE_IOCTL_LOGIN_USER:
> +		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
> +			  current_euid().val);

It doesn't make sense to use sCnprintf() instead of snprintf() if we're
not going to preserve the error code.  Probably it's better to preserve
the error code actually so we can avoid the strlen(name) later on.

> +		break;
> +
> +	case TEE_IOCTL_LOGIN_GROUP:
> +		memcpy(&ns_grp, connection_data, sizeof(gid_t));
> +		grp = make_kgid(current_user_ns(), ns_grp);
> +		if (!gid_valid(grp) || !in_egroup_p(grp)) {
> +			rc = -EPERM;
> +			goto out;
> +		}
> +
> +		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "gid=%x", grp.val);
> +		break;
> +
> +	default:
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	rc = uuid_v5(uuid, &tee_client_uuid_ns, name, strlen(name));
> +out:
> +	kfree(name);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(tee_session_calc_client_uuid);

regards,
dan carpenter

Re: [PATCH 1/3] tee: add support for session's client UUID generation

[Dan Carpenter]

On Thu, Apr 23, 2020 at 06:16:59PM +0300, Vesa Jääskeläinen wrote:
> +static int uuid_v5(uuid_t *uuid, const uuid_t *ns, const void *name,
> +		   size_t size)
> +{
> +	unsigned char hash[SHA1_DIGEST_SIZE];
> +	struct crypto_shash *shash = NULL;
> +	struct shash_desc *desc = NULL;
> +	int rc;
> +
> +	shash = crypto_alloc_shash("sha1", 0, 0);
> +	if (IS_ERR(shash)) {
> +		rc = PTR_ERR(shash);
> +		pr_err("shash(sha1) allocation failed\n");
> +		return rc;
> +	}
> +
> +	desc = kzalloc(sizeof(*desc) + crypto_shash_descsize(shash),
> +		       GFP_KERNEL);
> +	if (IS_ERR(desc)) {


kzalloc() returns NULL on error.

> +		rc = PTR_ERR(desc);
> +		goto out;

Please choose a label name so that we can tell what the goto will do
like "goto free_shash;".

> +	}
> +
> +	desc->tfm = shash;
> +
> +	rc = crypto_shash_init(desc);
> +	if (rc < 0)
> +		goto out2;
> +
> +	rc = crypto_shash_update(desc, (const u8 *)ns, sizeof(*ns));
> +	if (rc < 0)
> +		goto out2;
> +
> +	rc = crypto_shash_update(desc, (const u8 *)name, size);
> +	if (rc < 0)
> +		goto out2;
> +
> +	rc = crypto_shash_final(desc, hash);
> +	if (rc < 0)
> +		goto out2;
> +
> +	memcpy(uuid->b, hash, UUID_SIZE);
> +
> +	/* Tag for version 5 */
> +	uuid->b[6] = (hash[6] & 0x0F) | 0x50;
> +	uuid->b[8] = (hash[8] & 0x3F) | 0x80;
> +
> +out2:
> +	kfree(desc);
> +
> +out:
> +	crypto_free_shash(shash);
> +	return rc;
> +}
> +
> +int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
> +				 const u8 connection_data[TEE_IOCTL_UUID_LEN])
> +{
> +	const char *application_id = NULL;
> +	gid_t ns_grp = (gid_t)-1;
> +	kgid_t grp = INVALID_GID;
> +	char *name = NULL;
> +	int rc;
> +
> +	if (connection_method == TEE_IOCTL_LOGIN_PUBLIC) {
> +		/* Nil UUID to be passed to TEE environment */
> +		uuid_copy(uuid, &uuid_null);
> +		return 0;
> +	}
> +
> +	/*
> +	 * In Linux environment client UUID is based on UUIDv5.
> +	 *
> +	 * Determine client UUID with following semantics for 'name':
> +	 *
> +	 * For TEEC_LOGIN_USER:
> +	 * uid=<uid>
> +	 *
> +	 * For TEEC_LOGIN_GROUP:
> +	 * gid=<gid>
> +	 *
> +	 */
> +
> +	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	switch (connection_method) {
> +	case TEE_IOCTL_LOGIN_USER:
> +		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
> +			  current_euid().val);

It doesn't make sense to use sCnprintf() instead of snprintf() if we're
not going to preserve the error code.  Probably it's better to preserve
the error code actually so we can avoid the strlen(name) later on.

> +		break;
> +
> +	case TEE_IOCTL_LOGIN_GROUP:
> +		memcpy(&ns_grp, connection_data, sizeof(gid_t));
> +		grp = make_kgid(current_user_ns(), ns_grp);
> +		if (!gid_valid(grp) || !in_egroup_p(grp)) {
> +			rc = -EPERM;
> +			goto out;
> +		}
> +
> +		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "gid=%x", grp.val);
> +		break;
> +
> +	default:
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	rc = uuid_v5(uuid, &tee_client_uuid_ns, name, strlen(name));
> +out:
> +	kfree(name);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(tee_session_calc_client_uuid);

regards,
dan carpenter

Re: [op-tee] [PATCH 1/3] tee: add support for session's client UUID generation

[Vesa Jääskeläinen]

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

Hi Dan,

Thanks for comments!

On 2020-04-23 20:35, Dan Carpenter wrote:
> On Thu, Apr 23, 2020 at 06:16:59PM +0300, Vesa Jääskeläinen wrote:
>> +static int uuid_v5(uuid_t *uuid, const uuid_t *ns, const void *name,
>> +		   size_t size)
>> +{
>> +	unsigned char hash[SHA1_DIGEST_SIZE];
>> +	struct crypto_shash *shash = NULL;
>> +	struct shash_desc *desc = NULL;
>> +	int rc;
>> +
>> +	shash = crypto_alloc_shash("sha1", 0, 0);
>> +	if (IS_ERR(shash)) {
>> +		rc = PTR_ERR(shash);
>> +		pr_err("shash(sha1) allocation failed\n");
>> +		return rc;
>> +	}
>> +
>> +	desc = kzalloc(sizeof(*desc) + crypto_shash_descsize(shash),
>> +		       GFP_KERNEL);
>> +	if (IS_ERR(desc)) {
> 
> 
> kzalloc() returns NULL on error.

Err...Right. Will fix for V2. Thanks for noticing this.

I was probably confused by this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/slab.h?h=v5.7-rc2#n553

As there is no error handling case documented for kmalloc and friends -- 
I was just looking that as there are non-zero error cases in that 
referenced line (ZERO_SIZE_PTR) so we need to check if pointer has error 
value as it was with crypto_alloc_shash().

But looking at users of kzalloc then convention seems to be just check 
for NULL.

Probably referenced code is then expected to cause page fault on error 
case as ZERO_SIZE_PTR is not error value as such.

> 
>> +		rc = PTR_ERR(desc);
>> +		goto out;
> 
> Please choose a label name so that we can tell what the goto will do
> like "goto free_shash;".

Ok. Will update for V2.

>> +	}
>> +
>> +	desc->tfm = shash;
>> +
>> +	rc = crypto_shash_init(desc);
>> +	if (rc < 0)
>> +		goto out2;
>> +
>> +	rc = crypto_shash_update(desc, (const u8 *)ns, sizeof(*ns));
>> +	if (rc < 0)
>> +		goto out2;
>> +
>> +	rc = crypto_shash_update(desc, (const u8 *)name, size);
>> +	if (rc < 0)
>> +		goto out2;
>> +
>> +	rc = crypto_shash_final(desc, hash);
>> +	if (rc < 0)
>> +		goto out2;
>> +
>> +	memcpy(uuid->b, hash, UUID_SIZE);
>> +
>> +	/* Tag for version 5 */
>> +	uuid->b[6] = (hash[6] & 0x0F) | 0x50;
>> +	uuid->b[8] = (hash[8] & 0x3F) | 0x80;
>> +
>> +out2:
>> +	kfree(desc);
>> +
>> +out:
>> +	crypto_free_shash(shash);
>> +	return rc;
>> +}
>> +
>> +int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
>> +				 const u8 connection_data[TEE_IOCTL_UUID_LEN])
>> +{
>> +	const char *application_id = NULL;
>> +	gid_t ns_grp = (gid_t)-1;
>> +	kgid_t grp = INVALID_GID;
>> +	char *name = NULL;
>> +	int rc;
>> +
>> +	if (connection_method == TEE_IOCTL_LOGIN_PUBLIC) {
>> +		/* Nil UUID to be passed to TEE environment */
>> +		uuid_copy(uuid, &uuid_null);
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * In Linux environment client UUID is based on UUIDv5.
>> +	 *
>> +	 * Determine client UUID with following semantics for 'name':
>> +	 *
>> +	 * For TEEC_LOGIN_USER:
>> +	 * uid=<uid>
>> +	 *
>> +	 * For TEEC_LOGIN_GROUP:
>> +	 * gid=<gid>
>> +	 *
>> +	 */
>> +
>> +	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
>> +	if (!name)
>> +		return -ENOMEM;
>> +
>> +	switch (connection_method) {
>> +	case TEE_IOCTL_LOGIN_USER:
>> +		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
>> +			  current_euid().val);
> 
> It doesn't make sense to use sCnprintf() instead of snprintf() if we're
> not going to preserve the error code.  Probably it's better to preserve
> the error code actually so we can avoid the strlen(name) later on.

Ok. I assume you meant here using snprintf where we can capture the 
error situation of too large output string. scnprintf does not seem to 
have error returning capabilities.

I assume you meant something like this:

name_len = snprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
		    current_euid().val);
if (name_len >= TEE_UUID_NS_NAME_SIZE) {
	rc = -E2BIG;
	goto out_free_name;
}

Will wait a bit more for comments before sending V2.

I already updated my devel branch with these for those wanting to play 
around with the updates:
https://github.com/vesajaaskelainen/linux/commits/optee-login-checks

Thanks,
Vesa Jääskeläinen

Re: [PATCH 1/3] tee: add support for session's client UUID generation

[Vesa Jääskeläinen]

Hi Dan,

Thanks for comments!

On 2020-04-23 20:35, Dan Carpenter wrote:
> On Thu, Apr 23, 2020 at 06:16:59PM +0300, Vesa Jääskeläinen wrote:
>> +static int uuid_v5(uuid_t *uuid, const uuid_t *ns, const void *name,
>> +		   size_t size)
>> +{
>> +	unsigned char hash[SHA1_DIGEST_SIZE];
>> +	struct crypto_shash *shash = NULL;
>> +	struct shash_desc *desc = NULL;
>> +	int rc;
>> +
>> +	shash = crypto_alloc_shash("sha1", 0, 0);
>> +	if (IS_ERR(shash)) {
>> +		rc = PTR_ERR(shash);
>> +		pr_err("shash(sha1) allocation failed\n");
>> +		return rc;
>> +	}
>> +
>> +	desc = kzalloc(sizeof(*desc) + crypto_shash_descsize(shash),
>> +		       GFP_KERNEL);
>> +	if (IS_ERR(desc)) {
> 
> 
> kzalloc() returns NULL on error.

Err...Right. Will fix for V2. Thanks for noticing this.

I was probably confused by this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/slab.h?h=v5.7-rc2#n553

As there is no error handling case documented for kmalloc and friends -- 
I was just looking that as there are non-zero error cases in that 
referenced line (ZERO_SIZE_PTR) so we need to check if pointer has error 
value as it was with crypto_alloc_shash().

But looking at users of kzalloc then convention seems to be just check 
for NULL.

Probably referenced code is then expected to cause page fault on error 
case as ZERO_SIZE_PTR is not error value as such.

> 
>> +		rc = PTR_ERR(desc);
>> +		goto out;
> 
> Please choose a label name so that we can tell what the goto will do
> like "goto free_shash;".

Ok. Will update for V2.

>> +	}
>> +
>> +	desc->tfm = shash;
>> +
>> +	rc = crypto_shash_init(desc);
>> +	if (rc < 0)
>> +		goto out2;
>> +
>> +	rc = crypto_shash_update(desc, (const u8 *)ns, sizeof(*ns));
>> +	if (rc < 0)
>> +		goto out2;
>> +
>> +	rc = crypto_shash_update(desc, (const u8 *)name, size);
>> +	if (rc < 0)
>> +		goto out2;
>> +
>> +	rc = crypto_shash_final(desc, hash);
>> +	if (rc < 0)
>> +		goto out2;
>> +
>> +	memcpy(uuid->b, hash, UUID_SIZE);
>> +
>> +	/* Tag for version 5 */
>> +	uuid->b[6] = (hash[6] & 0x0F) | 0x50;
>> +	uuid->b[8] = (hash[8] & 0x3F) | 0x80;
>> +
>> +out2:
>> +	kfree(desc);
>> +
>> +out:
>> +	crypto_free_shash(shash);
>> +	return rc;
>> +}
>> +
>> +int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
>> +				 const u8 connection_data[TEE_IOCTL_UUID_LEN])
>> +{
>> +	const char *application_id = NULL;
>> +	gid_t ns_grp = (gid_t)-1;
>> +	kgid_t grp = INVALID_GID;
>> +	char *name = NULL;
>> +	int rc;
>> +
>> +	if (connection_method == TEE_IOCTL_LOGIN_PUBLIC) {
>> +		/* Nil UUID to be passed to TEE environment */
>> +		uuid_copy(uuid, &uuid_null);
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * In Linux environment client UUID is based on UUIDv5.
>> +	 *
>> +	 * Determine client UUID with following semantics for 'name':
>> +	 *
>> +	 * For TEEC_LOGIN_USER:
>> +	 * uid=<uid>
>> +	 *
>> +	 * For TEEC_LOGIN_GROUP:
>> +	 * gid=<gid>
>> +	 *
>> +	 */
>> +
>> +	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
>> +	if (!name)
>> +		return -ENOMEM;
>> +
>> +	switch (connection_method) {
>> +	case TEE_IOCTL_LOGIN_USER:
>> +		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
>> +			  current_euid().val);
> 
> It doesn't make sense to use sCnprintf() instead of snprintf() if we're
> not going to preserve the error code.  Probably it's better to preserve
> the error code actually so we can avoid the strlen(name) later on.

Ok. I assume you meant here using snprintf where we can capture the 
error situation of too large output string. scnprintf does not seem to 
have error returning capabilities.

I assume you meant something like this:

name_len = snprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
		    current_euid().val);
if (name_len >= TEE_UUID_NS_NAME_SIZE) {
	rc = -E2BIG;
	goto out_free_name;
}

Will wait a bit more for comments before sending V2.

I already updated my devel branch with these for those wanting to play 
around with the updates:
https://github.com/vesajaaskelainen/linux/commits/optee-login-checks

Thanks,
Vesa Jääskeläinen

Re: [op-tee] [PATCH 1/3] tee: add support for session's client UUID generation

[Dan Carpenter]

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

On Sat, Apr 25, 2020 at 09:16:57AM +0300, Vesa Jääskeläinen wrote:
> Hi Dan,
> 
> Thanks for comments!
> 
> On 2020-04-23 20:35, Dan Carpenter wrote:
> > On Thu, Apr 23, 2020 at 06:16:59PM +0300, Vesa Jääskeläinen wrote:
> > > +static int uuid_v5(uuid_t *uuid, const uuid_t *ns, const void *name,
> > > +		   size_t size)
> > > +{
> > > +	unsigned char hash[SHA1_DIGEST_SIZE];
> > > +	struct crypto_shash *shash = NULL;
> > > +	struct shash_desc *desc = NULL;
> > > +	int rc;
> > > +
> > > +	shash = crypto_alloc_shash("sha1", 0, 0);
> > > +	if (IS_ERR(shash)) {
> > > +		rc = PTR_ERR(shash);
> > > +		pr_err("shash(sha1) allocation failed\n");
> > > +		return rc;
> > > +	}
> > > +
> > > +	desc = kzalloc(sizeof(*desc) + crypto_shash_descsize(shash),
> > > +		       GFP_KERNEL);
> > > +	if (IS_ERR(desc)) {
> > 
> > 
> > kzalloc() returns NULL on error.
> 
> Err...Right. Will fix for V2. Thanks for noticing this.
> 
> I was probably confused by this:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/slab.h?h=v5.7-rc2#n553
> 
> As there is no error handling case documented for kmalloc and friends -- I
> was just looking that as there are non-zero error cases in that referenced
> line (ZERO_SIZE_PTR) so we need to check if pointer has error value as it
> was with crypto_alloc_shash().
> 
> But looking at users of kzalloc then convention seems to be just check for
> NULL.
> 
> Probably referenced code is then expected to cause page fault on error case
> as ZERO_SIZE_PTR is not error value as such.


Zero size buffers are not an error.  You can copy zero bytes to them or
from them without a problem.

	size = 0;
	p = kmalloc(size, GFP_KERNEL);
	memcpy(p, src, 0);
	for (i = 0; i < size; i++)
		printk("%d\n", p[i]);
	copy_to_user(user_ptr, p, size);

All that's fine and does nothing.  You should never test for
ZERO_SIZE_PTR.  Check for "size == 0" instead.

> 
> > 
> > > +		rc = PTR_ERR(desc);
> > > +		goto out;
> > 
> > Please choose a label name so that we can tell what the goto will do
> > like "goto free_shash;".
> 
> Ok. Will update for V2.
> 
> > > +	}
> > > +
> > > +	desc->tfm = shash;
> > > +
> > > +	rc = crypto_shash_init(desc);
> > > +	if (rc < 0)
> > > +		goto out2;
> > > +
> > > +	rc = crypto_shash_update(desc, (const u8 *)ns, sizeof(*ns));
> > > +	if (rc < 0)
> > > +		goto out2;
> > > +
> > > +	rc = crypto_shash_update(desc, (const u8 *)name, size);
> > > +	if (rc < 0)
> > > +		goto out2;
> > > +
> > > +	rc = crypto_shash_final(desc, hash);
> > > +	if (rc < 0)
> > > +		goto out2;
> > > +
> > > +	memcpy(uuid->b, hash, UUID_SIZE);
> > > +
> > > +	/* Tag for version 5 */
> > > +	uuid->b[6] = (hash[6] & 0x0F) | 0x50;
> > > +	uuid->b[8] = (hash[8] & 0x3F) | 0x80;
> > > +
> > > +out2:
> > > +	kfree(desc);
> > > +
> > > +out:
> > > +	crypto_free_shash(shash);
> > > +	return rc;
> > > +}
> > > +
> > > +int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
> > > +				 const u8 connection_data[TEE_IOCTL_UUID_LEN])
> > > +{
> > > +	const char *application_id = NULL;
> > > +	gid_t ns_grp = (gid_t)-1;
> > > +	kgid_t grp = INVALID_GID;
> > > +	char *name = NULL;
> > > +	int rc;
> > > +
> > > +	if (connection_method == TEE_IOCTL_LOGIN_PUBLIC) {
> > > +		/* Nil UUID to be passed to TEE environment */
> > > +		uuid_copy(uuid, &uuid_null);
> > > +		return 0;
> > > +	}
> > > +
> > > +	/*
> > > +	 * In Linux environment client UUID is based on UUIDv5.
> > > +	 *
> > > +	 * Determine client UUID with following semantics for 'name':
> > > +	 *
> > > +	 * For TEEC_LOGIN_USER:
> > > +	 * uid=<uid>
> > > +	 *
> > > +	 * For TEEC_LOGIN_GROUP:
> > > +	 * gid=<gid>
> > > +	 *
> > > +	 */
> > > +
> > > +	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
> > > +	if (!name)
> > > +		return -ENOMEM;
> > > +
> > > +	switch (connection_method) {
> > > +	case TEE_IOCTL_LOGIN_USER:
> > > +		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
> > > +			  current_euid().val);
> > 
> > It doesn't make sense to use sCnprintf() instead of snprintf() if we're
> > not going to preserve the error code.  Probably it's better to preserve
> > the error code actually so we can avoid the strlen(name) later on.
> 
> Ok. I assume you meant here using snprintf where we can capture the error
> situation of too large output string. scnprintf does not seem to have error
> returning capabilities.
> 
> I assume you meant something like this:
> 
> name_len = snprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
> 		    current_euid().val);
> if (name_len >= TEE_UUID_NS_NAME_SIZE) {
> 	rc = -E2BIG;
> 	goto out_free_name;
> }
> 

None of the kernel snprintf() functions return negative error codes.
The difference between snprintf() and scnprintf() is that snprintf()
returns the number of bytes that would have been copied if there were
enough space and scnprintf() returns the number of bytes which were
*actually* copied.

But if we don't care about the return code, then we should default to
using snprintf() because it is the simpler looking function.  Always
right the simplest code you can.

regards,
dan carpenter

Re: [PATCH 1/3] tee: add support for session's client UUID generation

[Dan Carpenter]

On Sat, Apr 25, 2020 at 09:16:57AM +0300, Vesa Jääskeläinen wrote:
> Hi Dan,
> 
> Thanks for comments!
> 
> On 2020-04-23 20:35, Dan Carpenter wrote:
> > On Thu, Apr 23, 2020 at 06:16:59PM +0300, Vesa Jääskeläinen wrote:
> > > +static int uuid_v5(uuid_t *uuid, const uuid_t *ns, const void *name,
> > > +		   size_t size)
> > > +{
> > > +	unsigned char hash[SHA1_DIGEST_SIZE];
> > > +	struct crypto_shash *shash = NULL;
> > > +	struct shash_desc *desc = NULL;
> > > +	int rc;
> > > +
> > > +	shash = crypto_alloc_shash("sha1", 0, 0);
> > > +	if (IS_ERR(shash)) {
> > > +		rc = PTR_ERR(shash);
> > > +		pr_err("shash(sha1) allocation failed\n");
> > > +		return rc;
> > > +	}
> > > +
> > > +	desc = kzalloc(sizeof(*desc) + crypto_shash_descsize(shash),
> > > +		       GFP_KERNEL);
> > > +	if (IS_ERR(desc)) {
> > 
> > 
> > kzalloc() returns NULL on error.
> 
> Err...Right. Will fix for V2. Thanks for noticing this.
> 
> I was probably confused by this:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/slab.h?h=v5.7-rc2#n553
> 
> As there is no error handling case documented for kmalloc and friends -- I
> was just looking that as there are non-zero error cases in that referenced
> line (ZERO_SIZE_PTR) so we need to check if pointer has error value as it
> was with crypto_alloc_shash().
> 
> But looking at users of kzalloc then convention seems to be just check for
> NULL.
> 
> Probably referenced code is then expected to cause page fault on error case
> as ZERO_SIZE_PTR is not error value as such.


Zero size buffers are not an error.  You can copy zero bytes to them or
from them without a problem.

	size = 0;
	p = kmalloc(size, GFP_KERNEL);
	memcpy(p, src, 0);
	for (i = 0; i < size; i++)
		printk("%d\n", p[i]);
	copy_to_user(user_ptr, p, size);

All that's fine and does nothing.  You should never test for
ZERO_SIZE_PTR.  Check for "size == 0" instead.

> 
> > 
> > > +		rc = PTR_ERR(desc);
> > > +		goto out;
> > 
> > Please choose a label name so that we can tell what the goto will do
> > like "goto free_shash;".
> 
> Ok. Will update for V2.
> 
> > > +	}
> > > +
> > > +	desc->tfm = shash;
> > > +
> > > +	rc = crypto_shash_init(desc);
> > > +	if (rc < 0)
> > > +		goto out2;
> > > +
> > > +	rc = crypto_shash_update(desc, (const u8 *)ns, sizeof(*ns));
> > > +	if (rc < 0)
> > > +		goto out2;
> > > +
> > > +	rc = crypto_shash_update(desc, (const u8 *)name, size);
> > > +	if (rc < 0)
> > > +		goto out2;
> > > +
> > > +	rc = crypto_shash_final(desc, hash);
> > > +	if (rc < 0)
> > > +		goto out2;
> > > +
> > > +	memcpy(uuid->b, hash, UUID_SIZE);
> > > +
> > > +	/* Tag for version 5 */
> > > +	uuid->b[6] = (hash[6] & 0x0F) | 0x50;
> > > +	uuid->b[8] = (hash[8] & 0x3F) | 0x80;
> > > +
> > > +out2:
> > > +	kfree(desc);
> > > +
> > > +out:
> > > +	crypto_free_shash(shash);
> > > +	return rc;
> > > +}
> > > +
> > > +int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
> > > +				 const u8 connection_data[TEE_IOCTL_UUID_LEN])
> > > +{
> > > +	const char *application_id = NULL;
> > > +	gid_t ns_grp = (gid_t)-1;
> > > +	kgid_t grp = INVALID_GID;
> > > +	char *name = NULL;
> > > +	int rc;
> > > +
> > > +	if (connection_method == TEE_IOCTL_LOGIN_PUBLIC) {
> > > +		/* Nil UUID to be passed to TEE environment */
> > > +		uuid_copy(uuid, &uuid_null);
> > > +		return 0;
> > > +	}
> > > +
> > > +	/*
> > > +	 * In Linux environment client UUID is based on UUIDv5.
> > > +	 *
> > > +	 * Determine client UUID with following semantics for 'name':
> > > +	 *
> > > +	 * For TEEC_LOGIN_USER:
> > > +	 * uid=<uid>
> > > +	 *
> > > +	 * For TEEC_LOGIN_GROUP:
> > > +	 * gid=<gid>
> > > +	 *
> > > +	 */
> > > +
> > > +	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
> > > +	if (!name)
> > > +		return -ENOMEM;
> > > +
> > > +	switch (connection_method) {
> > > +	case TEE_IOCTL_LOGIN_USER:
> > > +		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
> > > +			  current_euid().val);
> > 
> > It doesn't make sense to use sCnprintf() instead of snprintf() if we're
> > not going to preserve the error code.  Probably it's better to preserve
> > the error code actually so we can avoid the strlen(name) later on.
> 
> Ok. I assume you meant here using snprintf where we can capture the error
> situation of too large output string. scnprintf does not seem to have error
> returning capabilities.
> 
> I assume you meant something like this:
> 
> name_len = snprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x",
> 		    current_euid().val);
> if (name_len >= TEE_UUID_NS_NAME_SIZE) {
> 	rc = -E2BIG;
> 	goto out_free_name;
> }
> 

None of the kernel snprintf() functions return negative error codes.
The difference between snprintf() and scnprintf() is that snprintf()
returns the number of bytes that would have been copied if there were
enough space and scnprintf() returns the number of bytes which were
*actually* copied.

But if we don't care about the return code, then we should default to
using snprintf() because it is the simpler looking function.  Always
right the simplest code you can.

regards,
dan carpenter

[op-tee] [PATCH 2/3] tee: optee: Add support for session login client UUID generation

[Vesa Jääskeläinen]

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

Adds support for client UUID generation for OP-TEE. For group based session
logins membership is verified.

Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>
---
 drivers/tee/optee/call.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index cf2367ba08d6..dbed3f480dc0 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -233,9 +233,13 @@ int optee_open_session(struct tee_context *ctx,
 	msg_arg->params[1].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
 				  OPTEE_MSG_ATTR_META;
 	memcpy(&msg_arg->params[0].u.value, arg->uuid, sizeof(arg->uuid));
-	memcpy(&msg_arg->params[1].u.value, arg->uuid, sizeof(arg->clnt_uuid));
 	msg_arg->params[1].u.value.c = arg->clnt_login;
 
+	rc = tee_session_calc_client_uuid((uuid_t *)&msg_arg->params[1].u.value,
+					  arg->clnt_login, arg->clnt_uuid);
+	if (rc)
+		goto out;
+
 	rc = optee_to_msg_param(msg_arg->params + 2, arg->num_params, param);
 	if (rc)
 		goto out;
-- 
2.17.1

[PATCH 2/3] tee: optee: Add support for session login client UUID generation

[Vesa Jääskeläinen]

Adds support for client UUID generation for OP-TEE. For group based session
logins membership is verified.

Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>
---
 drivers/tee/optee/call.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index cf2367ba08d6..dbed3f480dc0 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -233,9 +233,13 @@ int optee_open_session(struct tee_context *ctx,
 	msg_arg->params[1].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT |
 				  OPTEE_MSG_ATTR_META;
 	memcpy(&msg_arg->params[0].u.value, arg->uuid, sizeof(arg->uuid));
-	memcpy(&msg_arg->params[1].u.value, arg->uuid, sizeof(arg->clnt_uuid));
 	msg_arg->params[1].u.value.c = arg->clnt_login;
 
+	rc = tee_session_calc_client_uuid((uuid_t *)&msg_arg->params[1].u.value,
+					  arg->clnt_login, arg->clnt_uuid);
+	if (rc)
+		goto out;
+
 	rc = optee_to_msg_param(msg_arg->params + 2, arg->num_params, param);
 	if (rc)
 		goto out;
-- 
2.17.1

[op-tee] [PATCH 3/3] [RFC] tee: add support for app id for client UUID generation

[Vesa Jääskeläinen]

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

Linux kernel does not provide common contex for application identifier,
instead different security frameworks provide own means to define
application identifier for running process. Code includes place holder for
such solutions but is left for later implementation.

Open questions:

1. App ID source

How to specify what source is used for app id?

Does it need to be protected on runtime?
- Should this be Kconfig setting?
- Cnfigure once during runtime thru sysfs or so?
- Configure from device tree?

2. Formatting for App ID

Should there be common format? Or common keyword id?

3. How to handle custom App ID sources

Android has own App ID so does Tizen.

Should there be place holder for this where to make local patch?

Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>
---
 drivers/tee/tee_core.c | 45 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 872272bf9dec..df03bd0071da 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -125,6 +125,15 @@ static int tee_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static const char *tee_session_get_application_id(void)
+{
+	return NULL;
+}
+
+static void tee_session_free_application_id(const char *app_id)
+{
+}
+
 /**
  * uuid_v5() - Calculate UUIDv5
  * @uuid: Resulting UUID
@@ -217,6 +226,14 @@ int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
 	 * For TEEC_LOGIN_GROUP:
 	 * gid=<gid>
 	 *
+	 * For TEEC_LOGIN_APPLICATION:
+	 * app=<application id>
+	 *
+	 * For TEEC_LOGIN_USER_APPLICATION:
+	 * uid=<uid>:app=<application id>
+	 *
+	 * For TEEC_LOGIN_GROUP_APPLICATION:
+	 * gid=<gid>:app=<application id>
 	 */
 
 	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
@@ -240,6 +257,34 @@ int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
 		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "gid=%x", grp.val);
 		break;
 
+	case TEE_IOCTL_LOGIN_APPLICATION:
+		application_id = tee_session_get_application_id();
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "app=%s",
+			  application_id);
+		tee_session_free_application_id(application_id);
+		break;
+
+	case TEE_IOCTL_LOGIN_USER_APPLICATION:
+		application_id = tee_session_get_application_id();
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x:app=%s",
+			  current_euid().val, application_id);
+		tee_session_free_application_id(application_id);
+		break;
+
+	case TEE_IOCTL_LOGIN_GROUP_APPLICATION:
+		memcpy(&ns_grp, connection_data, sizeof(gid_t));
+		grp = make_kgid(current_user_ns(), ns_grp);
+		if (!gid_valid(grp) || !in_egroup_p(grp)) {
+			rc = -EPERM;
+			goto out;
+		}
+
+		application_id = tee_session_get_application_id();
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "gid=%x:app=%s",
+			  grp.val, application_id);
+		tee_session_free_application_id(application_id);
+		break;
+
 	default:
 		rc = -EINVAL;
 		goto out;
-- 
2.17.1

[PATCH 3/3] [RFC] tee: add support for app id for client UUID generation

[Vesa Jääskeläinen]

Linux kernel does not provide common contex for application identifier,
instead different security frameworks provide own means to define
application identifier for running process. Code includes place holder for
such solutions but is left for later implementation.

Open questions:

1. App ID source

How to specify what source is used for app id?

Does it need to be protected on runtime?
- Should this be Kconfig setting?
- Cnfigure once during runtime thru sysfs or so?
- Configure from device tree?

2. Formatting for App ID

Should there be common format? Or common keyword id?

3. How to handle custom App ID sources

Android has own App ID so does Tizen.

Should there be place holder for this where to make local patch?

Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com>
---
 drivers/tee/tee_core.c | 45 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 872272bf9dec..df03bd0071da 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -125,6 +125,15 @@ static int tee_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static const char *tee_session_get_application_id(void)
+{
+	return NULL;
+}
+
+static void tee_session_free_application_id(const char *app_id)
+{
+}
+
 /**
  * uuid_v5() - Calculate UUIDv5
  * @uuid: Resulting UUID
@@ -217,6 +226,14 @@ int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
 	 * For TEEC_LOGIN_GROUP:
 	 * gid=<gid>
 	 *
+	 * For TEEC_LOGIN_APPLICATION:
+	 * app=<application id>
+	 *
+	 * For TEEC_LOGIN_USER_APPLICATION:
+	 * uid=<uid>:app=<application id>
+	 *
+	 * For TEEC_LOGIN_GROUP_APPLICATION:
+	 * gid=<gid>:app=<application id>
 	 */
 
 	name = kzalloc(TEE_UUID_NS_NAME_SIZE, GFP_KERNEL);
@@ -240,6 +257,34 @@ int tee_session_calc_client_uuid(uuid_t *uuid, u32 connection_method,
 		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "gid=%x", grp.val);
 		break;
 
+	case TEE_IOCTL_LOGIN_APPLICATION:
+		application_id = tee_session_get_application_id();
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "app=%s",
+			  application_id);
+		tee_session_free_application_id(application_id);
+		break;
+
+	case TEE_IOCTL_LOGIN_USER_APPLICATION:
+		application_id = tee_session_get_application_id();
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "uid=%x:app=%s",
+			  current_euid().val, application_id);
+		tee_session_free_application_id(application_id);
+		break;
+
+	case TEE_IOCTL_LOGIN_GROUP_APPLICATION:
+		memcpy(&ns_grp, connection_data, sizeof(gid_t));
+		grp = make_kgid(current_user_ns(), ns_grp);
+		if (!gid_valid(grp) || !in_egroup_p(grp)) {
+			rc = -EPERM;
+			goto out;
+		}
+
+		application_id = tee_session_get_application_id();
+		scnprintf(name, TEE_UUID_NS_NAME_SIZE, "gid=%x:app=%s",
+			  grp.val, application_id);
+		tee_session_free_application_id(application_id);
+		break;
+
 	default:
 		rc = -EINVAL;
 		goto out;
-- 
2.17.1