From 38dbf76226b32d5ecd3cd159ff1d9d65a3b3c391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Exp=C3=B3sito?= Date: Mon, 27 May 2024 10:26:11 +0200 Subject: [PATCH] Fix findings from static application security testing (SAST) Resolves: https://issues.redhat.com/browse/RHEL-28774 Resolves: https://issues.redhat.com/browse/RHEL-29915 --- ...dgpu_cs_signal_semaphore-thread-safe.patch | 94 +++++++++++++++++++ libdrm.spec | 7 +- 2 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 0001-amdgpu-Make-amdgpu_cs_signal_semaphore-thread-safe.patch diff --git a/0001-amdgpu-Make-amdgpu_cs_signal_semaphore-thread-safe.patch b/0001-amdgpu-Make-amdgpu_cs_signal_semaphore-thread-safe.patch new file mode 100644 index 0000000..0592b58 --- /dev/null +++ b/0001-amdgpu-Make-amdgpu_cs_signal_semaphore-thread-safe.patch @@ -0,0 +1,94 @@ +From 4df9173595dcc65662516b634f9d10001fd060e2 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Jos=C3=A9=20Exp=C3=B3sito?= +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 +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, +-- +2.45.1 + diff --git a/libdrm.spec b/libdrm.spec index 0059105..20325c5 100644 --- a/libdrm.spec +++ b/libdrm.spec @@ -51,7 +51,7 @@ end} Name: libdrm Summary: Direct Rendering Manager runtime library Version: 2.4.120 -Release: 3%{?dist} +Release: 4%{?dist} License: MIT URL: https://dri.freedesktop.org @@ -85,6 +85,8 @@ BuildRequires: chrpath Patch1001: libdrm-make-dri-perms-okay.patch # remove backwards compat not needed on Fedora Patch1002: libdrm-2.4.0-no-bc.patch +# Fix findings from static application security testing (SAST) +Patch1003: 0001-amdgpu-Make-amdgpu_cs_signal_semaphore-thread-safe.patch %description Direct Rendering Manager runtime library @@ -282,6 +284,9 @@ cp %{SOURCE1} %{buildroot}%{_docdir}/libdrm %endif %changelog +* Mon May 27 2024 José Expósito - 2.4.120-4 +- Fix findings from static application security testing (SAST) + * Thu Jan 25 2024 Fedora Release Engineering - 2.4.120-3 - Rebuilt for https://fedoraproject.org/wiki/Fedora_40_Mass_Rebuild