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

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

[Sreeraj S Kurup]

This patch series addresses race conditions, boundary check bugs, logic
inversions, and teardown ordering in the AMDGPU ACA (Accelerated Compute
Architecture) and RAS driver subsystems.

v4 -> v5:
  - Dropped former Patch 4 (NULL check on banks parameter) and Patch 5
    (snprintf buffer size adjustment) from v5.
  - Patch 3: Dropped invalid kfree(handle) from remove_aca_handle()
    since ACA handles can be embedded in ras_manager. Removed non-existent
    mgr->lock references and retained list_del_init() prior to
    aca_fini_error_cache().
  - Patch 4 (formerly Patch 6): Retained the original work cancellation
    order in amdgpu_ras_fini() to prevent work re-queueing, while moving
    cancellation prior to mutex_destroy(&con->page_rsv_lock) in
    amdgpu_ras_recovery_fini().

v3 -> v4:
  - Patch 3: Moved node unlinking prior to aca_fini_error_cache().
  - Patch 4 (formerly Patch 6): Updated the subject and moved
    cancel_work_sync() and cancel_delayed_work_sync() prior to
    mutex_destroy(&con->page_rsv_lock) in amdgpu_ras_recovery_fini().

v2 -> v3:
  - Patch 3: Updated amdgpu_aca_get_error_data() to return 0 instead
    of -EOPNOTSUPP for invalid handles, allowing global RAS error
    queries to safely pass through non-ACA blocks.

v1 -> v2:
  - Patch 3: Updated remove_aca_handle() to use list_del_init()
    instead of list_del(), ensuring list_empty() correctly evaluates
    removed handles.

Sreeraj S Kurup (4):
  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/ras: Fix delayed work cancellation order during teardown

 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 34 ++++++++++---------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c |  7 +++--
 2 files changed, 17 insertions(+), 24 deletions(-)

-- 
2.54.0

[PATCH v5 1/4] 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 v5 2/4] 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 v5 3/4] drm/amdgpu/aca: Fix inverted validation logic and list cleanup

[Sreeraj S Kurup]

"aca_handle_is_valid()" used inverted list-membership logic. It
returned false when "!list_empty(&handle->node)" was true, even though
a non-empty node indicates that the handle is currently registered. As
a result, registered handles were incorrectly considered invalid.

Fix "aca_handle_is_valid()" to verify that the handle is non-NULL, has a
valid mask, and is currently registered in the handle list.

Update "amdgpu_aca_get_error_data()" to reject invalid handles and
return 0 rather than "-EOPNOTSUPP". This allows global RAS queries to
safely skip non-ACA blocks without treating an unsupported block as an
error.

Additionally, use "list_del_init()" in "remove_aca_handle()" before
"aca_fini_error_cache()". Unlike "list_del()", which poisons the
removed node's pointers, "list_del_init()" reinitializes the node as an
empty list. This ensures that "aca_handle_is_valid()" recognizes the
handle as unregistered before its error cache is destroyed.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 14 ++++++--------
 1 file changed, 6 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..d48f2c4d6c84 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,10 @@ 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--;
+	aca_fini_error_cache(handle);
+
 }
 
 static void remove_aca_sysfs(struct aca_handle *handle)
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent execution of lockless list traversal and `remove_aca_handle` leads to TOCTOU use-after-free or hard infinite loops due to `list_del_init()`.
--

--- Patch [3]: [PATCH v5 3/4] drm/amdgpu/aca: Fix inverted validation logic and list cleanup ---
commit 2db17afc321c2a877e8d6ebfef5f2cce5ce3142f
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

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

This commit fixes aca_handle_is_valid() to properly check for valid handles 
rather than empty nodes. It also changes remove_aca_handle() to use 
list_del_init() before destroying the error cache, ensuring the handle is 
recognized as unregistered before its resources are freed.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index c76664af9902e..d48f2c4d6c845 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
[ ... ]
> @@ -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]
Does this lockless validity check introduce a time-of-check to time-of-use
(TOCTOU) race window?

During driver teardown, amdgpu_ras_fini() calls amdgpu_aca_fini() (which 
eventually destroys handles and their mutexes) before it cancels 
ras_counte_delay_work.

If the worker calls this function concurrently, it could pass this lockless 
validity check right before remove_aca_handle() destroys the error cache and
its lock. This would result in a use-after-free or an attempt to lock a 
destroyed mutex when the worker attempts to use the handle in 
aca_log_aca_error().

>  	if ((type < 0) || (!(BIT(type) & handle->mask)))
>  		return  0;
[ ... ]
> @@ -716,9 +713,10 @@ 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--;
> +	aca_fini_error_cache(handle);
> +
>  }

[Severity: High]
Could the use of list_del_init() here cause an infinite loop for concurrent 
list iterators?

When list_del_init() removes the node, it reinitializes the node by pointing 
its next and prev pointers to itself. 

There are functions like aca_dispatch_bank() that iterate over mgr->list 
locklessly. If a concurrent iterator is on this node exactly when it is 
removed, following the self-referential next pointer will cause the iterator 
to loop on this node infinitely, leading to an RCU stall or hard lockup.

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

