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

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 on the return path of `tgu_write_all_hw_regs()`, causing undefined behavior.

Pre-existing issues:
- [High] Sysfs attributes are exposed to userspace before their underlying data structures are allocated.
- [High] Probe error paths fail to remove manually created sysfs groups, leading to Use-After-Free/NULL pointer dereferences.
- [High] The driver mismanages runtime PM usage counters, leading to underflows and permanent leaks.
--

--- Patch [4]: [PATCH v12 4/7] qcom-tgu: Add TGU decode support ---
commit 9ed009439c45349e788fcf926637cdd6928ba5be
Author: Songwei Chai <[email protected]>

qcom-tgu: Add TGU decode support

Decoding is when all the potential pieces for creating a trigger
are brought together for a given step. This patch programs the condition
decode inputs for TGU steps.

> diff --git a/drivers/hwtracing/qcom/tgu.c b/drivers/hwtracing/qcom/tgu.c
> index 14d6ad410d89f..85fe1140eab58 100644
> --- a/drivers/hwtracing/qcom/tgu.c
> +++ b/drivers/hwtracing/qcom/tgu.c

[ ... ]

> -static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
> +static ssize_t tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
>  {
>  	int i, j, k, index;

[ ... ]

> @@ -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]
Can index be used uninitialized here?

In tgu_write_all_hw_regs(), index is declared without initialization. It is
conditionally assigned within nested loops bounded by values read from
hardware like num_step, num_reg, and num_condition_decode.

If the loops are bypassed because the hardware reports 0 for these limits,
index remains uninitialized. The function then evaluates index >= 0 ? 0 :
-EINVAL, which evaluates uninitialized stack memory and can cause
unpredictable device enablement success or failure.

[ ... ]

> @@ -278,6 +382,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 isn't a bug introduced by this patch, but are sysfs attributes exposed
to userspace here before their underlying data structures are allocated?

If a userspace process like udev reads the sysfs attributes immediately
after sysfs_create_groups() completes, callbacks like tgu_dataset_show()
will execute. These callbacks unconditionally dereference the unallocated
value_table pointer, leading to a NULL pointer dereference and a kernel
crash.

>  	if (ret) {
> @@ -300,6 +405,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 does returning -ENOMEM here
fail to remove manually created sysfs groups?

If an allocation like devm_kcalloc fails, the probe function returns an error.
The driver core cleans up devm resources and sets drvdata to NULL, but leaves
the manually created sysfs nodes untouched since they were not managed via
devres.

If a user subsequently interacts with the remaining sysfs files, callbacks
like enable_tgu_show() will fetch a NULL drvdata and dereference it. Should
sysfs_remove_groups() be called on this error path?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this driver mismanage runtime PM
usage counters?

In drivers/hwtracing/qcom/tgu.c:enable_tgu_store():

    } else {
        tgu_disable(dev);
        pm_runtime_put(dev);
    }

Writing 0 to the enable_tgu sysfs node unconditionally executes
pm_runtime_put(dev) even if the device is already disabled, leading to a
usage count underflow.

Additionally, during device unbinding in drivers/hwtracing/qcom/tgu.c:tgu_remove():

    sysfs_remove_groups(&dev->kobj, tgu_attr_groups);
    tgu_disable(dev);

If the device was previously enabled, the removal path fails to call
pm_runtime_put(dev), leaking the PM reference acquired in enable_tgu_store()
permanently.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4