[PATCH v2 0/5] drm/amdgpu/aca: Robustness and safety fixes for ACA driver

[PATCH v2 0/5] drm/amdgpu/aca: Robustness and safety fixes for ACA driver

[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) driver subsystem.

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 (5):
  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 handle 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

 drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 33 ++++++++++---------------
 1 file changed, 13 insertions(+), 20 deletions(-)

-- 
2.54.0

[PATCH v2 1/5] 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 v2 2/5] 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 v2 3/5] drm/amdgpu/aca: Fix inverted validation logic and handle 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. Negate the check in amdgpu_aca_get_error_data() so valid
handles are processed properly.

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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
index c76664af9902..b4c1438a56d8 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,7 +596,7 @@ 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))
+	if (!aca_handle_is_valid(handle))
 		return -EOPNOTSUPP;
 
 	if ((type < 0) || (!(BIT(type) & handle->mask)))
@@ -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 v2 3/5] drm/amdgpu/aca: Fix inverted validation logic and handle cleanup

[sashiko-bot]

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

New issues:
- [High] The logic change in `amdgpu_aca_get_error_data()` causes global RAS error count queries to fail with `-EOPNOTSUPP` for non-ACA blocks.

Pre-existing issues:
- [High] Delayed work `ras_counte_delay_work` is cancelled after ACA subsystems are torn down, leading to a TOCTOU race condition and use-after-destroy of a mutex.
--

--- Patch [3]: [PATCH v2 3/5] drm/amdgpu/aca: Fix inverted validation logic and handle cleanup ---
commit e884a156d93ba3896781b055b3f77054754fcf85
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

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

This commit fixes the validation logic in aca_handle_is_valid() and its
caller amdgpu_aca_get_error_data() to properly process valid handles. It
also changes remove_aca_handle() to use list_del_init() to prevent a
potential use-after-free during device teardown.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> index c76664af9902e..b4c1438a56d8c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
[ ... ]
> @@ -599,7 +596,7 @@ 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))
> +	if (!aca_handle_is_valid(handle))
>  		return -EOPNOTSUPP;
[Severity: High]
Does this logic change cause global RAS error count queries to fail for
non-ACA blocks?

Before this patch, zero-initialized handles (where handle->mask is 0) would
cause aca_handle_is_valid() to return false. Since the check here wasn't
negated, the code would bypass the -EOPNOTSUPP return and fall through to:

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

which gracefully returned 0.

With the new negated check, zero-initialized handles for non-ACA blocks (like
HDP) evaluate as invalid and immediately return -EOPNOTSUPP.

Could this overwrite the return value in the loop within
amdgpu_ras_query_error_count() when iterating over RAS blocks?

amdgpu_ras.c:amdgpu_ras_query_error_count() {
    ...
    list_for_each_entry(obj, &con->head, node) {
        ...
        ret = amdgpu_ras_query_error_count_helper(adev, &ce, &ue, &info);
    }
}

If a non-ACA block is queried at the end of the loop, this -EOPNOTSUPP could
propagate up, completely breaking global RAS telemetry and preventing cached
counters from updating.

>  
>  	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);
[Severity: High]
This is a pre-existing issue, but does this still leave a time-of-check to
time-of-use race condition during device teardown?

Looking at amdgpu_ras_fini(), the delayed work is cancelled after the ACA
subsystems are torn down:

amdgpu_ras.c:amdgpu_ras_fini() {
    ...
    amdgpu_aca_fini();
    ...
    cancel_delayed_work_sync(&con->ras_counte_delay_work);
}

If the delayed work runs concurrently, could it perform the lockless
!list_empty(&handle->node) check in aca_handle_is_valid() and evaluate to
true just before this teardown thread calls remove_aca_handle()?

If remove_aca_handle() then unlinks the node and destroys the handle's mutex,
the worker thread might proceed into __aca_get_error_data() and attempt to
lock a destroyed mutex.

>  	mgr->nr_handles--;
>  }
>

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

[PATCH v2 4/5] 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 b4c1438a56d8..f22b58c559b6 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 v2 5/5] 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 f22b58c559b6..c2fe98b83eee 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