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

[PATCH v3 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.

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: Cancel delayed work before ACA teardown in
    amdgpu_ras_fini

 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 35 ++++++++++---------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c |  3 +--
 2 files changed, 15 insertions(+), 23 deletions(-)

-- 
2.54.0

[PATCH v3 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 v3 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 v3 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, update remove_aca_handle() to use list_del_init()
instead of list_del(). Standard list_del() leaves node pointers
poisoned, which causes !list_empty() in aca_handle_is_valid() to
evaluate to true for removed handles, leading to a potential
use-after-free during device teardown.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
index c76664af9902..06cb3ad1ce62 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;
@@ -717,7 +714,7 @@ 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);
+	list_del_init(&handle->node);
 	mgr->nr_handles--;
 }
 
-- 
2.54.0

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

[sashiko-bot]

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

New issues:
- [High] Using `list_del_init()` on a locklessly-iterated list causes a potential kernel infinite loop if a handle is removed concurrently with list traversal.

Pre-existing issues:
- [High] A TOCTOU race condition exists between background RAS querying and device teardown, leading to a use-after-free of the ACA error cache and its mutex.
--

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

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

This commit fixes the validation logic in aca_handle_is_valid() by ensuring
the handle is non-NULL, contains a valid mask, and is actively registered
in the list. It also updates amdgpu_aca_get_error_data() to return 0 for
invalid handles, and modifies remove_aca_handle() to use list_del_init()
to prevent potential use-after-free issues.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index c76664af9902e..06cb3ad1ce623 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
[ ... ]
> @@ -717,7 +714,7 @@ 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);
> +	list_del_init(&handle->node);

[Severity: High]
This is a pre-existing issue, but does the ordering in this function create
a race condition with the background RAS query thread?

If the background thread evaluates aca_handle_is_valid() concurrently
during teardown, it might see a valid node because the background work is
cancelled after the handles are torn down:

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:amdgpu_ras_fini() {
	...
	amdgpu_aca_fini(adev);
	...
	cancel_delayed_work_sync(&con->ras_counte_delay_work);
	...
}

Because aca_fini_error_cache() is called before the node is removed from
the list, could the background thread see a valid node and then attempt to
acquire a destroyed mutex here?

drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:aca_log_aca_error() {
	...
	mutex_lock(&aerr->lock);
	...
}

[Severity: High]
Does switching to list_del_init() here create a risk of an infinite loop
during list traversal?

The handle list appears to be iterated locklessly here:

drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c:aca_dispatch_bank() {
	...
	list_for_each_entry(handle, &mgr->list, node) {
	...
}

If a handle is removed concurrently with list traversal, list_del_init()
will set the node's next pointer to itself. Will this cause the lockless
iterator in aca_dispatch_bank() to get stuck repeatedly visiting the same
removed node?

>  	mgr->nr_handles--;
>  }

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

[PATCH v3 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 06cb3ad1ce62..79326389d75c 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 v3 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 79326389d75c..4305092f134b 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 v3 6/6] drm/amdgpu/ras: Cancel delayed work before ACA teardown in amdgpu_ras_fini

[Sreeraj S Kurup]

In amdgpu_ras_fini(), cancel_delayed_work_sync() is currently called
after ACA subsystems and locks are destroyed. If the delayed work
(ras_counte_delay_work) runs concurrently during teardown, it can
evaluate aca_handle_is_valid() right before remove_aca_handle()
frees the handle and destroys its mutex, resulting in a use-after-free
and locking violation.

Move cancel_delayed_work_sync() to the beginning of amdgpu_ras_fini()
to ensure all pending work is flushed and stopped before resource
deallocation starts.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 764cd4950408..118716ad0437 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -4751,6 +4751,7 @@ 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 v3 6/6] drm/amdgpu/ras: Cancel delayed work before ACA teardown in amdgpu_ras_fini

[sashiko-bot]

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

Pre-existing issues:
- [High] Race condition and Locking Violation / Use-After-Free in `amdgpu_ras_recovery_fini` where `page_rsv_lock` is destroyed before `page_retirement_dwork` is reliably canceled.
--

--- Patch [6]: [PATCH v3 6/6] drm/amdgpu/ras: Cancel delayed work before ACA teardown in amdgpu_ras_fini ---
commit dd2776768892778a29776f233fd342f5f59be385
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

drm/amdgpu/ras: Cancel delayed work before ACA teardown in amdgpu_ras_fini
    
In amdgpu_ras_fini(), cancel_delayed_work_sync() is currently called
after ACA subsystems and locks are destroyed. This patch moves it to the
beginning to ensure all pending work is flushed and stopped before resource
deallocation starts, preventing a use-after-free and locking violation.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 764cd49504083..118716ad04379 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -4751,6 +4751,7 @@ 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);

[Severity: High]
This is a pre-existing issue, but while looking at this fix for early delayed
work cancellation, I noticed a very similar pattern in
amdgpu_ras_recovery_fini():

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:amdgpu_ras_recovery_fini() {
    ...
	do {
		flush_delayed_work(&con->page_retirement_dwork);
		ret = amdgpu_ras_schedule_retirement_dwork(con, 0);
	} while (ret && max_flush_timeout--);

	...

	mutex_destroy(&con->page_rsv_lock);

	cancel_work_sync(&con->recovery_work);

	cancel_delayed_work_sync(&con->page_retirement_dwork);
    ...
}

If the loop exhausts its max_flush_timeout, it exits while the delayed work
is still pending. The code then proceeds to call
mutex_destroy(&con->page_rsv_lock) before calling
cancel_delayed_work_sync(&con->page_retirement_dwork).

If the delayed work (page_retirement_dwork) runs after the page_rsv_lock
mutex is destroyed, could it attempt to acquire the destroyed mutex in
amdgpu_ras_reserve_page(), leading to a locking violation and
use-after-free?

Should cancel_delayed_work_sync(&con->page_retirement_dwork) be moved above
mutex_destroy(&con->page_rsv_lock) here as well?

[ ... ]

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