[PATCH v4 0/6] drm/amdgpu: Robustness and safety fixes for ACA and RAS drivers

[PATCH v4 0/6] drm/amdgpu: Robustness and safety fixes for ACA and RAS drivers

[Sreeraj S Kurup]

This patch series addresses several race conditions, boundary check
bugs, logic inversions, NULL pointer checks, and buffer size
parameters across the AMDGPU ACA (Accelerated Compute Architecture)
and RAS driver subsystems.

v3 -> v4:
  - Patch 3: Wrapped list_del_init() inside mgr->lock in
    remove_aca_handle() and moved node unlinking prior to
    aca_fini_error_cache(). This prevents infinite loops during
    concurrent list traversal and stops background queries from
    referencing destroyed error cache mutexes during teardown.
  - Patch 6: Updated subject and moved cancel_work_sync() and
    cancel_delayed_work_sync() prior to
    mutex_destroy(&con->page_rsv_lock) in amdgpu_ras_recovery_fini()
    to prevent work executing after lock destruction.

v2 -> v3:
  - Patch 3: Updated amdgpu_aca_get_error_data() to return 0 instead
    of -EOPNOTSUPP for invalid handles, ensuring global RAS error
    queries safely pass through non-ACA blocks without breaking
    telemetry.
  - Patch 6: Moved cancel_delayed_work_sync() in amdgpu_ras_fini()
    prior to ACA subsystem and lock cleanup to avoid teardown races
    and UAF.

v1 -> v2:
  - Patch 3: Updated remove_aca_handle() to use list_del_init()
    instead of list_del(), ensuring list_empty() properly evaluates
    removed handles and avoiding potential UAF during device
    teardown.

Sreeraj S Kurup (6):
  drm/amdgpu/aca: Fix race condition and UAF in error cache logging
  drm/amdgpu/aca: Add upper bounds check in aca_bank_hwip_is_matched
  drm/amdgpu/aca: Fix inverted validation logic and list cleanup
  drm/amdgpu/aca: Add missing NULL check for banks parameter in
    aca_banks_add_bank
  drm/amdgpu/aca: Fix off-by-one buffer size parameter in add_aca_sysfs
  drm/amdgpu/ras: Fix delayed work cancellation order during teardown

 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 41 ++++++++++++-------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 11 +++----
 2 files changed, 24 insertions(+), 28 deletions(-)

-- 
2.54.0

[PATCH v4 1/6] drm/amdgpu/aca: Fix race condition and UAF in error cache logging

[Sreeraj S Kurup]

In aca_error_cache_log_bank_error(), find_bank_error() released
aerr->lock prior to returning bank_error. This created a time-of-check
to time-of-use (TOCTOU) race window where a concurrent caller of
aca_log_aca_error() could acquire aerr->lock and free the bank_error
node via aca_bank_error_remove().

When execution returned to aca_error_cache_log_bank_error(),
incrementing bank_error->count resulted in a Use-After-Free and
potential kernel memory corruption. Additionally, bank_error->count
was updated outside mutex lock protection.

Fix this by acquiring aerr->lock at the start of
aca_error_cache_log_bank_error() and holding it continuously across
lookup, creation, and counter updates, while removing redundant
internal lock acquisitions in helper functions.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
index db7858fe0c3d..d0d473082431 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
@@ -237,10 +237,8 @@ static struct aca_bank_error *new_bank_error(struct aca_error *aerr, struct aca_
 	INIT_LIST_HEAD(&bank_error->node);
 	memcpy(&bank_error->info, info, sizeof(*info));
 
-	mutex_lock(&aerr->lock);
 	list_add_tail(&bank_error->node, &aerr->list);
 	aerr->nr_errors++;
-	mutex_unlock(&aerr->lock);
 
 	return bank_error;
 }
