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

[James Clark]

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