linux-kernel - Re: [PATCH] coresight: etm3x: Fix buffer overwrite in cntr_val

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aSCbJR9aT1vOpz0a@google.com>
Date: Sat, 22 Nov 2025 01:02:29 +0800
From: Kuan-Wei Chiu <visitorckw@...il.com>
To: James Clark <james.clark@...aro.org>
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()

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

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