On Fri, 29 Sept 2023 at 12:30, 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 removal of
> 'sess' from session list in destroy_session() as a 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>
> ---
> v2:
> * Introduced kref_put_mutex() as suggested by Sumit Garg.
>
> drivers/tee/amdtee/core.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
Reviewed-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
-Sumit
> diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
> index 372d64756ed6..3c15f6a9e91c 100644
> --- a/drivers/tee/amdtee/core.c
> +++ b/drivers/tee/amdtee/core.c
> @@ -217,12 +217,12 @@ 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 +272,8 @@ 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);
> - kref_put(&sess->refcount, destroy_session);
> + kref_put_mutex(&sess->refcount, destroy_session,
> + &session_list_mutex);
> goto out;
> }
>
> @@ -290,7 +291,8 @@ 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);
> - kref_put(&sess->refcount, destroy_session);
> + kref_put_mutex(&sess->refcount, destroy_session,
> + &session_list_mutex);
> rc = -ENOMEM;
> goto out;
> }
> @@ -331,7 +333,7 @@ int amdtee_close_session(struct tee_context *ctx, u32 session)
> handle_close_session(ta_handle, session_info);
> handle_unload_ta(ta_handle);
>
> - kref_put(&sess->refcount, destroy_session);
> + kref_put_mutex(&sess->refcount, destroy_session, &session_list_mutex);
>
> return 0;
> }
> --
> 2.25.1
>
