Message-ID: <05babb6d-a588-49f8-a34a-c82d5f58adf5@linaro.org>
Date: Mon, 24 Nov 2025 16:12:34 +0000
From: James Clark <james.clark@...aro.org>
To: Kuan-Wei Chiu <visitorckw@...il.com>
Cc: suzuki.poulose@....com, mike.leach@...aro.org,
 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 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.

>>
>> 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