Hi Rijo,
On Fri, 22 Sept 2023 at 12:26, Rijo Thomas <Rijo-john.Thomas@xxxxxxx> wrote:
>
> There is a potential race condition in amdtee_close_session that may
> cause use-after-free in amdtee_open_session. For instance, if a session
> has refcount == 1, and one thread tries to free this session via:
>
> kref_put(&sess->refcount, destroy_session);
>
> the reference count will get decremented, and the next step would be to
> call destroy_session(). However, if in another thread,
> amdtee_open_session() is called before destroy_session() has completed
> execution, alloc_session() may return 'sess' that will be freed up
> later in destroy_session() leading to use-after-free in
> amdtee_open_session.
>
> To fix this issue, treat decrement of sess->refcount and invocation of
> destroy_session() as a single critical section, so that it is executed
> atomically.
>
> Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@xxxxxxx>
> ---
> drivers/tee/amdtee/core.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
> index 372d64756ed6..04cee03bec9d 100644
> --- a/drivers/tee/amdtee/core.c
> +++ b/drivers/tee/amdtee/core.c
> @@ -217,14 +217,13 @@ static int copy_ta_binary(struct tee_context *ctx, void *ptr, void **ta,
> return rc;
> }
>
> +/* mutex must be held by caller */
> static void destroy_session(struct kref *ref)
> {
> struct amdtee_session *sess = container_of(ref, struct amdtee_session,
> refcount);
>
> - mutex_lock(&session_list_mutex);
> list_del(&sess->list_node);
> - mutex_unlock(&session_list_mutex);
> kfree(sess);
> }
>
> @@ -272,7 +271,9 @@ int amdtee_open_session(struct tee_context *ctx,
> if (arg->ret != TEEC_SUCCESS) {
> pr_err("open_session failed %d\n", arg->ret);
> handle_unload_ta(ta_handle);
> + mutex_lock(&session_list_mutex);
> kref_put(&sess->refcount, destroy_session);
How about you rather use kref_put_mutex() here and then keep the
mutex_unlock() within the destroy_session()?
> + mutex_unlock(&session_list_mutex);
> goto out;
> }
>
> @@ -290,7 +291,9 @@ int amdtee_open_session(struct tee_context *ctx,
> pr_err("reached maximum session count %d\n", TEE_NUM_SESSIONS);
> handle_close_session(ta_handle, session_info);
> handle_unload_ta(ta_handle);
> + mutex_lock(&session_list_mutex);
> kref_put(&sess->refcount, destroy_session);
Ditto.
> + mutex_unlock(&session_list_mutex);
> rc = -ENOMEM;
> goto out;
> }
> @@ -331,7 +334,9 @@ int amdtee_close_session(struct tee_context *ctx, u32 session)
> handle_close_session(ta_handle, session_info);
> handle_unload_ta(ta_handle);
>
> + mutex_lock(&session_list_mutex);
> kref_put(&sess->refcount, destroy_session);
Ditto.
-Sumit
> + mutex_unlock(&session_list_mutex);
>
> return 0;
> }
> --
> 2.25.1
>
