From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 08A29324B31 for ; Thu, 27 Nov 2025 09:17:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764235038; cv=none; b=E3KexC5j2s8bfpnWiaMTGjFfDeEYXXkXvjcBoH6xji1ffJ+fsHFiQS0mkxZprdc/evbySmzVz+X7pCOattSKiDXr7XuO40xuwzScIkiul3q4B31NWULG1nyE8AUda1HNjIAgaY44E3p3GrAmGqJh4paJnTNgwG3pF5UpmE3Ovbg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764235038; c=relaxed/simple; bh=YDOfzoZ/mylGViObTQkVdxhAwu3WNN9PKmmuWjZ4nFs=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=u+kuJMbL1BTYa7uzBPbxmvRmRYchWzbAXiBL5tvMucIREsQkYWVm8TOgIksr0rKWxThdD4aXO1Hu/SdXDtaF7dsEPXzv0Yg2mRm5zVk8bkprMYONk7awoqY7j6Ti+iTlPIQMR+51nGcjvQZgiQmiZ7kzktwi72T7zFeX/9wafII= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=cpSPYw+v; arc=none smtp.client-ip=209.85.221.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="cpSPYw+v" Received: by mail-wr1-f44.google.com with SMTP id ffacd0b85a97d-42bb288c1bfso344525f8f.2 for ; Thu, 27 Nov 2025 01:17:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1764235034; x=1764839834; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=R7s6cE++aqSUFKnBMsR0G5G74lCzGeUp9KJnQeOz+5E=; b=cpSPYw+vDQ6dd7TgE42qq6DZRVZKJfaRGTd3wY/hw4Xj/9YbfTJMQrRIxyOoGaA8ot a0lSfLQyfTxmZriJOJXz8AZFbCuDOntfKyvVUtIY6vLhLMriLP3gtQbMOVZ/Uf16Czs5 kFeP0DKhNX0f+vVvQnoVItBfW6xuumVdrOuJj/PnG2+8iwW1mbjF3goztznK15S1PmXc A/wgaoDYbgtWLLtsfxQ7avMUjeC5PHApqX3HdKdvmIUVOeAerv0O0bDUyGn8gfjiQLUo VxWOZn4aOjyQ3Lb3WKbzpWW8vGKIut/pNHvO26EvUMZQ9Xv+BdNxvI/k+SzOEySaybSY jVtQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764235034; x=1764839834; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=R7s6cE++aqSUFKnBMsR0G5G74lCzGeUp9KJnQeOz+5E=; b=PJSP/2YTKpJMTVmJCK/0UYpN7jpQ0xgXP2YgL9XXQPbaFpTwNZNtV1RSD3YNUSnyIU Kf9TzjrNg9RMTM84NyNxAcfc+le2RP77x6ICBQx1kTuMB1lFd565n5hw4o6/PmK4Scs6 6gd7DRvkwQEF8ltAzdADz8zkdWd4GWOudvu8T8DmvmZRIdJfd/Zx3vZbnNDnRyuGtOtn Fke3lRc+dWcdwQ0/UVU/voPMo3Py/OBFhj9QnbMVblkbhQlo6hmG+nq9J5+E8E+pxC/P 2OtlL3vBFwPR2a7SGjj7lj4uRDYbEPuSdA2SXYsUaW235TDlJkSP8NjTm7LibKJIgt7r P+IA== X-Forwarded-Encrypted: i=1; AJvYcCWqd2f0ndV2PjWSRMIzRK1VNSONQsCzsa+NDArrMWuNw5I75t/bWWt1ivK5Pga2jhBjB4w2dKPCf4Ee4xY=@vger.kernel.org X-Gm-Message-State: AOJu0Yw0pOq8A6+Y1wzuiZdvC7WumEGuYJs9hTAFALa/tn59cXS6j9Db QWBsYtrEKxqANAJJslL4cjw3hQAflg7kh02N0gnSc1LwSIL8dl7pz0Kyvjei4zjV1wM= X-Gm-Gg: ASbGncv09MVgXBCxqxnUAr6oxj3AjNHcExb9MDkbwe2LwzEYJnROPMcPuQzwrDtoYbB A9t4Ax6UykcXfJXgixoy0jZ1VmmOqY/tmRucx+wC7Nqn/lt05HR5/CxV6AK/xNTjYDSu9g7lgg8 RmvwkFfNiTjvKl4ntVTJELkwq/QTbLdkznZaFB7dfDEwPTWavwVP3WGanFvzJpzUxG16GFMBy4E Vhhaj5Yto51e2RvPXzcj3KAqcnjr4xNcu+TrehLvN7iufoXw0lLJUDOoF9uu1twPKRu2SjenOfq Tyz4QYsFm+ucji2hsroIm4y/Zzl4Uec8GsRtgesHgkYIFyq9QMkuobTVOoSG1yEyEASWm/L5i4o gyWK5x0zVrCbc0Zgboj3va/pp5JeRgGyX9fBf1rSB/BdonmFduqQ1pt2W3x7lTdEe80QKnVwVMS EX2mcSZEs3f2gGcppG X-Google-Smtp-Source: AGHT+IHMYZZ56RtTkgImb02ngxPYjvhlBk8TwzkqtoGo5krRVnM2mglutVBFMQNBv9Z7KKR5+lHRAw== X-Received: by 2002:a5d:588c:0:b0:3ee:3dce:f672 with SMTP id ffacd0b85a97d-42e0f1e3428mr10030410f8f.4.1764235034134; Thu, 27 Nov 2025 01:17:14 -0800 (PST) Received: from [192.168.1.3] ([185.48.77.170]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-42e1cab9af3sm2315819f8f.41.2025.11.27.01.17.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 27 Nov 2025 01:17:13 -0800 (PST) Message-ID: Date: Thu, 27 Nov 2025 09:17:12 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] coresight: etm3x: Fix buffer overwrite in cntr_val_show() To: Kuan-Wei Chiu Cc: Mike Leach , suzuki.poulose@arm.com, alexander.shishkin@linux.intel.com, pratikp@codeaurora.org, mathieu.poirier@linaro.org, gregkh@linuxfoundation.org, jserv@ccns.ncku.edu.tw, marscheng@google.com, ericchancf@google.com, milesjiang@google.com, nickpan@google.com, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <20251121002350.1166758-1-visitorckw@gmail.com> <172ca2d9-4a6f-4498-bdfd-8aa7428581ce@linaro.org> <05babb6d-a588-49f8-a34a-c82d5f58adf5@linaro.org> <68efd5a2-3b65-4f37-9bf0-40c4e5ade480@linaro.org> Content-Language: en-US From: James Clark In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 27/11/2025 8:44 am, Kuan-Wei Chiu wrote: > 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 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 >>>>>>> --- >>>>>>> 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 > > The change looks good. I wouldn't wait for rc1, nobody else is touching these files so nothing will change, so makes sense to send it now. >>> 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; >>>>>> >>>> >>> >>> >>