Message-ID: <f217cbdc-a350-438e-a1b9-a377e6267337@linaro.org>
Date: Thu, 27 Nov 2025 09:17:12 +0000
From: James Clark <james.clark@...aro.org>
To: Kuan-Wei Chiu <visitorckw@...il.com>
Cc: Mike Leach <mike.leach@...aro.org>, suzuki.poulose@....com,
 alexander.shishkin@...ux.intel.com, pratikp@...eaurora.org,
 mathieu.poirier@...aro.org, gregkh@...uxfoundation.org,
 jserv@...s.ncku.edu.tw, marscheng@...gle.com, ericchancf@...gle.com,
 milesjiang@...gle.com, nickpan@...gle.com, coresight@...ts.linaro.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] coresight: etm3x: Fix buffer overwrite in cntr_val_show()



On 27/11/2025 8:44 am, Kuan-Wei Chiu wrote:
> On Wed, Nov 26, 2025 at 10:57:23AM +0000, James Clark wrote:
>>
>>
>> On 26/11/2025 10:49 am, Mike Leach wrote:
>>> Hi,
>>>
>>> On Mon, 24 Nov 2025 at 16:12, James Clark <james.clark@...aro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 21/11/2025 5:02 pm, Kuan-Wei Chiu wrote:
>>>>> Hi James,
>>>>>
>>>>> On Fri, Nov 21, 2025 at 09:50:03AM +0000, James Clark wrote:
>>>>>>
>>>>>>
>>>>>> On 21/11/2025 12:23 am, Kuan-Wei Chiu wrote:
>>>>>>> The cntr_val_show() function is meant to display the values of all
>>>>>>> available counters. However, the sprintf() call inside the loop was
>>>>>>> always writing to the beginning of the buffer, causing the output of
>>>>>>> previous iterations to be overwritten. As a result, only the value of
>>>>>>> the last counter was actually returned to the user.
>>>>>>>
>>>>>>> Fix this by using the return value of sprintf() to calculate the
>>>>>>> correct offset into the buffer for the next write, ensuring that all
>>>>>>> counter values are appended sequentially.
>>>>>>>
>>>>>>> Fixes: a939fc5a71ad ("coresight-etm: add CoreSight ETM/PTM driver")
>>>>>>> Signed-off-by: Kuan-Wei Chiu <visitorckw@...il.com>
>>>>>>> ---
>>>>>>> Build tested only. I do not have the hardware to run the etm3x driver,
>>>>>>> so I would be grateful if someone could verify this on actual hardware.
>>>>>>>
>>>>>>> I noticed this issue while browsing the coresight code after attending
>>>>>>> a technical talk on the subject. This code dates back to the initial
>>>>>>> driver submission over 10 years ago, so I was surprised it hadn't been
>>>>>>> caught earlier. Although I cannot perform runtime testing, the logic
>>>>>>> error seems obvious to me, so I still decided to submit this patch.
>>>>>>
>>>>>> Nice find. I think the point that it wasn't caught changes how we fix it.
>>>>>> Either nobody used it ever - so we can just delete it. Or someone was using
>>>>>> it and they expect it to always return a single entry with the value of the
>>>>>> last counter and this is a potentially breaking change. So maybe instead of
>>>>>> fixing this we should add a new cntr_vals_show() which works correctly. But
>>>>>> then again if nobody is using it we shouldn't do that either.
>>>>>
>>>>> Thanks for your feedback.
>>>>>
>>>>> I agree that if any tool relies on the current behavior, this patch
>>>>> would break the ABI and violate the hard rule that we must never break
>>>>> userspace.
>>>>>
>>>>> However, I am not sure how to determine if any userspace tools are
>>>>> actually using this sysfs interface. Is there a recommended way to
>>>>> verify this, or a standard procedure/convention to follow in this
>>>>> situation?
>>>>>
>>>>> Regards,
>>>>> Kuan-Wei
>>>>>
>>>>
>>>> There's no way of knowing apart from deducing that there are 0 users of
>>>> the 'correct' version of the API because it never existed. This isn't
>>>> even a regression, it was broken from the beginning.
>>>>
>>>> I suppose it does work when hardware only has one counter, but I don't
>>>> know how likely that is?
>>>>
>>>> Looking at cntr_val_store() I think I was wrong before when I said it
>>>> should be a separate file for each counter. Writing to it already writes
>>>> to the counter currently selected by cntr_idx and splitting it out to
>>>> separate files would have to break that too. So we can fix the display
>>>> bug by making show operate on the currently selected counter in the same
>>>> way as store. Then it makes a bit more sense and we can delete the
>>>> "counter %d: " prefix.
>>>>
>>>> ETM4 already does it this way too.
>>>>
>>> I agree with this.
>>>
>>> The difficulty in having a file per counter is that different
>>> implementations have different numbers of counters. Rather than
>>> complicate the driver by dynamically generating the sysfs files, the
>>
>> It wouldn't be that hard because there is a maximum number of counters.
>> You'd generate them all and then use the is_visible() thing to hide ones
>> that didn't exist on that device.
>>
>> But it looks like we don't want to do it that way for other reasons anyway.
>>
> 
> I don't feel it is my place to say whether the etm3x driver should be
> removed entirely.
> 
> However, if we decide to keep it, I agree that aligning cntr_val_show
> with the cntr_val_store behavior (using cntr_idx) makes more sense.
> 
> Here is my plan for v2:
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> index 762109307b86..77578885e8f3 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> @@ -717,26 +717,19 @@ static DEVICE_ATTR_RW(cntr_rld_event);
>   static ssize_t cntr_val_show(struct device *dev,
>   			     struct device_attribute *attr, char *buf)
>   {
> -	int i, ret = 0;
>   	u32 val;
>   	struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>   	struct etm_config *config = &drvdata->config;
>   
>   	if (!coresight_get_mode(drvdata->csdev)) {
>   		spin_lock(&drvdata->spinlock);
> -		for (i = 0; i < drvdata->nr_cntr; i++)
> -			ret += sprintf(buf, "counter %d: %x\n",
> -				       i, config->cntr_val[i]);
> +		val = config->cntr_val[config->cntr_idx];
>   		spin_unlock(&drvdata->spinlock);
> -		return ret;
> -	}
> -
> -	for (i = 0; i < drvdata->nr_cntr; i++) {
> -		val = etm_readl(drvdata, ETMCNTVRn(i));
> -		ret += sprintf(buf, "counter %d: %x\n", i, val);
> +		return sprintf(buf, "%x\n", val);
>   	}
>   
> -	return ret;
> +	val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx));
> +	return sprintf(buf, "%x\n", val);
>   }
>   
>   static ssize_t cntr_val_store(struct device *dev,
> 
> 
> Given the upcoming merge window, I plan to submit this v2 after -rc1
> is released.
> 
> Alternatively, if the consensus is to drop the driver, I am happy to
> submit a patch for that instead.
> 
> Regards,
> Kuan-Wei
> 
> 

The change looks good. I wouldn't wait for rc1, nobody else is touching 
these files so nothing will change, so makes sense to send it now.

>>> single file + selector paradigm makes things easier. The index is
>>> validated against the actual number of counters for this instance, and
>>> read/write occurs for the selected one.
>>>
>>> Regards
>>>
>>> Mike
>>>
>>>>>>
>>>>>> The interface isn't even that great, it should be a separate file per
>>>>>> counter. You don't want to be parsing strings and colons to try to read a
>>>>>> single value, especially in C. Separate files allows you to read it directly
>>>>>> without any hassle.
>>>>>>
>>>>>>>
>>>>>>>      drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 4 ++--
>>>>>>>      1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
>>>>>>> index 762109307b86..312033e74b7a 100644
>>>>>>> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
>>>>>>> @@ -725,7 +725,7 @@ static ssize_t cntr_val_show(struct device *dev,
>>>>>>>       if (!coresight_get_mode(drvdata->csdev)) {
>>>>>>>               spin_lock(&drvdata->spinlock);
>>>>>>>               for (i = 0; i < drvdata->nr_cntr; i++)
>>>>>>> -                   ret += sprintf(buf, "counter %d: %x\n",
>>>>>>> +                   ret += sprintf(buf + ret, "counter %d: %x\n",
>>>>>>>                                      i, config->cntr_val[i]);
>>>>>>>               spin_unlock(&drvdata->spinlock);
>>>>>>>               return ret;
>>>>>>> @@ -733,7 +733,7 @@ static ssize_t cntr_val_show(struct device *dev,
>>>>>>>       for (i = 0; i < drvdata->nr_cntr; i++) {
>>>>>>>               val = etm_readl(drvdata, ETMCNTVRn(i));
>>>>>>> -           ret += sprintf(buf, "counter %d: %x\n", i, val);
>>>>>>> +           ret += sprintf(buf + ret, "counter %d: %x\n", i, val);
>>>>>>>       }
>>>>>>>       return ret;
>>>>>>
>>>>
>>>
>>>
>>

Powered by blists - more mailing lists