From 4df9173595dcc65662516b634f9d10001fd060e2 Mon Sep 17 00:00:00 2001 From: José Expósito Date: Thu, 21 Mar 2024 11:41:18 +0100 Subject: 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 Signed-off-by: José Expósito --- 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, -- cgit v1.2.3