| 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
| ||
|
Message-ID: <20251020143718.GH281971@e132581.arm.com>
Date: Mon, 20 Oct 2025 15:37:18 +0100
From: Leo Yan <leo.yan@....com>
To: Xiaoqi Zhuang <xiaoqi.zhuang@....qualcomm.com>
Cc: Suzuki K Poulose <suzuki.poulose@....com>,
Mike Leach <mike.leach@...aro.org>,
James Clark <james.clark@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH] coresight: ETR: Fix ETR buffer use-after-free issue
On Mon, Oct 20, 2025 at 05:06:46PM +0800, Xiaoqi Zhuang wrote:
> When ETR is enabled as CS_MODE_SYSFS, if the buffer size is changed
> and enabled again, currently sysfs_buf will point to the newly
> allocated memory(buf_new) and free the old memory(buf_old). But the
> etr_buf that is being used by the ETR remains pointed to buf_old, not
> updated to buf_new. In this case, it will result in a memory
> use-after-free issue.
I struggled to understand how to reproduce the issue under the condition
"if the buffer size is changed and enabled again."
I don't think the flow below where the trace is re-enabled would cause
an issue:
- Step 1: Enable trace path between ETM0 -> ETR0;
- Step 2: Change the buffer size for ETR0;
- Step 3: Disable trace path between ETM0 -> ETR0;
- Step 4: Enable again trace path between ETM0 -> ETR0.
In this case, step3 releases the buffer and update "drvdata->etr_buf" to
NULL, and step 4 allocates a new buffer and assign it to
"drvdata->etr_buf".
The problem should occur when operating on two trace paths, E.g.,
- Step 1: Enable trace path between ETM0 -> ETR0;
- Step 2: Change the buffer size for ETR0;
- Step 3: Enable trace path between ETM1 -> ETR0;
In step3, the driver releases the existed buffer and allocate a new one.
At the meantime, "drvdata->etr_buf" still holds the buffer allocated in
step 1.
> Fix this by checking ETR's mode before updating and releasing buf_old,
> if the mode is CS_MODE_SYSFS, then skip updating and releasing it.
Given that we now have a couple of reported issues related to ETR mode,
I'd like to refactor the ETR mode handling and its reference counting
thoroughly. I've drafted a large change (it's quite big, but we can
split it into small patches if we agree to proceed).
Thanks for reporting the issue!
Leo
---8<---
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index b07fcdb3fe1a..d0fac958c614 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1241,6 +1241,8 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
struct etr_buf *sysfs_buf = NULL, *new_buf = NULL, *free_buf = NULL;
+ WARN_ON(coresight_get_mode(csdev) != CS_MODE_SYSFS);
+
/*
* If we are enabling the ETR from disabled state, we need to make
* sure we have a buffer with the right size. The etr_buf is not reset
@@ -1263,7 +1265,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
raw_spin_lock_irqsave(&drvdata->spinlock, flags);
}
- if (drvdata->reading || coresight_get_mode(csdev) == CS_MODE_PERF) {
+ if (drvdata->reading) {
ret = -EBUSY;
goto out;
}
@@ -1292,30 +1294,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
int ret = 0;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- struct etr_buf *sysfs_buf = tmc_etr_get_sysfs_buffer(csdev);
+ struct etr_buf *sysfs_buf;
+ sysfs_buf = tmc_etr_get_sysfs_buffer(csdev);
if (IS_ERR(sysfs_buf))
return PTR_ERR(sysfs_buf);
raw_spin_lock_irqsave(&drvdata->spinlock, flags);
-
- /*
- * In sysFS mode we can have multiple writers per sink. Since this
- * sink is already enabled no memory is needed and the HW need not be
- * touched, even if the buffer size has changed.
- */
- if (coresight_get_mode(csdev) == CS_MODE_SYSFS) {
- csdev->refcnt++;
- goto out;
- }
-
ret = tmc_etr_enable_hw(drvdata, sysfs_buf);
- if (!ret) {
- coresight_set_mode(csdev, CS_MODE_SYSFS);
- csdev->refcnt++;
- }
-
-out:
raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
if (!ret)
@@ -1735,11 +1721,6 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle);
raw_spin_lock_irqsave(&drvdata->spinlock, flags);
- /* Don't use this sink if it is already claimed by sysFS */
- if (coresight_get_mode(csdev) == CS_MODE_SYSFS) {
- rc = -EBUSY;
- goto unlock_out;
- }
if (WARN_ON(!etr_perf || !etr_perf->etr_buf)) {
rc = -EINVAL;
@@ -1759,18 +1740,14 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
* No HW configuration is needed if the sink is already in
* use for this session.
*/
- if (drvdata->pid == pid) {
- csdev->refcnt++;
+ if (drvdata->pid == pid)
goto unlock_out;
- }
rc = tmc_etr_enable_hw(drvdata, etr_perf->etr_buf);
if (!rc) {
/* Associate with monitored process. */
drvdata->pid = pid;
- coresight_set_mode(csdev, CS_MODE_PERF);
drvdata->perf_buf = etr_perf->etr_buf;
- csdev->refcnt++;
}
unlock_out:
@@ -1778,17 +1755,76 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
return rc;
}
+static int tmc_acquire_mode(struct coresight_device *csdev, enum cs_mode mode)
+{
+ struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+ if (mode != CS_MODE_SYSFS && mode != CS_MODE_PERF)
+ return -EINVAL;
+
+ scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock);
+
+ if (coresight_get_mode(csdev) == CS_MODE_DISABLED) {
+ if (!csdev->refcnt)
+ coresight_set_mode(csdev, mode);
+ csdev->refcnt++;
+ } else if (coresight_get_mode(csdev) != mode) {
+ ret = -EBUSY;
+ }
+
+ return csdev->refcnt;
+}
+
+static void tmc_release_mode(struct coresight_device *csdev, enum cs_mode mode)
+{
+ struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+ scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock);
+
+ if (WARN_ON(coresight_get_mode(csdev) != mode))
+ return;
+
+ csdev->refcnt--;
+ if (!csdev->refcnt)
+ coresight_set_mode(csdev, CS_MODE_DISABLED);
+}
+
static int tmc_enable_etr_sink(struct coresight_device *csdev,
enum cs_mode mode, void *data)
{
+ unsigned long flags;
+ struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+ int ret;
+
+ ret = tmc_acquire_mode(csdev, mode);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * For sysfs mode, the higher level mutex ensures exclusively
+ * enabling sink, it is safe to bail out if this is not the
+ * first time to enable sink.
+ *
+ * A perf session can enable the same sink simultaneously, fall
+ * through to call tmc_enable_etr_sink_perf() to ensure the sink
+ * has been enabled.
+ */
+ if (mode == CS_MODE_SYSFS && ret > 1)
+ return 0;
+
switch (mode) {
case CS_MODE_SYSFS:
- return tmc_enable_etr_sink_sysfs(csdev);
+ ret = tmc_enable_etr_sink_sysfs(csdev);
case CS_MODE_PERF:
- return tmc_enable_etr_sink_perf(csdev, data);
+ ret = tmc_enable_etr_sink_perf(csdev, data);
default:
- return -EINVAL;
+ ret = -EINVAL;
}
+
+ if (ret)
+ tmc_release_mode(csdev, mode);
+
+ return ret;
}
static int tmc_disable_etr_sink(struct coresight_device *csdev)
@@ -1796,30 +1832,20 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- raw_spin_lock_irqsave(&drvdata->spinlock, flags);
+ tmc_release_mode(csdev, mode);
- if (drvdata->reading) {
- raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
- return -EBUSY;
- }
+ scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock);
- csdev->refcnt--;
- if (csdev->refcnt) {
- raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
+ if (csdev->refcnt || drvdata->reading)
return -EBUSY;
- }
- /* Complain if we (somehow) got out of sync */
- WARN_ON_ONCE(coresight_get_mode(csdev) == CS_MODE_DISABLED);
+ if (drvdata->pid == -1)
+ return 0;
+
tmc_etr_disable_hw(drvdata);
- /* Dissociate from monitored process. */
- drvdata->pid = -1;
- coresight_set_mode(csdev, CS_MODE_DISABLED);
/* Reset perf specific data */
drvdata->perf_buf = NULL;
- raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
-
dev_dbg(&csdev->dev, "TMC-ETR disabled\n");
return 0;
}
Powered by blists - more mailing lists