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

[Kuan-Wei Chiu <visitorckw@gmail.com>]

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

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