Re: [PATCH 1/2] accel/amdxdna: check the command chain payload before using it

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Missing READ_ONCE() when reading `cc->command_count` from a user-space mapped buffer introduces a Time-of-Check to Time-of-Use (TOCTOU) vulnerability.

Pre-existing issues:
- [High] Reference leak for the chained GEM object `abo` in `amdxdna_cmd_set_error()` when `amdxdna_gem_vmap()` fails.
- [High] Unprotected access to user-mapped flexible array `cc->data[0]` causes data races and triggers UBSAN panics via `__counted_by`.
--

commit a1b05572fd8c69d4959c5969b2a34b2e5ce07591
Author: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
Subject: accel/amdxdna: check the command chain payload before using it

This commit introduces a new function amdxdna_cmd_get_chain() to ensure the
chain payload of a BO is checked before use. It makes the size parameter
mandatory in amdxdna_cmd_get_payload() and updates amdxdna_cmd_set_error()
to utilize the new chain validation logic.

> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 855da8c79a1cd..143cbbbf4b310 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c

> [ ... ]

> @@ -125,20 +125,42 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)

> [ ... ]

> +/*
> + * Returns the chain payload of @abo, with @count set to a command count that
> + * has been checked to fit. The chain fields live in a BO user space keeps
> + * mapped, so nothing may read them without going through here.
> + */
> +struct amdxdna_cmd_chain *
> +amdxdna_cmd_get_chain(struct amdxdna_gem_obj *abo, u32 *count)
> +{
> +	struct amdxdna_cmd_chain *cc;
> +	u32 len, ccnt;
> +
> +	cc = amdxdna_cmd_get_payload(abo, &len);
> +	if (!cc || len < sizeof(*cc))
> +		return NULL;
> +
> +	ccnt = cc->command_count;
> +	if (len < struct_size(cc, data, ccnt))
> +		return NULL;
> +
> +	*count = ccnt;

[Severity: Medium]
Since the memory for cc resides in a GEM object mapped to userspace, can the
compiler perform multiple memory loads of cc->command_count here because it
lacks a READ_ONCE() wrapper?

If a user modifies cc->command_count concurrently between the struct_size()
bounds check and the assignment to *count, could this bypass the newly added
bounds check and pass a larger ccnt to the caller?

> +	return cc;
> +}

> [ ... ]

> @@ -177,8 +199,13 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>  	cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state);
>  
>  	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
> -		cc = amdxdna_cmd_get_payload(abo, NULL);
> -		cc->error_index = (cmd_idx < cc->command_count) ? cmd_idx : 0;
> +		u32 ccnt;
> +
> +		cc = amdxdna_cmd_get_chain(abo, &ccnt);
> +		if (!cc || !ccnt)
> +			return -EINVAL;
> +
> +		cc->error_index = (cmd_idx < ccnt) ? cmd_idx : 0;
>  		abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);

[Severity: High]
This is a pre-existing issue, but since struct amdxdna_cmd_chain defines data
as a flexible array u64 data[] __counted_by(command_count), does accessing
cc->data[0] dynamically re-read cc->command_count from memory to assert
0 < command_count?

If cc is backed by user-mapped memory, couldn't a malicious user concurrently
write 0 to command_count, causing the dynamically validated access to fail
and trigger a UBSAN kernel panic if CONFIG_UBSAN_BOUNDS is enabled?

>  		if (!abo)
>  			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this function leak the reference to
the chained GEM object abo if amdxdna_gem_vmap() fails?

Looking at amdxdna_cmd_set_error(), it successfully looks up the chained BO
using amdxdna_gem_get_obj(), which increments its reference count:

	abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);
	if (!abo)
		return -EINVAL;
	cmd = amdxdna_gem_vmap(abo);
	if (!cmd)
		return -ENOMEM;

If amdxdna_gem_vmap() fails, the function returns -ENOMEM without calling
amdxdna_gem_put_obj(abo). Can an unprivileged user trigger this by causing
job failures under memory pressure or by crafting a BO that fails to map,
leading to a memory leak and eventual Denial of Service?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826143036.100089-1-taimuraz@kaitmazov.com?part=1