Re: [PATCH v15 4/7] qcom-tgu: Add TGU decode support

[sashiko-bot@kernel.org]

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

New issues:
- [High] The local variable `index` is used uninitialized in `tgu_write_all_hw_regs()`, leading to unpredictable return values.

Pre-existing issues:
- [High] Probe error paths fail to remove manually created sysfs groups, creating a Use-After-Free vulnerability.
- [High] Sysfs attributes are exposed to userspace before their internal data structures are allocated, exposing a window for a NULL pointer dereference.
- [High] TOCTOU race conditions in `enable_tgu_store` and `reset_tgu_store` allow concurrent executions to permanently leak PM reference counts.
--

commit 02dfd241f8e959109566cea8502a03ef77cfcf4f
Author: Songwei Chai <songwei.chai@oss.qualcomm.com>

qcom-tgu: Add TGU decode support

This patch adds condition decode support to the Qualcomm Trace and Gating Unit
(TGU). It introduces registers to establish conditions for triggers or step
movements.

> diff --git a/drivers/hwtracing/qcom/tgu.c b/drivers/hwtracing/qcom/tgu.c
> index 7d69986c3e3d2..937211923d937 100644
> --- a/drivers/hwtracing/qcom/tgu.c
> +++ b/drivers/hwtracing/qcom/tgu.c

[ ... ]

> @@ -91,8 +161,10 @@ static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
>  	for (i = 0; i < drvdata->num_step; i++) {
>  		for (j = 0; j < MAX_PRIORITY; j++) {
>  			for (k = 0; k < drvdata->num_reg; k++) {
> -				index = calculate_array_location(
> +				index = check_array_location(
>  							drvdata, i, j, k);
> +				if (index == -EINVAL)
> +					goto exit;
>  
>  				writel(drvdata->value_table->priority[index],
>  					drvdata->base +
> @@ -100,9 +172,23 @@ static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
>  			}
>  		}
>  	}
> +
> +	for (i = 0; i < drvdata->num_step; i++) {
> +		for (j = 0; j < drvdata->num_condition_decode; j++) {
> +			index = check_array_location(drvdata, i,
> +						TGU_CONDITION_DECODE, j);
> +			if (index == -EINVAL)
> +				goto exit;
> +
> +			writel(drvdata->value_table->condition_decode[index],
> +				drvdata->base + CONDITION_DECODE_STEP(i, j));
> +		}
> +	}
>  	/* Enable TGU to program the triggers */
>  	writel(1, drvdata->base + TGU_CONTROL);
> +exit:
>  	TGU_LOCK(drvdata->base);
> +	return index >= 0 ? 0 : -EINVAL;

[Severity: High]
Could index be used uninitialized here?

If the hardware reports 0 for drvdata->num_step (e.g., via TGU_DEVID_STEPS),
the nested loops in tgu_write_all_hw_regs() are bypassed entirely.

The function then evaluates index >= 0 ? 0 : -EINVAL using the uninitialized
stack value of index, leading to unpredictable initialization failures or
spurious successes.

>  }

[ ... ]

> @@ -131,16 +217,26 @@ static void tgu_set_steps(struct tgu_drvdata *drvdata)
>  	drvdata->num_step = TGU_DEVID_STEPS(devid);
>  }
>  
> +static void tgu_set_conditions(struct tgu_drvdata *drvdata)
> +{
> +	u32 devid;
> +
> +	devid = readl(drvdata->base + TGU_DEVID);
> +	drvdata->num_condition_decode = TGU_DEVID_CONDITIONS(devid);
> +}
> +
>  static int tgu_enable(struct device *dev)
>  {
>  	struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
> +	int ret;
>  
>  	guard(spinlock)(&drvdata->lock);
> -	drvdata->enabled = true;
>  
> -	tgu_write_all_hw_regs(drvdata);
> +	ret = tgu_write_all_hw_regs(drvdata);
> +	if (!ret)
> +		drvdata->enabled = true;

[Severity: High]
This isn't a bug introduced by this patch, but is there a TOCTOU race
condition in enable_tgu_store() and reset_tgu_store() that allows
concurrent executions to permanently leak PM reference counts?

enable_tgu_store() checks drvdata->enabled under a spinlock, drops the lock,
and acquires a PM reference via pm_runtime_resume_and_get() before calling
tgu_enable().

Two concurrent writers can both bypass the check and acquire references, but
a subsequent disable will only drop one reference, preventing the device and
its hierarchy from entering low-power states.

>  
> -	return 0;
> +	return ret;
>  }

[ ... ]

> @@ -288,6 +392,7 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	tgu_set_reg_number(drvdata);
>  	tgu_set_steps(drvdata);
> +	tgu_set_conditions(drvdata);
>  
>  	ret = sysfs_create_groups(&dev->kobj, tgu_attr_groups);

[Severity: High]
This is a pre-existing issue, but does this early sysfs creation expose a
window for a NULL pointer dereference?

In tgu_probe(), sysfs_create_groups() exposes the device attributes to user
space before drvdata->value_table and its nested arrays are allocated.

If a user process accesses the sysfs files immediately, the sysfs handlers
(like tgu_dataset_show) will dereference a NULL value_table pointer, causing
a kernel panic. Any unprivileged user or udev rule can read the 0644 sysfs
files during this window.

>  	if (ret) {
> @@ -310,6 +415,16 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	drvdata->value_table->priority = priority;
>  
> +	condition_size = drvdata->num_condition_decode * drvdata->num_step;
> +
> +	condition = devm_kcalloc(dev, condition_size,
> +				sizeof(*(drvdata->value_table->condition_decode)),
> +				GFP_KERNEL);
> +	if (!condition)
> +		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this error path fail
to remove manually created sysfs groups, creating a Use-After-Free
vulnerability?

If memory allocations fail after sysfs_create_groups() is called in
tgu_probe(), the function returns -ENOMEM without calling
sysfs_remove_groups().

The driver core then frees the devm-managed drvdata, leaving dangling sysfs
files on the device's kobject. Any local unprivileged user can read the
0644 sysfs files, triggering a UAF on the freed drvdata pointer.

> +
> +	drvdata->value_table->condition_decode = condition;
> +
>  	drvdata->enabled = false;
>  
>  	pm_runtime_put(&adev->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701055529.445819-1-songwei.chai@oss.qualcomm.com?part=4