amdtee: Address bug report

PATCH In review Rijo Thomas 3 participants 8 messages Jan 9, 2020 – Jan 16, 2020

Subsystems (2)

Crypto API TEE

lore

Share & tools

lore

Rijo Thomas

author

2020-01-09 12:53 UTC #0 lore raw

cover amdtee: Address bug report

This patch series addresses the bug report submitted by Dan Carpenter.

Link https://lists.linaro.org/pipermail/tee-dev/2020-January/001417.html

Since, these patches are based on cryptodev-2.6 tree, I have included linux-crypto list as well.

This patch series does not fix the static checker warning reported due to incorrect use of IS_ERR. Colin Ian King has submitted a fix for this issue. Link: https://lkml.org/lkml/2020/1/8/88

Rijo Thomas (5):
tee: amdtee: remove unused variable initialization
tee: amdtee: print error message if tee not present
tee: amdtee: skip tee_device_unregister if tee_device_alloc fails
tee: amdtee: rename err label to err_device_unregister
tee: amdtee: remove redundant NULL check for pool

 drivers/tee/amdtee/call.c | 14 +++++++-------
 drivers/tee/amdtee/core.c | 32 +++++++++++++++++---------------
 2 files changed, 24 insertions(+), 22 deletions(-)

--
2.17.1

Rijo Thomas

author

2020-01-09 12:54 UTC #1 lore raw

PATCH 1/5 tee: amdtee: remove unused variable initialization

Remove unused variable initialization from driver code.

If enabled as a compiler option, compiler may throw warning for unused assignments.

Reported-by Dan Carpenter <[email protected]>

Fixes 757cc3e9ff1d ("tee: add AMD-TEE driver")

Signed-off-by Rijo Thomas <[email protected]>

---
 drivers/tee/amdtee/call.c | 14 +++++++-------
 drivers/tee/amdtee/core.c | 14 +++++++-------
 2 files changed, 14 insertions(+), 14 deletions(-)

drivers/tee/amdtee/call.c +7 -7

