[PATCH 0/5] amdtee: Address bug report

[PATCH 0/5] amdtee: Address bug report

[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

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

--
2.17.1

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

[Rijo Thomas]

Remove unused variable initialization from driver code.

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

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
---
 drivers/tee/amdtee/call.c | 14 +++++++-------
 drivers/tee/amdtee/core.c | 14 +++++++-------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/tee/amdtee/call.c b/drivers/tee/amdtee/call.c
index 87ccad256686..096dd4d92d39 100644
--- a/drivers/tee/amdtee/call.c
+++ b/drivers/tee/amdtee/call.c
@@ -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;
diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
index 9d0cee1c837f..b3f8235579f7 100644
--- a/drivers/tee/amdtee/core.c
+++ b/drivers/tee/amdtee/core.c
@@ -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();
-- 
2.17.1

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

[Rijo Thomas]

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

Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
---
 drivers/tee/amdtee/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
index b3f8235579f7..0840be03a3ab 100644
--- a/drivers/tee/amdtee/core.c
+++ b/drivers/tee/amdtee/core.c
@@ -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;
 }
-- 
2.17.1

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

[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 <dan.carpenter@oracle.com>
Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
---
 drivers/tee/amdtee/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
index 0840be03a3ab..c657b9728eae 100644
--- a/drivers/tee/amdtee/core.c
+++ b/drivers/tee/amdtee/core.c
@@ -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);
 
-- 
2.17.1

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

[Rijo Thomas]

Rename err label to err_device_unregister for better
readability.

Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
---
 drivers/tee/amdtee/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
index c657b9728eae..45402844b669 100644
--- a/drivers/tee/amdtee/core.c
+++ b/drivers/tee/amdtee/core.c
@@ -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:
-- 
2.17.1

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

[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 <dan.carpenter@oracle.com>
Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
---
 drivers/tee/amdtee/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
index 45402844b669..be8937eb5d43 100644
--- a/drivers/tee/amdtee/core.c
+++ b/drivers/tee/amdtee/core.c
@@ -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);
-- 
2.17.1

Re: [PATCH 0/5] amdtee: Address bug report

[Dan Carpenter]

Thanks!

regards,
dan carpenter

Re: [PATCH 0/5] amdtee: Address bug report

[Herbert Xu]

On Thu, Jan 09, 2020 at 06:23:17PM +0530, Rijo Thomas wrote:
> 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(-)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt