diff mbox series

[1/1] tee: amdtee: fix race condition in amdtee_open_session

Message ID 5bf4f1d679e2f80814d07937309602877afd02c6.1677577036.git.Rijo-john.Thomas@amd.com
State New
Headers
Series [1/1] tee: amdtee: fix race condition in amdtee_open_session |

Commit Message

Rijo Thomas Feb. 28, 2023, 9:41 a.m. UTC 
  There is a potential race condition in amdtee_open_session that may
lead to use-after-free. For instance, in amdtee_open_session() after
sess->sess_mask is set, and before setting:

    sess->session_info[i] = session_info;

if amdtee_close_session() closes this same session, then 'sess' data
structure will be released, causing kernel panic when 'sess' is
accessed within amdtee_open_session().

The solution is to set the bit sess->sess_mask as the last step in
amdtee_open_session().

Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
Cc: stable@vger.kernel.org
Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
---
 drivers/tee/amdtee/core.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Comments

Sumit Garg Feb. 28, 2023, 11:07 a.m. UTC | #1 
On Tue, 28 Feb 2023 at 15:11, Rijo Thomas <Rijo-john.Thomas@amd.com> wrote:
>
> There is a potential race condition in amdtee_open_session that may
> lead to use-after-free. For instance, in amdtee_open_session() after
> sess->sess_mask is set, and before setting:
>
>     sess->session_info[i] = session_info;
>
> if amdtee_close_session() closes this same session, then 'sess' data
> structure will be released, causing kernel panic when 'sess' is
> accessed within amdtee_open_session().
>
> The solution is to set the bit sess->sess_mask as the last step in
> amdtee_open_session().
>
> Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
> ---
>  drivers/tee/amdtee/core.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
>

Looks sane to me, FWIW:

Acked-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
> index 297dc62bca29..372d64756ed6 100644
> --- a/drivers/tee/amdtee/core.c
> +++ b/drivers/tee/amdtee/core.c
> @@ -267,35 +267,34 @@ int amdtee_open_session(struct tee_context *ctx,
>                 goto out;
>         }
>
> +       /* Open session with loaded TA */
> +       handle_open_session(arg, &session_info, param);
> +       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);
> +               goto out;
> +       }
> +
>         /* Find an empty session index for the given TA */
>         spin_lock(&sess->lock);
>         i = find_first_zero_bit(sess->sess_mask, TEE_NUM_SESSIONS);
> -       if (i < TEE_NUM_SESSIONS)
> +       if (i < TEE_NUM_SESSIONS) {
> +               sess->session_info[i] = session_info;
> +               set_session_id(ta_handle, i, &arg->session);
>                 set_bit(i, sess->sess_mask);
> +       }
>         spin_unlock(&sess->lock);
>
>         if (i >= TEE_NUM_SESSIONS) {
>                 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);
>                 rc = -ENOMEM;
>                 goto out;
>         }
>
> -       /* Open session with loaded TA */
> -       handle_open_session(arg, &session_info, param);
> -       if (arg->ret != TEEC_SUCCESS) {
> -               pr_err("open_session failed %d\n", arg->ret);
> -               spin_lock(&sess->lock);
> -               clear_bit(i, sess->sess_mask);
> -               spin_unlock(&sess->lock);
> -               handle_unload_ta(ta_handle);
> -               kref_put(&sess->refcount, destroy_session);
> -               goto out;
> -       }
> -
> -       sess->session_info[i] = session_info;
> -       set_session_id(ta_handle, i, &arg->session);
>  out:
>         free_pages((u64)ta, get_order(ta_size));
>         return rc;
> --
> 2.25.1
>
diff mbox series

Patch

diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
index 297dc62bca29..372d64756ed6 100644
--- a/drivers/tee/amdtee/core.c
+++ b/drivers/tee/amdtee/core.c
@@ -267,35 +267,34 @@  int amdtee_open_session(struct tee_context *ctx,
 		goto out;
 	}
 
+	/* Open session with loaded TA */
+	handle_open_session(arg, &session_info, param);
+	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);
+		goto out;
+	}
+
 	/* Find an empty session index for the given TA */
 	spin_lock(&sess->lock);
 	i = find_first_zero_bit(sess->sess_mask, TEE_NUM_SESSIONS);
-	if (i < TEE_NUM_SESSIONS)
+	if (i < TEE_NUM_SESSIONS) {
+		sess->session_info[i] = session_info;
+		set_session_id(ta_handle, i, &arg->session);
 		set_bit(i, sess->sess_mask);
+	}
 	spin_unlock(&sess->lock);
 
 	if (i >= TEE_NUM_SESSIONS) {
 		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);
 		rc = -ENOMEM;
 		goto out;
 	}
 
-	/* Open session with loaded TA */
-	handle_open_session(arg, &session_info, param);
-	if (arg->ret != TEEC_SUCCESS) {
-		pr_err("open_session failed %d\n", arg->ret);
-		spin_lock(&sess->lock);
-		clear_bit(i, sess->sess_mask);
-		spin_unlock(&sess->lock);
-		handle_unload_ta(ta_handle);
-		kref_put(&sess->refcount, destroy_session);
-		goto out;
-	}
-
-	sess->session_info[i] = session_info;
-	set_session_id(ta_handle, i, &arg->session);
 out:
 	free_pages((u64)ta, get_order(ta_size));
 	return rc;