index 87ccad256686..096dd4d92d39 100644
@@ -124,8 +124,8 @@ static int amd_params_to_tee_params(struct tee_param *tee, u32 count,
 int handle_unload_ta(u32 ta_handle)
 {
 	struct tee_cmd_unload_ta cmd = {0};
-	int ret = 0;
 	u32 status;
+	int ret;
 
 	if (!ta_handle)
 		return -EINVAL;

@@ -145,8 +145,8 @@ int handle_unload_ta(u32 ta_handle)
 int handle_close_session(u32 ta_handle, u32 info)
 {
 	struct tee_cmd_close_session cmd = {0};
-	int ret = 0;
 	u32 status;
+	int ret;
 
 	if (ta_handle == 0)
 		return -EINVAL;

@@ -167,8 +167,8 @@ int handle_close_session(u32 ta_handle, u32 info)
 void handle_unmap_shmem(u32 buf_id)
 {
 	struct tee_cmd_unmap_shared_mem cmd = {0};
-	int ret = 0;
 	u32 status;
+	int ret;
 
 	cmd.buf_id = buf_id;

@@ -183,7 +183,7 @@ int handle_invoke_cmd(struct tee_ioctl_invoke_arg *arg, u32 sinfo,
 		      struct tee_param *p)
 {
 	struct tee_cmd_invoke_cmd cmd = {0};
-	int ret = 0;
+	int ret;
 
 	if (!arg || (!p && arg->num_params))
 		return -EINVAL;

@@ -229,7 +229,7 @@ int handle_map_shmem(u32 count, struct shmem_desc *start, u32 *buf_id)
 {
 	struct tee_cmd_map_shared_mem *cmd;
 	phys_addr_t paddr;
-	int ret = 0, i;
+	int ret, i;
 	u32 status;
 
 	if (!count || !start || !buf_id)

@@ -294,7 +294,7 @@ int handle_open_session(struct tee_ioctl_open_session_arg *arg, u32 *info,
 			struct tee_param *p)
 {
 	struct tee_cmd_open_session cmd = {0};
-	int ret = 0;
+	int ret;
 
 	if (!arg || !info || (!p && arg->num_params))
 		return -EINVAL;

@@ -342,7 +342,7 @@ int handle_load_ta(void *data, u32 size, struct tee_ioctl_open_session_arg *arg)
 {
 	struct tee_cmd_load_ta cmd = {0};
 	phys_addr_t blob;
-	int ret = 0;
+	int ret;
 
 	if (size == 0 || !data || !arg)
 		return -EINVAL;

drivers/tee/amdtee/core.c +7 -7

index 9d0cee1c837f..b3f8235579f7 100644
@@ -50,7 +50,7 @@ static int amdtee_open(struct tee_context *ctx)
 
 static void release_session(struct amdtee_session *sess)
 {
-	int i = 0;
+	int i;
 
 	/* Close any open session */
 	for (i = 0; i < TEE_NUM_SESSIONS; ++i) {

@@ -173,7 +173,7 @@ static int copy_ta_binary(struct tee_context *ctx, void *ptr, void **ta,
 		u16 hi_ver;
 		u8 seq_n[8];
 	} *uuid = ptr;
-	int n = 0, rc = 0;
+	int n, rc = 0;
 
 	n = snprintf(fw_name, TA_PATH_MAX,
 		     "%s/%08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.bin",

@@ -219,9 +219,9 @@ int amdtee_open_session(struct tee_context *ctx,
 	struct amdtee_context_data *ctxdata = ctx->data;
 	struct amdtee_session *sess = NULL;
 	u32 session_info;
-	void *ta = NULL;
 	size_t ta_size;
-	int rc = 0, i;
+	int rc, i;
+	void *ta;
 
 	if (arg->clnt_login != TEE_IOCTL_LOGIN_PUBLIC) {
 		pr_err("unsupported client login method\n");

@@ -368,8 +368,8 @@ int amdtee_map_shmem(struct tee_shm *shm)
 
 void amdtee_unmap_shmem(struct tee_shm *shm)
 {
+	struct amdtee_shm_data *shmnode;
 	u32 buf_id;
-	struct amdtee_shm_data *shmnode = NULL;
 
 	if (!shm)
 		return;

@@ -434,9 +434,9 @@ static const struct tee_desc amdtee_desc = {
 
 static int __init amdtee_driver_init(void)
 {
-	struct amdtee *amdtee = NULL;
 	struct tee_device *teedev;
-	struct tee_shm_pool *pool = ERR_PTR(-EINVAL);
+	struct tee_shm_pool *pool;
+	struct amdtee *amdtee;
 	int rc;
 
 	rc = psp_check_tee_status();

Signature · 1 line

2.17.1

Rijo Thomas

author

2020-01-09 12:54 UTC #2 lore raw

PATCH 2/5 tee: amdtee: print error message if tee not present

In reply to Rijo Thomas

If there is no TEE with which the driver can communicate, then print an error message and return.

Signed-off-by Rijo Thomas <[email protected]>

---
 drivers/tee/amdtee/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/tee/amdtee/core.c +4 -3

index b3f8235579f7..0840be03a3ab 100644
@@ -440,8 +440,10 @@ static int __init amdtee_driver_init(void)
 	int rc;
 
 	rc = psp_check_tee_status();
-	if (rc)
-		goto err_fail;
+	if (rc) {
+		pr_err("amd-tee driver: tee not present\n");
+		return rc;
+	}
 
 	drv_data = kzalloc(sizeof(*drv_data), GFP_KERNEL);
 	if (IS_ERR(drv_data))

@@ -490,7 +492,6 @@ static int __init amdtee_driver_init(void)
 	kfree(drv_data);
 	drv_data = NULL;
 
-err_fail:
 	pr_err("amd-tee driver initialization failed\n");
 	return rc;
 }

Signature · 1 line

2.17.1

Rijo Thomas

author

2020-01-09 12:54 UTC #3 lore raw

PATCH 3/5 tee: amdtee: skip tee_device_unregister if tee_device_alloc fails

In reply to Rijo Thomas

Currently, if tee_device_alloc() fails, then tee_device_unregister() is a no-op. Therefore, skip the function call to tee_device_unregister() by introducing a new goto label 'err_free_pool'.

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Rijo Thomas <[email protected]>

---
 drivers/tee/amdtee/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/tee/amdtee/core.c +3 -1

index 0840be03a3ab..c657b9728eae 100644
@@ -465,7 +465,7 @@ static int __init amdtee_driver_init(void)
 	teedev = tee_device_alloc(&amdtee_desc, NULL, pool, amdtee);
 	if (IS_ERR(teedev)) {
 		rc = PTR_ERR(teedev);
-		goto err;
+		goto err_free_pool;
 	}
 	amdtee->teedev = teedev;

@@ -482,6 +482,8 @@ static int __init amdtee_driver_init(void)
 
 err:
 	tee_device_unregister(amdtee->teedev);
+
+err_free_pool:
 	if (pool)
 		tee_shm_pool_free(pool);

Signature · 1 line

2.17.1

Rijo Thomas

author

2020-01-09 12:54 UTC #4 lore raw

PATCH 4/5 tee: amdtee: rename err label to err_device_unregister

In reply to Rijo Thomas

Rename err label to err_device_unregister for better readability.

Signed-off-by Rijo Thomas <[email protected]>

---
 drivers/tee/amdtee/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/tee/amdtee/core.c +2 -2

index c657b9728eae..45402844b669 100644
@@ -471,7 +471,7 @@ static int __init amdtee_driver_init(void)
 
 	rc = tee_device_register(amdtee->teedev);
 	if (rc)
-		goto err;
+		goto err_device_unregister;
 
 	amdtee->pool = pool;

@@ -480,7 +480,7 @@ static int __init amdtee_driver_init(void)
 	pr_info("amd-tee driver initialization successful\n");
 	return 0;
 
-err:
+err_device_unregister:
 	tee_device_unregister(amdtee->teedev);
 
 err_free_pool:

Signature · 1 line

2.17.1

Rijo Thomas

author

2020-01-09 12:54 UTC #5 lore raw

PATCH 5/5 tee: amdtee: remove redundant NULL check for pool

In reply to Rijo Thomas

Remove NULL check for pool variable, since in the current code path it is guaranteed to be non-NULL.

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Rijo Thomas <[email protected]>

---
 drivers/tee/amdtee/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/tee/amdtee/core.c +1 -2

index 45402844b669..be8937eb5d43 100644
@@ -484,8 +484,7 @@ static int __init amdtee_driver_init(void)
 	tee_device_unregister(amdtee->teedev);
 
 err_free_pool:
-	if (pool)
-		tee_shm_pool_free(pool);
+	tee_shm_pool_free(pool);
 
 err_kfree_amdtee:
 	kfree(amdtee);

Signature · 1 line

2.17.1

Dan Carpenter

2020-01-09 16:15 UTC #6 lore raw

In reply to Rijo Thomas

Thanks!

regards,
dan carpenter

Herbert Xu

maintainer

2020-01-16 07:29 UTC #7 lore raw

In reply to Rijo Thomas

Quoted 18 lines · Rijo Thomas

This patch series addresses the bug report submitted by Dan Carpenter.
Link https://lists.linaro.org/pipermail/tee-dev/2020-January/001417.html
Since, these patches are based on cryptodev-2.6 tree, I have included linux-crypto list as well.
This patch series does not fix the static checker warning reported due to incorrect use of IS_ERR. Colin Ian King has submitted a fix for this issue. Link: https://lkml.org/lkml/2020/1/8/88
Rijo Thomas (5):
tee: amdtee: remove unused variable initialization
tee: amdtee: print error message if tee not present
tee: amdtee: skip tee_device_unregister if tee_device_alloc fails
tee: amdtee: rename err label to err_device_unregister
tee: amdtee: remove redundant NULL check for pool

Quoted 3 lines

 drivers/tee/amdtee/call.c | 14 +++++++-------
 drivers/tee/amdtee/core.c | 32 +++++++++++++++++---------------
 2 files changed, 24 insertions(+), 22 deletions(-)

All applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt