2020-01-09 13:22:07

by Rijo Thomas

[permalink] [raw]
Subject: [PATCH 0/5] 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


2020-01-09 13:22:28

by Rijo Thomas

[permalink] [raw]
Subject: [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(-)

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

2020-01-09 13:23:23

by Rijo Thomas

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

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

Suggested-by: Dan Carpenter <[email protected]>
Signed-off-by: Rijo Thomas <[email protected]>
---
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

2020-01-09 13:23:47

by Rijo Thomas

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

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(-)

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

2020-01-09 13:23:51

by Rijo Thomas

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

Rename err label to err_device_unregister for better
readability.

Suggested-by: Dan Carpenter <[email protected]>
Signed-off-by: Rijo Thomas <[email protected]>
---
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

2020-01-09 13:25:19

by Rijo Thomas

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

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(-)

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

2020-01-09 19:48:02

by Dan Carpenter

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

Thanks!

regards,
dan carpenter

2020-01-16 07:39:10

by Herbert Xu

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt