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] [thread-next>] [day] [month] [year] [list]

Message-ID: <bed84247-c3c0-47b8-a491-9af9e51ca5a7@linaro.org>
Date: Thu, 27 Nov 2025 09:30:28 +0000
From: James Clark <james.clark@...aro.org>
To: Leo Yan <leo.yan@....com>, Kuan-Wei Chiu <visitorckw@...il.com>
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 27/11/2025 9:22 am, Leo Yan wrote:
> On Thu, Nov 27, 2025 at 04:44:53PM +0800, Kuan-Wei Chiu wrote:
> 
> [...]
> 
>> I don't feel it is my place to say whether the etm3x driver should be
>> removed entirely.
> 
> Sorry for confusion.  Your fix patch is welcome, this is useful no
> matter if remove the ETMv3 driver or not.
> 
>> 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));
> 
> It is not right to read register at here (it cannot promise to read the
> CPU (cp14) register on the target CPU).
> 
> Please refer to the same function in coresight-etm4x-sysfs.c. I think
> we can do the same thing at here.
> 

That's a different fix than the display bug though. This change doesn't 
change that if it's already wrong. The display fix should go in alone 
and then if there is an issue with not running things on the right CPU 
that should go in separately.

>> +	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.
> 
> Please continue this patch.  Thanks a lot!
> 
> Leo