[PATCH v5 4/4] drm/amdgpu/ras: Fix delayed work cancellation order during teardown

[Sreeraj S Kurup]

In amdgpu_ras_recovery_fini(), mutex_destroy(&con->page_rsv_lock) was
called prior to cancel_work_sync(&con->recovery_work) and
cancel_delayed_work_sync(&con->page_retirement_dwork).

If pending work ran during teardown, it could attempt to acquire
page_rsv_lock after its destruction, leading to a locking violation and
potential use-after-free.

Fix this by moving cancel_work_sync(&con->recovery_work) and
cancel_delayed_work_sync(&con->page_retirement_dwork) before
mutex_destroy(&con->page_rsv_lock), ensuring both work items are
synchronously canceled while the lock remains intact.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 764cd4950408..1d6b4300d80a 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);
-- 
2.54.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] A TOCTOU race in sysfs/debugfs handlers can lead to a NULL pointer dereference because `con->eh_data` is freed in `amdgpu_ras_recovery_fini()` before the sysfs and debugfs interfaces are removed.
- [High] The delayed work `ras_counte_delay_work` can iterate over the `con->head` list while it is being concurrently modified without locks by `amdgpu_ras_disable_all_features()`.
- [High] `amdgpu_ras_critical_region_init()` is incorrectly called inside the teardown function `amdgpu_ras_recovery_fini()`, rendering the feature non-functional and needlessly allocating memory that is immediately freed.
- [High] The MCA notifier is unregistered at the end of `amdgpu_ras_recovery_fini`, after locks are destroyed and data is freed, allowing use-after-free and locking violations.
--

--- Patch [4]: [PATCH v5 4/4] drm/amdgpu/ras: Fix delayed work cancellation order during teardown ---
commit 1c0d53c82e9e39018aa03ebc14a76ad3fb030607
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

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

In amdgpu_ras_recovery_fini(), mutex_destroy(&con->page_rsv_lock) was
called prior to cancel_work_sync(&con->recovery_work) and
cancel_delayed_work_sync(&con->page_retirement_dwork).

If pending work ran during teardown, it could attempt to acquire
page_rsv_lock after its destruction, leading to a locking violation and
potential use-after-free.

Fix this by moving cancel_work_sync(&con->recovery_work) and
cancel_delayed_work_sync(&con->page_retirement_dwork) before
mutex_destroy(&con->page_rsv_lock), ensuring both work items are
synchronously canceled while the lock remains intact.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 764cd49504083..1d6b4300d80aa 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);

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a NULL
pointer dereference during driver unload?

amdgpu_ras_recovery_fini() sets con->eh_data to NULL and frees it just after
this lock is acquired. However, sysfs interfaces are not removed until
amdgpu_ras_fs_fini() is called later.

If a concurrent read in amdgpu_ras_badpages_read() checks con->eh_data outside
the lock, blocks on recovery_lock, and resumes after the teardown function has
freed it:

amdgpu_ras_badpages_read() {
        ...
        mutex_lock(&con->recovery_lock);
        data = con->eh_data;
        if (start < data->count) {
        ...
}

It will dereference the now-NULL con->eh_data when evaluating
start < data->count.


[Severity: High]
This is a pre-existing issue, but does the unregistration of the MCA notifier
happen too late?

Right after this diff context, amdgpu_ras_recovery_fini() unregisters the MCA
notifier via amdgpu_unregister_bad_pages_mca_notifier(). This happens after
con->page_rsv_lock is destroyed and con->eh_data is freed.

If a hardware Machine Check Exception (MCE) occurs during this window,
amdgpu_bad_page_notifier() executes and calls amdgpu_ras_add_bad_pages(),
which attempts to acquire the already-destroyed con->page_rsv_lock in
amdgpu_ras_reserve_page():

amdgpu_ras_reserve_page() {
        ...
        mutex_lock(&con->page_rsv_lock);
        ...
}


[Severity: High]
This isn't a bug introduced by this patch, but is it intentional that
amdgpu_ras_critical_region_init() is called during teardown?

Just after the diff context in amdgpu_ras_recovery_fini(), we have:

        amdgpu_ras_critical_region_init(adev);
#ifdef CONFIG_X86_MCE_AMD
        amdgpu_unregister_bad_pages_mca_notifier(adev);
#endif

This function allocates memory for a critical region and adds it to
con->critical_region_head. Because it is called during teardown, the critical
region list remains empty during normal operation, causing
amdgpu_ras_check_critical_address() to always return false. When the driver
unloads, the memory is pointlessly allocated and then freed shortly after.


[Severity: High]
This is a pre-existing issue, but could ras_counte_delay_work cause a
use-after-free or list corruption during driver teardown?

During teardown, amdgpu_ras_disable_all_features() locklessly iterates over
con->head using list_for_each_entry_safe() and removes nodes.

However, ras_counte_delay_work is not synchronously canceled until after this
occurs in amdgpu_ras_fini(). If the delayed work executes concurrently, it calls
amdgpu_ras_query_error_count(), which locklessly iterates over con->head:

amdgpu_ras_query_error_count() {
        ...
        list_for_each_entry(obj, &con->head, node) {
        ...
}

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