From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) (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 742E2314B6A for ; Mon, 24 Nov 2025 16:12:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764000759; cv=none; b=KUAn9jcijW5lbNeHAIzvMdgVasDDxxhiybsXf98YZ4FCO3KQZgxxRWA+z0JPe/mg2c3YL8wK0GBAy+IKV//jee1qPHr45dtV3LNNcRo7EE+wud8zNfCVTEQRGh1yzRgOHfd3z4JNwZP7hZl6dcoeg2Eo4zIBywrSMBwk9x94PsQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764000759; c=relaxed/simple; bh=Y9MOuEQiSj+lQxJV+tPDlWcwXYWdDz1BVSv8a6qO9DM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Ek+14inmb5Usn+NB7YUUwrBfN6MO6VVHWkUxpY0/DtlldFIQgvMo05aN5kJq0gzJOYw8rJQGbv9PK24oko87HgRKXXD+YiS96IoFBnT2KwixxWndMjDuSsdp8JoIr0HFPdKnZMROKNnD2+fTdyn9wBMGW7MAxKsxu9Pf5dOAtm0= 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=mzYSadPR; arc=none smtp.client-ip=209.85.128.41 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="mzYSadPR" Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-4777771ed1aso29890305e9.2 for ; Mon, 24 Nov 2025 08:12:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1764000756; x=1764605556; 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=9kUwL5iJCQukHjA2c6jwYym9BNWDh7Ih2TgBHqMmyUg=; b=mzYSadPRQTw3NVKH4ts2oujEyi8Sja4NhUp77oLaH9R1UfCs86ZA5VqzjQVVsbj4uw 4iQKUOGJo2rPJKR1W3iSXi81p3mngqCvWenAKRNOMXlk2oHAdqCLy3t8e0+pd+05uGNq NsyvNTXJJd3Afkrwv+rJiuj0ZX7NGpDQsSp2ZtNzhIu98O8JJB7ODcLc0xA/dJo8Z90m jnNYNd3F8Puq+SbCdZTx+1dKxDdVO3qcRmChoPoqsiuPObCrohDv1Oydmj8+tNbmFeiG WJxg9j63rRnNGxkxrc3lwyN+lWGzzF/IMaPjCH+Mxyw9XDd8E4XHOefbbenwz8QJk84q qyQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764000756; x=1764605556; 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=9kUwL5iJCQukHjA2c6jwYym9BNWDh7Ih2TgBHqMmyUg=; b=C33a4Erhd271QS36w7X7pZ/1WFUl/l+RoGlFOr7WV6sVPkKSfOpDF31rhCA2ZaB6M6 S7VH2rx337DPx45qbVnXnY0l/JQ1YirqIeSTVZlu8uSFNU0cZL2QNqisnC1XqSCvg8Ge uRd8vBM53k7rQFhPXqCkwLXmlQNenP3DFzVpP8V2qSKc/nrdcu7Am7rnHkEFp9qrGpB+ gG7uV89IkWNvAusjr5TZKbVkZEgpw7CMDzJ8wQyAxbLa89iAMERAcpJyZ3B3eN2M2ner pUItYeA6AsXbg26zgFenj1SOgrwHSPx4aX8CleUSY2xkFTjHHWM0jEj6IcEekEwoWhq2 z+ZQ== X-Forwarded-Encrypted: i=1; AJvYcCW+a/7E/NnyvgUG003zfkte5Tc6mhS+PhfCHP/jfs7PihdzCTSxedBNrSRdDs3ZrdxlmXyMjt10vuZFTVw=@vger.kernel.org X-Gm-Message-State: AOJu0YyiMDVe/7if3FJD3ve5Z1HftRgAfECYsgWc/45P+z5rYRyTECbx 4pRSNiBLje4NYCO/wR+Aqp6BLOwGrmRtUiouWlw/2bUbxFjRj1n7d1x58n4yyuSG9MY= X-Gm-Gg: ASbGncs3xitFS69xfeIwUJEnrho7zL+/JHt5vyKnnBMas6HNgp2+48IsS+DeRMLR4OT etdn3JHttMLnnzK2ApvnQ5CqMzUW+zZ7el9TDa91Otx/VUsjDRELozXkO/Twgz/mf2WcLbC+ccM Z8Gb26cMAZcel9ID9/9tQwDHwfF/WcAhHJlaNhEa7T/k1Kud5ZY7GhByb+q9MsULz4Br/Q7GnFX BxwFX/bhD8Lv8oYReR8PpleILGxIqjEaM+zGA9EwRBp8PbIVVspOMvElVFxv6MGMhZVO6aA/XcX 9fhEyKK9qoZESXmy44JtD/lmv/zTYXsCh8EtbtYVSVVdjcAmnkkUjuUPhnDS9tIYcUxnM5pRZ4c s4qsHn5xbSEuUBGvoZAevdxYqoglbpFW/woq4pE8O4axXqIgyIKXvrRU66F6HSipxaamj9Zt66T 5YEECgacEa+2fUBQRF X-Google-Smtp-Source: AGHT+IFy72lQUXBFhPuwWRndjDr59yWxK1zJh9Nt3ZYz3copTeRMO5SUVt2oB9vIYaFjGLRGWbz7rA== X-Received: by 2002:a05:600c:1d0e:b0:477:89d5:fdb2 with SMTP id 5b1f17b1804b1-477c017d9damr124191795e9.14.1764000755679; Mon, 24 Nov 2025 08:12:35 -0800 (PST) Received: from [192.168.1.3] ([185.48.77.170]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-477bf1df392sm205509945e9.1.2025.11.24.08.12.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 24 Nov 2025 08:12:35 -0800 (PST) Message-ID: <05babb6d-a588-49f8-a34a-c82d5f58adf5@linaro.org> Date: Mon, 24 Nov 2025 16:12:34 +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: suzuki.poulose@arm.com, mike.leach@linaro.org, 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> Content-Language: en-US From: James Clark In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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. >> >> 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; >>