Message-ID: <CAJ9a7VipxW5-2roAQA3y212sQWr1kq2HNbLK1jmLNP1jFseYiQ@mail.gmail.com>
Date: Wed, 26 Nov 2025 10:49:15 +0000
From: Mike Leach <mike.leach@...aro.org>
To: James Clark <james.clark@...aro.org>
Cc: Kuan-Wei Chiu <visitorckw@...il.com>, 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()

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


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Powered by blists - more mailing lists