Re: [PATCH] coresight: etm3x: Fix buffer overwrite in cntr_val_show()

[James Clark <james.clark@linaro.org>]

On 26/11/2025 10:49 am, Mike Leach wrote:
> Hi,
> 
> On Mon, 24 Nov 2025 at 16:12, James Clark <james.clark@linaro.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@gmail.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.

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