Message-ID: <aSgPhd4u3Oqi3Ymy@google.com>
Date: Thu, 27 Nov 2025 16:44:53 +0800
From: Kuan-Wei Chiu <visitorckw@...il.com>
To: James Clark <james.clark@...aro.org>
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 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


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