From 0d292d11f2158424499ed57da657b7b920131e5c Mon Sep 17 00:00:00 2001 From: Sumukh Hallymysore Ravindra Date: Wed, 8 Sep 2021 13:43:05 +0530 Subject: [PATCH] msm: synx: Prevent accessing valid but released handles Release function removes the reference taken during create, but same release function can be invoked again to take away the valid reference held by other threads, which got these references before release was invoked the first time. This could lead to successive release call/s taking away reference held by other threads leading to UAF. Change adds another member variable to fail release function even though it is still valid, if the rel_count member has reached zero. rel_count variable is set to 1 during synx creation and decremented on release. All functions after release will fail, as per design and avoid taking away the reference of pending functions on same handle. Change-Id: Ie7ab279bc1e08e82c394b704e181716b726bcf4e Signed-off-by: Sumukh Hallymysore Ravindra --- drivers/media/platform/msm/synx/synx.c | 44 ++++++++++++------- .../media/platform/msm/synx/synx_private.h | 4 +- drivers/media/platform/msm/synx/synx_util.c | 4 ++ 3 files changed, 35 insertions(+), 17 deletions(-) diff --git a/drivers/media/platform/msm/synx/synx.c b/drivers/media/platform/msm/synx/synx.c index 4ab19050a39f..8043cc1489ed 100644 --- a/drivers/media/platform/msm/synx/synx.c +++ b/drivers/media/platform/msm/synx/synx.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved. + * Copyright (c) 2019-2021, The Linux Foundation. All rights reserved. */ #define pr_fmt(fmt) "synx: " fmt @@ -681,9 +681,9 @@ EXPORT_SYMBOL(synx_merge); int synx_release(struct synx_session session_id, s32 h_synx) { int rc = 0; + u32 idx; struct synx_client *client; struct synx_handle_coredata *synx_data; - struct synx_coredata *synx_obj; pr_debug("[sess: %u] Enter release from pid %d\n", session_id.client_id, current->pid); @@ -692,21 +692,31 @@ int synx_release(struct synx_session session_id, s32 h_synx) if (!client) return -EINVAL; - synx_data = synx_util_acquire_handle(client, h_synx); - synx_obj = synx_util_obtain_object(synx_data); - /* no need to check for fence here */ - if (!synx_obj) { - pr_err("%s: [sess: %u] invalid handle access %d\n", - __func__, client->id, h_synx); - rc = -EINVAL; - goto fail; + synx_data = NULL; + idx = synx_util_handle_index(h_synx); + + mutex_lock(&client->synx_table_lock[idx]); + synx_data = &client->synx_table[idx]; + if (!synx_data->synx_obj) { + pr_err("[sess: %u] invalid object handle %d\n", + client->id, h_synx); + } else if (synx_data->handle != h_synx) { + pr_err("[sess: %u] stale object handle %d\n", + client->id, h_synx); + } else if (synx_data->rel_count == 0) { + pr_err("[sess: %u] released object handle %d\n", + client->id, h_synx); + } else if (!kref_read(&synx_data->internal_refcount)) { + pr_err("[sess: %u] destroyed object handle %d\n", + client->id, h_synx); + } else { + synx_data->rel_count--; + /* release the reference obtained at synx creation */ + kref_put(&synx_data->internal_refcount, + synx_util_destroy_internal_handle); } + mutex_unlock(&client->synx_table_lock[idx]); - /* release the reference obtained at synx creation */ - synx_util_release_handle(synx_data); - -fail: - synx_util_release_handle(synx_data); synx_put_client(client); pr_debug("[sess: %u] exit release with status %d\n", session_id.client_id, rc); @@ -953,8 +963,10 @@ int synx_addrefcount(struct synx_session session_id, s32 h_synx, s32 count) idx = synx_util_handle_index(h_synx); mutex_lock(&client->synx_table_lock[idx]); /* acquire additional references to handle */ - while (count--) + while (count--) { + synx_data->rel_count++; kref_get(&synx_data->internal_refcount); + } mutex_unlock(&client->synx_table_lock[idx]); fail: diff --git a/drivers/media/platform/msm/synx/synx_private.h b/drivers/media/platform/msm/synx/synx_private.h index e8f5d8e3ba41..9ebc162ee88c 100644 --- a/drivers/media/platform/msm/synx/synx_private.h +++ b/drivers/media/platform/msm/synx/synx_private.h @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* - * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved. + * Copyright (c) 2019-2021, The Linux Foundation. All rights reserved. */ #ifndef __SYNX_PRIVATE_H__ @@ -181,6 +181,7 @@ struct synx_device; * @import_refcount : References for external clients for import * @id : Synx object handle for the client * @key : Key for import authentication + * @rel_count : No of allowed release counts */ struct synx_handle_coredata { struct synx_client *client; @@ -189,6 +190,7 @@ struct synx_handle_coredata { struct kref import_refcount; u32 handle; u16 key; + u32 rel_count; }; /** diff --git a/drivers/media/platform/msm/synx/synx_util.c b/drivers/media/platform/msm/synx/synx_util.c index 3739f81a13d3..737351fadf60 100644 --- a/drivers/media/platform/msm/synx/synx_util.c +++ b/drivers/media/platform/msm/synx/synx_util.c @@ -231,6 +231,7 @@ int synx_util_init_handle(struct synx_client *client, synx_data->handle = h_synx; synx_data->synx_obj = synx_obj; kref_init(&synx_data->internal_refcount); + synx_data->rel_count = 1; mutex_unlock(&client->synx_table_lock[idx]); *new_synx = h_synx; @@ -570,6 +571,9 @@ struct synx_handle_coredata *synx_util_acquire_handle( } else if (synx_data->handle != h_synx) { pr_err("[sess: %u] stale object handle %d\n", client->id, h_synx); + } else if (synx_data->rel_count == 0) { + pr_err("[sess: %u] released object handle %d\n", + client->id, h_synx); } else if (!kref_read(&synx_data->internal_refcount)) { pr_err("[sess: %u] destroyed object handle %d\n", client->id, h_synx);