@@ -249,22 +247,16 @@ static struct aca_bank_error *find_bank_error(struct aca_error *aerr, struct aca
 {
 	struct aca_bank_error *bank_error = NULL;
 	struct aca_bank_info *tmp_info;
-	bool found = false;
 
-	mutex_lock(&aerr->lock);
 	list_for_each_entry(bank_error, &aerr->list, node) {
 		tmp_info = &bank_error->info;
 		if (tmp_info->socket_id == info->socket_id &&
 		    tmp_info->die_id == info->die_id) {
-			found = true;
-			goto out_unlock;
+			return bank_error;
 		}
 	}
 
-out_unlock:
-	mutex_unlock(&aerr->lock);
-
-	return found ? bank_error : NULL;
+	return NULL;
 }
 
 static void aca_bank_error_remove(struct aca_error *aerr, struct aca_bank_error *bank_error)
@@ -306,11 +298,15 @@ int aca_error_cache_log_bank_error(struct aca_handle *handle, struct aca_bank_in
 		return 0;
 
 	aerr = &error_cache->errors[type];
+	mutex_lock(&aerr->lock);
 	bank_error = get_bank_error(aerr, info);
-	if (!bank_error)
+	if (!bank_error) {
+		mutex_unlock(&aerr->lock);
 		return -ENOMEM;
+	}
 
 	bank_error->count += count;
+	mutex_unlock(&aerr->lock);
 
 	return 0;
 }
-- 
2.54.0

[PATCH v4 2/6] drm/amdgpu/aca: Add upper bounds check in aca_bank_hwip_is_matched

[Sreeraj S Kurup]

In aca_bank_hwip_is_matched(), the 'type' parameter is used directly
as an array index into aca_hwid_mcatypes[]. The function previously
checked whether 'type' was equal to ACA_HWIP_TYPE_UNKNOW, but did not
validate whether 'type' was less than ACA_HWIP_TYPE_COUNT or negative.

If an invalid or out-of-bounds enum value is passed, an out-of-bounds
memory read occurs on the aca_hwid_mcatypes array.

Fix this by validating that 'type' is strictly greater than
ACA_HWIP_TYPE_UNKNOW and less than ACA_HWIP_TYPE_COUNT before
performing the array lookup.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
index d0d473082431..c76664af9902 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
@@ -138,7 +138,7 @@ static bool aca_bank_hwip_is_matched(struct aca_bank *bank, enum aca_hwip_type t
 	int hwid, mcatype;
 	u64 ipid;
 
-	if (!bank || type == ACA_HWIP_TYPE_UNKNOW)
+	if (!bank || type <= ACA_HWIP_TYPE_UNKNOW || type >= ACA_HWIP_TYPE_COUNT)
 		return false;
 
 	hwip = &aca_hwid_mcatypes[type];
-- 
2.54.0

[PATCH v4 3/6] drm/amdgpu/aca: Fix inverted validation logic and list cleanup

[Sreeraj S Kurup]

aca_handle_is_valid() returned false if !list_empty(&handle->node)
evaluated to true. Because active registered handles have non-empty
nodes in the handle list, valid handles evaluated as invalid.
Consequently, amdgpu_aca_get_error_data() returned -EOPNOTSUPP
whenever aca_handle_is_valid() evaluated to true.

Fix the logic in aca_handle_is_valid() to verify that the handle is
non-NULL, contains a valid mask, and is currently registered in the
list. Update amdgpu_aca_get_error_data() to check for invalid
handles and return 0 instead of -EOPNOTSUPP so non-ACA blocks in
global RAS queries pass through safely without breaking error
telemetry.

Additionally, modify remove_aca_handle() to unlink handle->node
under mgr->lock using list_del_init() prior to calling
aca_fini_error_cache(). Standard list_del() leaves node pointers
poisoned, causing !list_empty() in aca_handle_is_valid() to evaluate
to true for removed handles. Unlinking under lock first prevents
concurrent list traversals from racing on node pointers and stops
background queries from referencing destroyed error cache mutexes during
handle teardown.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
index c76664af9902..c35f6be2ef77 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
@@ -586,10 +586,7 @@ static int __aca_get_error_data(struct amdgpu_device *adev, struct aca_handle *h
 
 static bool aca_handle_is_valid(struct aca_handle *handle)
 {
-	if (!handle->mask || !list_empty(&handle->node))
-		return false;
-
-	return true;
+	return handle && handle->mask && !list_empty(&handle->node);
 }
 
 int amdgpu_aca_get_error_data(struct amdgpu_device *adev, struct aca_handle *handle,
@@ -599,8 +596,8 @@ int amdgpu_aca_get_error_data(struct amdgpu_device *adev, struct aca_handle *han
 	if (!handle || !err_data)
 		return -EINVAL;
 
-	if (aca_handle_is_valid(handle))
-		return -EOPNOTSUPP;
+	if (!aca_handle_is_valid(handle))
+		return 0;
 
 	if ((type < 0) || (!(BIT(type) & handle->mask)))
 		return  0;
@@ -716,9 +713,13 @@ static void remove_aca_handle(struct aca_handle *handle)
 {
 	struct aca_handle_manager *mgr = handle->mgr;
 
-	aca_fini_error_cache(handle);
-	list_del(&handle->node);
+	mutex_lock(&mgr->lock);
+	list_del_init(&handle->node);
 	mgr->nr_handles--;
+	mutex_unlock(&mgr->lock);
+	aca_fini_error_cache(handle);
+	kfree(handle);
+
 }
 
 static void remove_aca_sysfs(struct aca_handle *handle)
-- 
2.54.0

[PATCH v4 4/6] drm/amdgpu/aca: Add missing NULL check for banks parameter in aca_banks_add_bank

[Sreeraj S Kurup]

aca_banks_add_bank() verified that the 'bank' parameter was non-NULL,
but passed 'banks' directly into list_add_tail(&node->node, &banks->list)
and incremented 'banks->nr_banks' without validating whether 'banks' was
NULL.

Add a NULL check for 'banks' to prevent a kernel NULL pointer
dereference if an invalid pointer is passed by a caller.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
index c35f6be2ef77..dcddeb381628 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
@@ -49,7 +49,7 @@ static int aca_banks_add_bank(struct aca_banks *banks, struct aca_bank *bank)
 {
 	struct aca_bank_node *node;
 
-	if (!bank)
+	if (!banks || !bank)
 		return -EINVAL;
 
 	node = kvzalloc_obj(*node);
-- 
2.54.0

[PATCH v4 5/6] drm/amdgpu/aca: Fix off-by-one buffer size parameter in add_aca_sysfs

[Sreeraj S Kurup]

snprintf() guarantees NUL-termination within the size limit specified by
its second argument. Passing 'sizeof(handle->attr_name) - 1'
unnecessarily reduces the usable buffer capacity by one byte, causing
sysfs attribute names to be truncated early.

Pass 'sizeof(handle->attr_name)' directly to snprintf() in add_aca_sysfs()
to allow full use of the allocated buffer space.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
index dcddeb381628..3da3d369a918 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
@@ -682,7 +682,7 @@ static int add_aca_sysfs(struct amdgpu_device *adev, struct aca_handle *handle)
 {
 	struct device_attribute *aca_attr = &handle->aca_attr;
 
-	snprintf(handle->attr_name, sizeof(handle->attr_name) - 1, "aca_%s", handle->name);
+	snprintf(handle->attr_name, sizeof(handle->attr_name), "aca_%s", handle->name);
 	aca_attr->show = aca_sysfs_read;
 	aca_attr->attr.name = handle->attr_name;
 	aca_attr->attr.mode = S_IRUGO;
-- 
2.54.0

[PATCH v4 6/6] drm/amdgpu/ras: Fix delayed work cancellation order during teardown

[Sreeraj S Kurup]

In amdgpu_ras_fini(), cancel_delayed_work_sync() is currently called
after ACA subsystems and locks are destroyed. Similarly, in
amdgpu_ras_recovery_fini(), mutex_destroy(&con->page_rsv_lock) is
called before cancel_delayed_work_sync(&con->page_retirement_dwork).

If delayed work runs while teardown is in progress, it can attempt
to acquire destroyed mutexes, leading to locking violations and
potential use-after-free conditions.

Fix this by moving cancel_delayed_work_sync() and cancel_work_sync()
prior to mutex destruction and subsystem teardown in both functions,
ensuring all pending work is synchronously canceled while locks
remain intact.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 764cd4950408..23b9844d9f1c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -3994,15 +3994,14 @@ static int amdgpu_ras_recovery_fini(struct amdgpu_device *adev)
 	if (con->page_retirement_thread)
 		kthread_stop(con->page_retirement_thread);
 
+	cancel_work_sync(&con->recovery_work);
+	cancel_delayed_work_sync(&con->page_retirement_dwork);
+
 	atomic_set(&con->page_retirement_req_cnt, 0);
 	atomic_set(&con->poison_creation_count, 0);
 
 	mutex_destroy(&con->page_rsv_lock);
 
-	cancel_work_sync(&con->recovery_work);
-
-	cancel_delayed_work_sync(&con->page_retirement_dwork);
-
 	amdgpu_ras_ecc_log_fini(&con->umc_ecc_log);
 
 	mutex_lock(&con->recovery_lock);
@@ -4751,6 +4750,8 @@ int amdgpu_ras_fini(struct amdgpu_device *adev)
 	if (!adev->ras_enabled || !con)
 		return 0;
 
+	cancel_delayed_work_sync(&con->ras_counte_delay_work);
+
 	amdgpu_ras_critical_region_fini(adev);
 	mutex_destroy(&con->critical_region_lock);
 
@@ -4785,8 +4786,6 @@ int amdgpu_ras_fini(struct amdgpu_device *adev)
 	if (AMDGPU_RAS_GET_FEATURES(con->features))
 		amdgpu_ras_disable_all_features(adev, 0);
 
-	cancel_delayed_work_sync(&con->ras_counte_delay_work);
-
 	amdgpu_ras_set_context(adev, NULL);
 	kfree(con);
 
-- 
2.54.0

Re: [PATCH v4 3/6] drm/amdgpu/aca: Fix inverted validation logic and list cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Invalid memory free causing SLUB corruption.
- [Low] Build failure due to missing `lock` member in `struct aca_handle_manager`.

Pre-existing issues:
- [High] TOCTOU race in `amdgpu_aca_get_error_data` and lockless list traversal in `aca_dispatch_bank`.
--

--- Patch [3]: [PATCH v4 3/6] drm/amdgpu/aca: Fix inverted validation logic and list cleanup ---
commit 248bb6795a4ccf19c8da116fbb952cdd7829d1fb
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

drm/amdgpu/aca: Fix inverted validation logic and list cleanup

This patch fixes inverted logic in aca_handle_is_valid() so that registered
handles evaluate as valid. It also modifies remove_aca_handle() to unlink 
nodes under a lock and explicitly free the handle.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index c76664af9902e..c35f6be2ef774 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> @@ -586,10 +586,7 @@ static int __aca_get_error_data(struct amdgpu_device *adev, struct aca_handle *h
>  
>  static bool aca_handle_is_valid(struct aca_handle *handle)
>  {
> -	if (!handle->mask || !list_empty(&handle->node))
> -		return false;
> -
> -	return true;
> +	return handle && handle->mask && !list_empty(&handle->node);
>  }
>  
>  int amdgpu_aca_get_error_data(struct amdgpu_device *adev, struct aca_handle *handle,
> @@ -599,8 +596,8 @@ int amdgpu_aca_get_error_data(struct amdgpu_device *adev, struct aca_handle *han
>  	if (!handle || !err_data)
>  		return -EINVAL;
>  
> -	if (aca_handle_is_valid(handle))
> -		return -EOPNOTSUPP;
> +	if (!aca_handle_is_valid(handle))
> +		return 0;

[Severity: High]
This is a pre-existing issue, but does adding the lock in remove_aca_handle()
actually prevent background queries from referencing destroyed mutexes?

The commit message notes that unlinking under a lock stops background queries
from referencing destroyed error cache mutexes. However, since
amdgpu_aca_get_error_data() still calls aca_handle_is_valid() locklessly, a
background thread can evaluate !list_empty(&handle->node) to true and get
preempted. 

Concurrent driver teardown in remove_aca_handle() can then unlink the handle
and destroy its mutexes. When the background thread resumes and enters
__aca_get_error_data(), won't it still lock a destroyed mutex?

Furthermore, aca_dispatch_bank() iterates mgr->list locklessly, which seems
like it will still race with list modifications in remove_aca_handle().

>  
>  	if ((type < 0) || (!(BIT(type) & handle->mask)))
>  		return  0;

[ ... ]

> @@ -716,9 +713,13 @@ static void remove_aca_handle(struct aca_handle *handle)
>  {
>  	struct aca_handle_manager *mgr = handle->mgr;
>  
> -	aca_fini_error_cache(handle);
> -	list_del(&handle->node);
> +	mutex_lock(&mgr->lock);

[Severity: Low]
Does struct aca_handle_manager actually have a lock field? 

Looking at drivers/gpu/drm/amd/amdgpu/amdgpu_aca.h, the struct only contains
a list head and nr_handles. Attempting to lock mgr->lock appears to cause a
build failure.

> +	list_del_init(&handle->node);
>  	mgr->nr_handles--;
> +	mutex_unlock(&mgr->lock);
> +	aca_fini_error_cache(handle);
> +	kfree(handle);
> +

[Severity: High]
Is it safe to call kfree(handle) here? 
    
Looking at amdgpu_ras_bind_aca() in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c,
the handle is passed as an interior pointer to the embedded aca_handle field
of struct ras_manager:

amdgpu_ras_bind_aca() {
    ...
    return amdgpu_aca_add_handle(adev, &obj->aca_handle, ...);
}

Calling kfree() on an embedded struct rather than a standalone heap allocation
will trigger a SLUB invalid-free kernel panic during driver teardown.

>  }
>  
>  static void remove_aca_sysfs(struct aca_handle *handle)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809084735.9743-1-sreekuttan2156239@gmail.com?part=3

Re: [PATCH v4 5/6] drm/amdgpu/aca: Fix off-by-one buffer size parameter in add_aca_sysfs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Invalid free of an interior pointer causing a kernel panic during driver teardown.
--

--- Patch [5]: [PATCH v4 5/6] drm/amdgpu/aca: Fix off-by-one buffer size parameter in add_aca_sysfs ---
commit 4e00dff410196f38991a5223ac5b71f0384ad6fa
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

    drm/amdgpu/aca: Fix off-by-one buffer size parameter in add_aca_sysfs
    
    snprintf() guarantees NUL-termination within the size limit specified by
    its second argument. Passing 'sizeof(handle->attr_name) - 1'
    unnecessarily reduces the usable buffer capacity by one byte, causing
    sysfs attribute names to be truncated early.
    
    Pass 'sizeof(handle->attr_name)' directly to snprintf() in add_aca_sysfs()
    to allow full use of the allocated buffer space.
    
    Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index dcddeb3816285..3da3d369a9187 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> @@ -682,7 +682,7 @@ static int add_aca_sysfs(struct amdgpu_device *adev, struct aca_handle *handle)
>  {
>  	struct device_attribute *aca_attr = &handle->aca_attr;
>  
> -	snprintf(handle->attr_name, sizeof(handle->attr_name) - 1, "aca_%s", handle->name);
> +	snprintf(handle->attr_name, sizeof(handle->attr_name), "aca_%s", handle->name);

[Severity: High]
Is the handle pointer passed here an interior pointer that gets improperly
freed later?

Looking at amdgpu_ras_bind_aca(), it appears an embedded pointer is passed
into the ACA subsystem:

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:amdgpu_ras_bind_aca() {
    ...
    return amdgpu_aca_add_handle(adev, &obj->aca_handle, ras_block_str(blk), aca_info, data);
}

During driver teardown, remove_aca_handle() unconditionally frees this
interior pointer:

drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:remove_aca_handle() {
    ...
    kfree(handle);
}

Could this lead to a SLUB panic when removing the driver module or tearing
down the device?

>  	aca_attr->show = aca_sysfs_read;
>  	aca_attr->attr.name = handle->attr_name;
>  	aca_attr->attr.mode = S_IRUGO;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809084735.9743-1-sreekuttan2156239@gmail.com?part=5

Re: [PATCH v4 4/6] drm/amdgpu/aca: Add missing NULL check for banks parameter in aca_banks_add_bank

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] The added NULL check for the `banks` parameter is dead code and introduces unnecessary branching because the parameter is structurally guaranteed to never be NULL.
- [Low] The `lock` member referenced in `remove_aca_handle()` does not exist in `struct aca_handle_manager`, resulting in a compilation failure.

Pre-existing issues:
- [High] Concurrent traversal and modification of `mgr->list` without holding `mgr->lock` on the reader side causes a severe data race.
--

--- Patch [4]: [PATCH v4 4/6] drm/amdgpu/aca: Add missing NULL check for banks parameter in aca_banks_add_bank ---
commit 613bad660f79f7955828e89d88180e64def84678
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

drm/amdgpu/aca: Add missing NULL check for banks parameter in aca_banks_add_bank
    
aca_banks_add_bank() verified that the 'bank' parameter was non-NULL,
but passed 'banks' directly into list_add_tail(&node->node, &banks->list)
and incremented 'banks->nr_banks' without validating whether 'banks' was
NULL.

Add a NULL check for 'banks' to prevent a kernel NULL pointer
dereference if an invalid pointer is passed by a caller.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index c35f6be2ef774..dcddeb3816285 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> @@ -49,7 +49,7 @@ static int aca_banks_add_bank(struct aca_banks *banks, struct aca_bank *bank)
>  {
>  	struct aca_bank_node *node;
>  
> -	if (!bank)
> +	if (!banks || !bank)

[Severity: Low]
Is this NULL check for banks necessary?

The parameter banks in aca_banks_add_bank() is always the address of a local
stack variable (&banks or &de_banks) in all of its calling paths (e.g., from
aca_banks_update() and aca_banks_generate_cper()), which means it can never
be NULL. Does this introduce unnecessary dead code and branching?

>  		return -EINVAL;
>  
>  	node = kvzalloc_obj(*node);

[ ... ]


There are also a couple of other issues in this area related to the aca
handle list:

[Severity: Low]
Can this code compile successfully?

The lock member referenced in remove_aca_handle() does not exist in
struct aca_handle_manager:

drivers/gpu/drm/amd/amdgpu/amdgpu_aca.h:
struct aca_handle_manager {
	struct list_head list;
	int nr_handles;
};

Attempting to reference mgr->lock here:

drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:remove_aca_handle() {
    ...
	mutex_lock(&mgr->lock);
	list_del_init(&handle->node);
	mgr->nr_handles--;
	mutex_unlock(&mgr->lock);
    ...
}

appears to result in a compilation failure since lock is not defined in the
struct.


[Severity: High]
This isn't a bug introduced by this patch, but there is an explicit attempt
to fix it here. Could this concurrent traversal cause a data race?

While the writer side in remove_aca_handle() attempts to use mgr->lock:

drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:remove_aca_handle() {
    ...
	mutex_lock(&mgr->lock);
	list_del_init(&handle->node);
	mgr->nr_handles--;
	mutex_unlock(&mgr->lock);
    ...
}

The reader side traverses mgr->list without holding any lock or using RCU
primitives:

drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:aca_dispatch_bank() {
    ...
	if (list_empty(&mgr->list))
		return 0;

	list_for_each_entry(handle, &mgr->list, node) {
		if (!aca_bank_is_valid(handle, bank, type))
			continue;
    ...
}

If a background RAS query or sysfs read (which calls aca_dispatch_bank())
traverses mgr->list while remove_aca_handle() concurrently unlinks and frees
a node during teardown, could this lead to a use-after-free or list
corruption and a kernel panic? The fix appears incomplete because it only
syntactically locks the writer side without adding synchronization to the
reader side.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809084735.9743-1-sreekuttan2156239@gmail.com?part=4

Re: [PATCH v4 6/6] drm/amdgpu/ras: Fix delayed work cancellation order during teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Widened race condition allowing delayed work to be re-queued, leading to a Use-After-Free.
--

--- Patch [6]: [PATCH v4 6/6] drm/amdgpu/ras: Fix delayed work cancellation order during teardown ---
commit 0a6b2c827639f23d781a308f6731a9c602763c4a
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

drm/amdgpu/ras: Fix delayed work cancellation order during teardown

This commit reorders the cancellation of delayed work in amdgpu_ras_fini()
and amdgpu_ras_recovery_fini(). It moves cancel_delayed_work_sync() and
cancel_work_sync() before mutex destruction and subsystem teardown to
prevent work handlers from attempting to acquire destroyed locks.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 764cd49504083..23b9844d9f1ca 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
[ ... ]
> @@ -4751,6 +4750,8 @@ int amdgpu_ras_fini(struct amdgpu_device *adev)
>  	if (!adev->ras_enabled || !con)
>  		return 0;
>  
> +	cancel_delayed_work_sync(&con->ras_counte_delay_work);
> +

[Severity: Critical]
This isn't a bug introduced by this patch, but by moving this
cancel_delayed_work_sync() earlier in amdgpu_ras_fini(), does this widen a race
condition that could lead to a Use-After-Free?

If a userspace process calls the AMDGPU_CTX_OP_QUERY_STATE2 ioctl concurrently
with device teardown, amdgpu_ctx_query2() can asynchronously re-queue the work:

drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c:amdgpu_ctx_query2() {
    ...
    schedule_delayed_work(&con->ras_counte_delay_work,
                          msecs_to_jiffies(AMDGPU_RAS_COUNTE_DELAY_MS));
    ...
}

Because the ioctl lacks drm_dev_enter() synchronization, it can re-arm the work
after this cancellation but before the end of amdgpu_ras_fini().

When kfree(con) is called later in amdgpu_ras_fini(), will the re-armed
ras_counte_delay_work access the freed con structure?

>  	amdgpu_ras_critical_region_fini(adev);
>  	mutex_destroy(&con->critical_region_lock);
>  
[ ... ]
> @@ -4785,8 +4786,6 @@ int amdgpu_ras_fini(struct amdgpu_device *adev)
>  	if (AMDGPU_RAS_GET_FEATURES(con->features))
>  		amdgpu_ras_disable_all_features(adev, 0);
>  
> -	cancel_delayed_work_sync(&con->ras_counte_delay_work);
> -
>  	amdgpu_ras_set_context(adev, NULL);
>  	kfree(con);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809084735.9743-1-sreekuttan2156239@gmail.com?part=6