38dbf76226
Resolves: https://issues.redhat.com/browse/RHEL-28774 Resolves: https://issues.redhat.com/browse/RHEL-29915
95 lines
3.7 KiB
Diff
95 lines
3.7 KiB
Diff
From 4df9173595dcc65662516b634f9d10001fd060e2 Mon Sep 17 00:00:00 2001
|
|
From: =?UTF-8?q?Jos=C3=A9=20Exp=C3=B3sito?= <jexposit@redhat.com>
|
|
Date: Thu, 21 Mar 2024 11:41:18 +0100
|
|
Subject: [PATCH] amdgpu: Make amdgpu_cs_signal_semaphore() thread-safe
|
|
MIME-Version: 1.0
|
|
Content-Type: text/plain; charset=UTF-8
|
|
Content-Transfer-Encoding: 8bit
|
|
|
|
The issue was found by a static analysis tool:
|
|
|
|
Error: LOCK_EVASION (CWE-543):
|
|
libdrm-2.4.115/amdgpu/amdgpu_cs.c:596: thread1_checks_field:
|
|
Thread1 uses the value read from field "context" in the
|
|
condition "sem->signal_fence.context". It sees that the
|
|
condition is false. Control is switched to Thread2.
|
|
libdrm-2.4.115/amdgpu/amdgpu_cs.c:596: thread2_checks_field:
|
|
Thread2 uses the value read from field "context" in the
|
|
condition "sem->signal_fence.context". It sees that the
|
|
condition is false.
|
|
libdrm-2.4.115/amdgpu/amdgpu_cs.c:598: thread2_acquires_lock:
|
|
Thread2 acquires lock "amdgpu_context.sequence_mutex".
|
|
libdrm-2.4.115/amdgpu/amdgpu_cs.c:599: thread2_modifies_field:
|
|
Thread2 sets "context" to a new value. Note that this write can
|
|
be reordered at runtime to occur before instructions that do
|
|
not access this field within this locked region. After Thread2
|
|
leaves the critical section, control is switched back to
|
|
Thread1.
|
|
libdrm-2.4.115/amdgpu/amdgpu_cs.c:598: thread1_acquires_lock:
|
|
Thread1 acquires lock "amdgpu_context.sequence_mutex".
|
|
libdrm-2.4.115/amdgpu/amdgpu_cs.c:599: thread1_overwrites_value_in_field:
|
|
Thread1 sets "context" to a new value. Now the two threads have
|
|
an inconsistent view of "context" and updates to fields of
|
|
"context" or fields correlated with "context" may be lost.
|
|
libdrm-2.4.115/amdgpu/amdgpu_cs.c:596: use_same_locks_for_read_and_modify:
|
|
Guard the modification of "context" and the read used to decide
|
|
whether to modify "context" with the same set of locks.
|
|
# 597| return -EINVAL;
|
|
# 598| pthread_mutex_lock(&ctx->sequence_mutex);
|
|
# 599|-> sem->signal_fence.context = ctx;
|
|
# 600| sem->signal_fence.ip_type = ip_type;
|
|
# 601| sem->signal_fence.ip_instance = ip_instance;
|
|
|
|
Check `sem->signal_fence.context` in the locked region to avoid a race
|
|
condition.
|
|
|
|
Reviewed-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
|
|
Signed-off-by: José Expósito <jexposit@redhat.com>
|
|
---
|
|
amdgpu/amdgpu_cs.c | 15 +++++++++++----
|
|
1 file changed, 11 insertions(+), 4 deletions(-)
|
|
|
|
diff --git a/amdgpu/amdgpu_cs.c b/amdgpu/amdgpu_cs.c
|
|
index 49fc16c3..2db49675 100644
|
|
--- a/amdgpu/amdgpu_cs.c
|
|
+++ b/amdgpu/amdgpu_cs.c
|
|
@@ -598,24 +598,31 @@ drm_public int amdgpu_cs_signal_semaphore(amdgpu_context_handle ctx,
|
|
uint32_t ring,
|
|
amdgpu_semaphore_handle sem)
|
|
{
|
|
+ int ret;
|
|
+
|
|
if (!ctx || !sem)
|
|
return -EINVAL;
|
|
if (ip_type >= AMDGPU_HW_IP_NUM)
|
|
return -EINVAL;
|
|
if (ring >= AMDGPU_CS_MAX_RINGS)
|
|
return -EINVAL;
|
|
- /* sem has been signaled */
|
|
- if (sem->signal_fence.context)
|
|
- return -EINVAL;
|
|
+
|
|
pthread_mutex_lock(&ctx->sequence_mutex);
|
|
+ /* sem has been signaled */
|
|
+ if (sem->signal_fence.context) {
|
|
+ ret = -EINVAL;
|
|
+ goto unlock;
|
|
+ }
|
|
sem->signal_fence.context = ctx;
|
|
sem->signal_fence.ip_type = ip_type;
|
|
sem->signal_fence.ip_instance = ip_instance;
|
|
sem->signal_fence.ring = ring;
|
|
sem->signal_fence.fence = ctx->last_seq[ip_type][ip_instance][ring];
|
|
update_references(NULL, &sem->refcount);
|
|
+ ret = 0;
|
|
+unlock:
|
|
pthread_mutex_unlock(&ctx->sequence_mutex);
|
|
- return 0;
|
|
+ return ret;
|
|
}
|
|
|
|
drm_public int amdgpu_cs_wait_semaphore(amdgpu_context_handle ctx,
|
|
--
|
|
2.45.1
|
|
|