* [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
@ 2025-01-09 17:19 Yeoreum Yun
2025-03-12 6:36 ` Yeo Reum Yun
2025-03-14 14:50 ` Mike Leach
0 siblings, 2 replies; 9+ messages in thread
From: Yeoreum Yun @ 2025-01-09 17:19 UTC (permalink / raw)
To: suzuki.poulose, mike.leach, james.clark, alexander.shishkin
Cc: coresight, linux-arm-kernel, linux-kernel, Yeoreum Yun
While enable active config via cscfg_csdev_enable_active_config(),
active config could be deactivated via configfs' sysfs interface.
This could make UAF issue in below scenario:
CPU0 CPU1
(sysfs enable) load module
cscfg_load_config_sets()
activate config. // sysfs
(sys_active_cnt == 1)
...
cscfg_csdev_enable_active_config()
lock(csdev->cscfg_csdev_lock)
// here load config activate by CPU1
unlock(csdev->cscfg_csdev_lock)
deactivate config // sysfs
(sys_activec_cnt == 0)
cscfg_unload_config_sets()
unload module
// access to config_desc which freed
// while unloading module.
cfs_csdev_enable_config
To address this, use cscfg_config_desc's active_cnt as a reference count
which will be holded when
- activate the config.
- enable the activated config.
and put the module reference when config_active_cnt == 0.
Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
from v2 to v3:
- add cscfg_config_desc_get()/put() (from Suzuki).
- remove sys_enable_cnt.
from v1 to v2:
- modify commit message.
---
.../hwtracing/coresight/coresight-config.h | 2 +-
.../coresight/coresight-etm4x-core.c | 3 +
.../hwtracing/coresight/coresight-syscfg.c | 55 ++++++++++++++-----
3 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
index b9ebc9fcfb7f..90fd937d3bd8 100644
--- a/drivers/hwtracing/coresight/coresight-config.h
+++ b/drivers/hwtracing/coresight/coresight-config.h
@@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
* @feats_csdev:references to the device features to enable.
*/
struct cscfg_config_csdev {
- const struct cscfg_config_desc *config_desc;
+ struct cscfg_config_desc *config_desc;
struct coresight_device *csdev;
bool enabled;
struct list_head node;
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 86893115df17..6218ef40acbc 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -986,6 +986,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
raw_spin_unlock(&drvdata->spinlock);
+
+ cscfg_csdev_disable_active_config(csdev);
+
cpus_read_unlock();
/*
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
index a70c1454b410..8766f72db33e 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.c
+++ b/drivers/hwtracing/coresight/coresight-syscfg.c
@@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
{
struct cscfg_config_csdev *config_csdev, *tmp;
+ unsigned long flags;
if (list_empty(&csdev->config_csdev_list))
return;
+ raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
if (config_csdev->config_desc->load_owner == load_owner)
list_del(&config_csdev->node);
}
+ raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
}
static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
@@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
}
EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
+static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
+{
+ if (enable)
+ return atomic_inc_not_zero(&config_desc->active_cnt);
+
+ /* when activate config */
+
+ /* must ensure that config cannot be unloaded in use */
+ if (cscfg_owner_get(config_desc->load_owner))
+ return false;
+
+ atomic_inc(&config_desc->active_cnt);
+
+ return true;
+}
+
+static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
+{
+ if (!atomic_dec_return(&config_desc->active_cnt))
+ cscfg_owner_put(config_desc->load_owner);
+}
+
/*
* This activate configuration for either perf or sysfs. Perf can have multiple
* active configs, selected per event, sysfs is limited to one.
@@ -890,22 +915,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
if (config_desc->available == false)
return -EBUSY;
- /* must ensure that config cannot be unloaded in use */
- err = cscfg_owner_get(config_desc->load_owner);
- if (err)
+ if (!cscfg_config_desc_get(config_desc, false)) {
+ err = -EINVAL;
break;
+ }
+
/*
* increment the global active count - control changes to
* active configurations
*/
atomic_inc(&cscfg_mgr->sys_active_cnt);
- /*
- * mark the descriptor as active so enable config on a
- * device instance will use it
- */
- atomic_inc(&config_desc->active_cnt);
-
err = 0;
dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
break;
@@ -920,9 +940,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)
list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
- atomic_dec(&config_desc->active_cnt);
atomic_dec(&cscfg_mgr->sys_active_cnt);
- cscfg_owner_put(config_desc->load_owner);
+ cscfg_config_desc_put(config_desc);
dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
break;
}
@@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
unsigned long cfg_hash, int preset)
{
struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
- const struct cscfg_config_desc *config_desc;
+ struct cscfg_config_desc *config_desc;
unsigned long flags;
int err = 0;
@@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
config_desc = config_csdev_item->config_desc;
- if ((atomic_read(&config_desc->active_cnt)) &&
- ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
+ if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
+ cscfg_config_desc_get(config_desc, true)) {
config_csdev_active = config_csdev_item;
csdev->active_cscfg_ctxt = (void *)config_csdev_active;
break;
@@ -1097,7 +1116,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
err = -EBUSY;
raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
}
+
+ if (err)
+ cscfg_config_desc_put(config_desc);
}
+
return err;
}
EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
@@ -1136,8 +1159,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
/* true if there was an enabled active config */
- if (config_csdev)
+ if (config_csdev) {
cscfg_csdev_disable_config(config_csdev);
+ cscfg_config_desc_put(config_csdev->config_desc);
+ }
}
EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);
--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
2025-01-09 17:19 [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config Yeoreum Yun
@ 2025-03-12 6:36 ` Yeo Reum Yun
2025-03-14 14:50 ` Mike Leach
1 sibling, 0 replies; 9+ messages in thread
From: Yeo Reum Yun @ 2025-03-12 6:36 UTC (permalink / raw)
To: Suzuki Poulose, mike.leach@linaro.org, james.clark@linaro.org,
alexander.shishkin@linux.intel.com
Cc: coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Gentle ping in case of forgotten,..
________________________________________
From: Yeoreum Yun <yeoreum.yun@arm.com>
Sent: 09 January 2025 17:19
To: Suzuki Poulose; mike.leach@linaro.org; james.clark@linaro.org; alexander.shishkin@linux.intel.com
Cc: coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Yeo Reum Yun
Subject: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
While enable active config via cscfg_csdev_enable_active_config(),
active config could be deactivated via configfs' sysfs interface.
This could make UAF issue in below scenario:
CPU0 CPU1
(sysfs enable) load module
cscfg_load_config_sets()
activate config. // sysfs
(sys_active_cnt == 1)
...
cscfg_csdev_enable_active_config()
lock(csdev->cscfg_csdev_lock)
// here load config activate by CPU1
unlock(csdev->cscfg_csdev_lock)
deactivate config // sysfs
(sys_activec_cnt == 0)
cscfg_unload_config_sets()
unload module
// access to config_desc which freed
// while unloading module.
cfs_csdev_enable_config
To address this, use cscfg_config_desc's active_cnt as a reference count
which will be holded when
- activate the config.
- enable the activated config.
and put the module reference when config_active_cnt == 0.
Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
from v2 to v3:
- add cscfg_config_desc_get()/put() (from Suzuki).
- remove sys_enable_cnt.
from v1 to v2:
- modify commit message.
---
.../hwtracing/coresight/coresight-config.h | 2 +-
.../coresight/coresight-etm4x-core.c | 3 +
.../hwtracing/coresight/coresight-syscfg.c | 55 ++++++++++++++-----
3 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
index b9ebc9fcfb7f..90fd937d3bd8 100644
--- a/drivers/hwtracing/coresight/coresight-config.h
+++ b/drivers/hwtracing/coresight/coresight-config.h
@@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
* @feats_csdev:references to the device features to enable.
*/
struct cscfg_config_csdev {
- const struct cscfg_config_desc *config_desc;
+ struct cscfg_config_desc *config_desc;
struct coresight_device *csdev;
bool enabled;
struct list_head node;
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 86893115df17..6218ef40acbc 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -986,6 +986,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
raw_spin_unlock(&drvdata->spinlock);
+
+ cscfg_csdev_disable_active_config(csdev);
+
cpus_read_unlock();
/*
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
index a70c1454b410..8766f72db33e 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.c
+++ b/drivers/hwtracing/coresight/coresight-syscfg.c
@@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
{
struct cscfg_config_csdev *config_csdev, *tmp;
+ unsigned long flags;
if (list_empty(&csdev->config_csdev_list))
return;
+ raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
if (config_csdev->config_desc->load_owner == load_owner)
list_del(&config_csdev->node);
}
+ raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
}
static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
@@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
}
EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
+static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
+{
+ if (enable)
+ return atomic_inc_not_zero(&config_desc->active_cnt);
+
+ /* when activate config */
+
+ /* must ensure that config cannot be unloaded in use */
+ if (cscfg_owner_get(config_desc->load_owner))
+ return false;
+
+ atomic_inc(&config_desc->active_cnt);
+
+ return true;
+}
+
+static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
+{
+ if (!atomic_dec_return(&config_desc->active_cnt))
+ cscfg_owner_put(config_desc->load_owner);
+}
+
/*
* This activate configuration for either perf or sysfs. Perf can have multiple
* active configs, selected per event, sysfs is limited to one.
@@ -890,22 +915,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
if (config_desc->available == false)
return -EBUSY;
- /* must ensure that config cannot be unloaded in use */
- err = cscfg_owner_get(config_desc->load_owner);
- if (err)
+ if (!cscfg_config_desc_get(config_desc, false)) {
+ err = -EINVAL;
break;
+ }
+
/*
* increment the global active count - control changes to
* active configurations
*/
atomic_inc(&cscfg_mgr->sys_active_cnt);
- /*
- * mark the descriptor as active so enable config on a
- * device instance will use it
- */
- atomic_inc(&config_desc->active_cnt);
-
err = 0;
dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
break;
@@ -920,9 +940,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)
list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
- atomic_dec(&config_desc->active_cnt);
atomic_dec(&cscfg_mgr->sys_active_cnt);
- cscfg_owner_put(config_desc->load_owner);
+ cscfg_config_desc_put(config_desc);
dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
break;
}
@@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
unsigned long cfg_hash, int preset)
{
struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
- const struct cscfg_config_desc *config_desc;
+ struct cscfg_config_desc *config_desc;
unsigned long flags;
int err = 0;
@@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
config_desc = config_csdev_item->config_desc;
- if ((atomic_read(&config_desc->active_cnt)) &&
- ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
+ if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
+ cscfg_config_desc_get(config_desc, true)) {
config_csdev_active = config_csdev_item;
csdev->active_cscfg_ctxt = (void *)config_csdev_active;
break;
@@ -1097,7 +1116,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
err = -EBUSY;
raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
}
+
+ if (err)
+ cscfg_config_desc_put(config_desc);
}
+
return err;
}
EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
@@ -1136,8 +1159,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
/* true if there was an enabled active config */
- if (config_csdev)
+ if (config_csdev) {
cscfg_csdev_disable_config(config_csdev);
+ cscfg_config_desc_put(config_csdev->config_desc);
+ }
}
EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);
--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
2025-01-09 17:19 [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config Yeoreum Yun
2025-03-12 6:36 ` Yeo Reum Yun
@ 2025-03-14 14:50 ` Mike Leach
2025-03-14 15:24 ` Yeo Reum Yun
1 sibling, 1 reply; 9+ messages in thread
From: Mike Leach @ 2025-03-14 14:50 UTC (permalink / raw)
To: Yeoreum Yun
Cc: suzuki.poulose, james.clark, alexander.shishkin, coresight,
linux-arm-kernel, linux-kernel
Hi,
On Thu, 9 Jan 2025 at 17:20, Yeoreum Yun <yeoreum.yun@arm.com> wrote:
>
> While enable active config via cscfg_csdev_enable_active_config(),
> active config could be deactivated via configfs' sysfs interface.
> This could make UAF issue in below scenario:
>
> CPU0 CPU1
> (sysfs enable) load module
> cscfg_load_config_sets()
> activate config. // sysfs
> (sys_active_cnt == 1)
> ...
> cscfg_csdev_enable_active_config()
> lock(csdev->cscfg_csdev_lock)
> // here load config activate by CPU1
> unlock(csdev->cscfg_csdev_lock)
>
> deactivate config // sysfs
> (sys_activec_cnt == 0)
> cscfg_unload_config_sets()
> unload module
>
> // access to config_desc which freed
> // while unloading module.
> cfs_csdev_enable_config
>
> To address this, use cscfg_config_desc's active_cnt as a reference count
> which will be holded when
> - activate the config.
> - enable the activated config.
> and put the module reference when config_active_cnt == 0.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---
> from v2 to v3:
> - add cscfg_config_desc_get()/put() (from Suzuki).
> - remove sys_enable_cnt.
>
> from v1 to v2:
> - modify commit message.
> ---
> .../hwtracing/coresight/coresight-config.h | 2 +-
> .../coresight/coresight-etm4x-core.c | 3 +
> .../hwtracing/coresight/coresight-syscfg.c | 55 ++++++++++++++-----
> 3 files changed, 44 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index b9ebc9fcfb7f..90fd937d3bd8 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
> * @feats_csdev:references to the device features to enable.
> */
> struct cscfg_config_csdev {
> - const struct cscfg_config_desc *config_desc;
> + struct cscfg_config_desc *config_desc;
> struct coresight_device *csdev;
> bool enabled;
> struct list_head node;
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 86893115df17..6218ef40acbc 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -986,6 +986,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
>
> raw_spin_unlock(&drvdata->spinlock);
> +
> + cscfg_csdev_disable_active_config(csdev);
> +
> cpus_read_unlock();
>
> /*
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index a70c1454b410..8766f72db33e 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
> static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
> {
> struct cscfg_config_csdev *config_csdev, *tmp;
> + unsigned long flags;
>
> if (list_empty(&csdev->config_csdev_list))
> return;
>
> + raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
> if (config_csdev->config_desc->load_owner == load_owner)
> list_del(&config_csdev->node);
> }
> + raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> }
>
> static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
>
> +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
> +{
> + if (enable)
> + return atomic_inc_not_zero(&config_desc->active_cnt);
> +
Not sure why we have an "enable" parameter here - it completely
changes the meaning of the function - with no comment at the start.
From where it is called it makes figuring out the logic confusing.
Just keep the "get" functionality and drop the parameter & the nz check.
> + /* when activate config */
> +
> + /* must ensure that config cannot be unloaded in use */
> + if (cscfg_owner_get(config_desc->load_owner))
> + return false;
> +
> + atomic_inc(&config_desc->active_cnt);
> +
> + return true;
> +}
> +
> +static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
> +{
> + if (!atomic_dec_return(&config_desc->active_cnt))
> + cscfg_owner_put(config_desc->load_owner);
> +}
> +
> /*
> * This activate configuration for either perf or sysfs. Perf can have multiple
> * active configs, selected per event, sysfs is limited to one.
> @@ -890,22 +915,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
> if (config_desc->available == false)
> return -EBUSY;
>
> - /* must ensure that config cannot be unloaded in use */
> - err = cscfg_owner_get(config_desc->load_owner);
> - if (err)
> + if (!cscfg_config_desc_get(config_desc, false)) {
Drop the false parameter here.
> + err = -EINVAL;
> break;
> + }
> +
> /*
> * increment the global active count - control changes to
> * active configurations
> */
> atomic_inc(&cscfg_mgr->sys_active_cnt);
>
> - /*
> - * mark the descriptor as active so enable config on a
> - * device instance will use it
> - */
> - atomic_inc(&config_desc->active_cnt);
> -
> err = 0;
> dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
> break;
> @@ -920,9 +940,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)
>
> list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> - atomic_dec(&config_desc->active_cnt);
> atomic_dec(&cscfg_mgr->sys_active_cnt);
> - cscfg_owner_put(config_desc->load_owner);
> + cscfg_config_desc_put(config_desc);
> dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> break;
> }
> @@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> unsigned long cfg_hash, int preset)
> {
> struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> - const struct cscfg_config_desc *config_desc;
> + struct cscfg_config_desc *config_desc;
> unsigned long flags;
> int err = 0;
>
> @@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> config_desc = config_csdev_item->config_desc;
> - if ((atomic_read(&config_desc->active_cnt)) &&
> - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> + cscfg_config_desc_get(config_desc, true)) {
This obfuscates the logic of the comparisons without good reason. With
the true parameter, the function does no "get" operation but just
replicates the logic being replaced - checking the active_cnt is
non-zero.
Restore this to the original logic to make it readable again
Regards
Mike
> config_csdev_active = config_csdev_item;
> csdev->active_cscfg_ctxt = (void *)config_csdev_active;
> break;
> @@ -1097,7 +1116,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> err = -EBUSY;
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> }
> +
> + if (err)
> + cscfg_config_desc_put(config_desc);
> }
> +
> return err;
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> @@ -1136,8 +1159,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
>
> /* true if there was an enabled active config */
> - if (config_csdev)
> + if (config_csdev) {
> cscfg_csdev_disable_config(config_csdev);
> + cscfg_config_desc_put(config_csdev->config_desc);
> + }
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);
>
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
2025-03-14 14:50 ` Mike Leach
@ 2025-03-14 15:24 ` Yeo Reum Yun
2025-03-21 16:40 ` Mike Leach
0 siblings, 1 reply; 9+ messages in thread
From: Yeo Reum Yun @ 2025-03-14 15:24 UTC (permalink / raw)
To: Mike Leach
Cc: Suzuki Poulose, james.clark@linaro.org,
alexander.shishkin@linux.intel.com, coresight@lists.linaro.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Hi, Mike.
> > static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> > }
> > EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> >
> > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
> > +{
> > + if (enable)
> > + return atomic_inc_not_zero(&config_desc->active_cnt);
> > +
>
> Not sure why we have an "enable" parameter here - it completely
> changes the meaning of the function - with no comment at the start.
Sorry. But what I intended is to distinguish
- activation of config
- enable of activated config.
Because, current coresight doesn't grab the module reference on enable of activate config,
But It grabs that reference only in activation.
That's why I used to "enable" parameter to distinguish this
while I integrate with module_owner count.
> > list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> > if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > - atomic_dec(&config_desc->active_cnt);
> > atomic_dec(&cscfg_mgr->sys_active_cnt);
> > - cscfg_owner_put(config_desc->load_owner);
> > + cscfg_config_desc_put(config_desc);
> > dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> > break;
> > }
> > @@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > unsigned long cfg_hash, int preset)
> > {
> > struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > - const struct cscfg_config_desc *config_desc;
> > + struct cscfg_config_desc *config_desc;
> > unsigned long flags;
> > int err = 0;
> >
> > @@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> > list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> > config_desc = config_csdev_item->config_desc;
> > - if ((atomic_read(&config_desc->active_cnt)) &&
> > - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > + cscfg_config_desc_get(config_desc, true)) {
> >
> This obfuscates the logic of the comparisons without good reason. With
> the true parameter, the function does no "get" operation but just
> replicates the logic being replaced - checking the active_cnt is
> non-zero.
>
> Restore this to the original logic to make it readable again
It's not a replicates of comparsion logic, but if true,
It get the reference of active_cnt but not get module reference.
The fundemental fault in the UAF becase of just "atomic_read()"
so, it should hold reference in here.
So, If you think the cscfg_config_desc_get()'s parameter makes obfuscation,
I think there're two way to modfiy.
1. cscfg_config_desc_get()/put() always grab/drop the module count.
2. remove cscfg_config_desc_get()/put() but just use atomic_XXX(&active_cnt) only
with cscfg_owner_get()/put()
Any thougt?
Thanks!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
2025-03-14 15:24 ` Yeo Reum Yun
@ 2025-03-21 16:40 ` Mike Leach
2025-03-24 13:17 ` Yeoreum Yun
2025-03-24 13:59 ` Yeoreum Yun
0 siblings, 2 replies; 9+ messages in thread
From: Mike Leach @ 2025-03-21 16:40 UTC (permalink / raw)
To: Yeo Reum Yun
Cc: Suzuki Poulose, james.clark@linaro.org,
alexander.shishkin@linux.intel.com, coresight@lists.linaro.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Hi
On Fri, 14 Mar 2025 at 15:25, Yeo Reum Yun <YeoReum.Yun@arm.com> wrote:
>
> Hi, Mike.
>
> > > static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > > @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> > > }
> > > EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> > >
> > > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
> > > +{
> > > + if (enable)
> > > + return atomic_inc_not_zero(&config_desc->active_cnt);
> > > +
> >
> > Not sure why we have an "enable" parameter here - it completely
> > changes the meaning of the function - with no comment at the start.
>
> Sorry. But what I intended is to distinguish
> - activation of config
> - enable of activated config.
> Because, current coresight doesn't grab the module reference on enable of activate config,
> But It grabs that reference only in activation.
> That's why I used to "enable" parameter to distinguish this
> while I integrate with module_owner count.
>
> > > list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> > > if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > > - atomic_dec(&config_desc->active_cnt);
> > > atomic_dec(&cscfg_mgr->sys_active_cnt);
> > > - cscfg_owner_put(config_desc->load_owner);
> > > + cscfg_config_desc_put(config_desc);
> > > dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> > > break;
> > > }
> > > @@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > unsigned long cfg_hash, int preset)
> > > {
> > > struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > > - const struct cscfg_config_desc *config_desc;
> > > + struct cscfg_config_desc *config_desc;
> > > unsigned long flags;
> > > int err = 0;
> > >
> > > @@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> > > list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> > > config_desc = config_csdev_item->config_desc;
> > > - if ((atomic_read(&config_desc->active_cnt)) &&
> > > - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > > + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > > + cscfg_config_desc_get(config_desc, true)) {
> > >
> > This obfuscates the logic of the comparisons without good reason. With
> > the true parameter, the function does no "get" operation but just
> > replicates the logic being replaced - checking the active_cnt is
> > non-zero.
> >
> > Restore this to the original logic to make it readable again
>
> It's not a replicates of comparsion logic, but if true,
sorry - missed that point .
> It get the reference of active_cnt but not get module reference.
> The fundemental fault in the UAF becase of just "atomic_read()"
> so, it should hold reference in here.
>
> So, If you think the cscfg_config_desc_get()'s parameter makes obfuscation,
> I think there're two way to modfiy.
>
> 1. cscfg_config_desc_get()/put() always grab/drop the module count.
> 2. remove cscfg_config_desc_get()/put() but just use atomic_XXX(&active_cnt) only
> with cscfg_owner_get()/put()
>
> Any thougt?
>
> Thanks!
>
>
The get and put functions are asymmetrical w.r.t. owner.
The put will put owner if active count decrements to 0,
The get if not on enable path will put owner unconditionally.
This means that the caller has to work out the correct input conditions.
Might be better if:-
get_desc()
{
if (! desc->refcnt) {
if (!get_owner())
return false;
}
desc->refcnt++;
return true;
}
put_desc()
{
desc->refcnt--;
if (! desc->refcnt)
put_owner()
}
and then change the enable_active_cfg matching logic to
if ( (desc->refcnt) && (desc->hash == hash) && get_desc()) {
< set active cfg>
}
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
2025-03-21 16:40 ` Mike Leach
@ 2025-03-24 13:17 ` Yeoreum Yun
2025-03-24 13:59 ` Yeoreum Yun
1 sibling, 0 replies; 9+ messages in thread
From: Yeoreum Yun @ 2025-03-24 13:17 UTC (permalink / raw)
To: Mike Leach
Cc: Suzuki Poulose, james.clark@linaro.org,
alexander.shishkin@linux.intel.com, coresight@lists.linaro.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Hi Mike,
> > > > static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > > > @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> > > > }
> > > > EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> > > >
> > > > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
> > > > +{
> > > > + if (enable)
> > > > + return atomic_inc_not_zero(&config_desc->active_cnt);
> > > > +
> > >
> > > Not sure why we have an "enable" parameter here - it completely
> > > changes the meaning of the function - with no comment at the start.
> >
> > Sorry. But what I intended is to distinguish
> > - activation of config
> > - enable of activated config.
> > Because, current coresight doesn't grab the module reference on enable of activate config,
> > But It grabs that reference only in activation.
> > That's why I used to "enable" parameter to distinguish this
> > while I integrate with module_owner count.
> >
> > > > list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> > > > if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > > > - atomic_dec(&config_desc->active_cnt);
> > > > atomic_dec(&cscfg_mgr->sys_active_cnt);
> > > > - cscfg_owner_put(config_desc->load_owner);
> > > > + cscfg_config_desc_put(config_desc);
> > > > dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> > > > break;
> > > > }
> > > > @@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > unsigned long cfg_hash, int preset)
> > > > {
> > > > struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > > > - const struct cscfg_config_desc *config_desc;
> > > > + struct cscfg_config_desc *config_desc;
> > > > unsigned long flags;
> > > > int err = 0;
> > > >
> > > > @@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> > > > list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> > > > config_desc = config_csdev_item->config_desc;
> > > > - if ((atomic_read(&config_desc->active_cnt)) &&
> > > > - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > > > + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > > > + cscfg_config_desc_get(config_desc, true)) {
> > > >
> > > This obfuscates the logic of the comparisons without good reason. With
> > > the true parameter, the function does no "get" operation but just
> > > replicates the logic being replaced - checking the active_cnt is
> > > non-zero.
> > >
> > > Restore this to the original logic to make it readable again
> >
> > It's not a replicates of comparsion logic, but if true,
>
> sorry - missed that point .
No worries :) Thanks!
>
> > It get the reference of active_cnt but not get module reference.
> > The fundemental fault in the UAF becase of just "atomic_read()"
> > so, it should hold reference in here.
> >
> > So, If you think the cscfg_config_desc_get()'s parameter makes obfuscation,
> > I think there're two way to modfiy.
> >
> > 1. cscfg_config_desc_get()/put() always grab/drop the module count.
> > 2. remove cscfg_config_desc_get()/put() but just use atomic_XXX(&active_cnt) only
> > with cscfg_owner_get()/put()
> >
> > Any thougt?
> >
> > Thanks!
> >
> >
>
> The get and put functions are asymmetrical w.r.t. owner.
>
> The put will put owner if active count decrements to 0,
> The get if not on enable path will put owner unconditionally.
>
> This means that the caller has to work out the correct input conditions.
>
> Might be better if:-
>
> get_desc()
> {
> if (! desc->refcnt) {
> if (!get_owner())
> return false;
> }
> desc->refcnt++;
> return true;
> }
>
I think it could be with atomic_fetch_add(),
although, it changes the order to get reference count
(config_dec->active_cnt, owner) cscfg_mutex will make sure
live of memory for csdev_config while unloading module
and give a change to get result of get_owner.
Thanks for sharing idea ;)
I'll respin it.
> put_desc()
> {
> desc->refcnt--;
> if (! desc->refcnt)
> put_owner()
> }
>
> and then change the enable_active_cfg matching logic to
>
> if ( (desc->refcnt) && (desc->hash == hash) && get_desc()) {
> < set active cfg>
> }
>
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
2025-03-21 16:40 ` Mike Leach
2025-03-24 13:17 ` Yeoreum Yun
@ 2025-03-24 13:59 ` Yeoreum Yun
2025-03-24 18:21 ` Mike Leach
1 sibling, 1 reply; 9+ messages in thread
From: Yeoreum Yun @ 2025-03-24 13:59 UTC (permalink / raw)
To: Mike Leach
Cc: Suzuki Poulose, james.clark@linaro.org,
alexander.shishkin@linux.intel.com, coresight@lists.linaro.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Hi Mike,
Please ignore my foremer mail.. and please see my comments for your
suggestion.
> Hi
>
> On Fri, 14 Mar 2025 at 15:25, Yeo Reum Yun <YeoReum.Yun@arm.com> wrote:
> >
> > Hi, Mike.
> >
> > > > static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > > > @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> > > > }
> > > > EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> > > >
> > > > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
> > > > +{
> > > > + if (enable)
> > > > + return atomic_inc_not_zero(&config_desc->active_cnt);
> > > > +
> > >
> > > Not sure why we have an "enable" parameter here - it completely
> > > changes the meaning of the function - with no comment at the start.
> >
> > Sorry. But what I intended is to distinguish
> > - activation of config
> > - enable of activated config.
> > Because, current coresight doesn't grab the module reference on enable of activate config,
> > But It grabs that reference only in activation.
> > That's why I used to "enable" parameter to distinguish this
> > while I integrate with module_owner count.
> >
> > > > list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> > > > if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > > > - atomic_dec(&config_desc->active_cnt);
> > > > atomic_dec(&cscfg_mgr->sys_active_cnt);
> > > > - cscfg_owner_put(config_desc->load_owner);
> > > > + cscfg_config_desc_put(config_desc);
> > > > dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> > > > break;
> > > > }
> > > > @@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > unsigned long cfg_hash, int preset)
> > > > {
> > > > struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > > > - const struct cscfg_config_desc *config_desc;
> > > > + struct cscfg_config_desc *config_desc;
> > > > unsigned long flags;
> > > > int err = 0;
> > > >
> > > > @@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> > > > list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> > > > config_desc = config_csdev_item->config_desc;
> > > > - if ((atomic_read(&config_desc->active_cnt)) &&
> > > > - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > > > + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > > > + cscfg_config_desc_get(config_desc, true)) {
> > > >
> > > This obfuscates the logic of the comparisons without good reason. With
> > > the true parameter, the function does no "get" operation but just
> > > replicates the logic being replaced - checking the active_cnt is
> > > non-zero.
> > >
> > > Restore this to the original logic to make it readable again
> >
> > It's not a replicates of comparsion logic, but if true,
>
> sorry - missed that point .
>
> > It get the reference of active_cnt but not get module reference.
> > The fundemental fault in the UAF becase of just "atomic_read()"
> > so, it should hold reference in here.
> >
> > So, If you think the cscfg_config_desc_get()'s parameter makes obfuscation,
> > I think there're two way to modfiy.
> >
> > 1. cscfg_config_desc_get()/put() always grab/drop the module count.
> > 2. remove cscfg_config_desc_get()/put() but just use atomic_XXX(&active_cnt) only
> > with cscfg_owner_get()/put()
> >
> > Any thougt?
> >
> > Thanks!
> >
> >
>
> The get and put functions are asymmetrical w.r.t. owner.
>
> The put will put owner if active count decrements to 0,
> The get if not on enable path will put owner unconditionally.
>
> This means that the caller has to work out the correct input conditions.
>
> Might be better if:-
>
> get_desc()
> {
> if (! desc->refcnt) {
> if (!get_owner())
> return false;
> }
> desc->refcnt++;
> return true;
> }
I think This makes another problem when
it races with _cscfg_deactivate_config().
CPU0 CPU1
(sysfs enable) load module
cscfg_load_config_sets()
activate config. // sysfs
(sys_active_cnt == 1)
// sysfs
_cscfg_deactivate_config()
(sys_active_cnt == 0)
(config->active_cnt = 0)
...
cscfg_csdev_enable_active_config()
lock(csdev->cscfg_csdev_lock)
// here get module reference??
// even sys_active_cnt == 0 and
// config->active_cnt == 1.
get_desc()
unlock(csdev->cscfg_csdev_lock)
// access to config_desc which freed
// while unloading module.
cfs_csdev_enable_config
Because, the desc->refcnt meaning of zero is different from the context.
- while activate . it should get module reference if zero.
- while enable active configuration, if zero, it should be failed.
that means to prevent this race, the core key point is:
when config->active_cnt == 0, it should be failed in cscfg_csdev_enable_active_config()
Because, according to context the handling the zero reference value is
different, It seems,to integrate the get_desc() interface to handle
above case together without extra arguments (in case of here is
"enable").
If this interface is really ugly and unhappy to you,
I think It should remove get_desc()/put_desc().
although we can add new interface for cscfg_config_desc_get() for enable
path. but it makes people more confused.
So my suggestion is:
- sustain this patch's contents
- or remove get_desc()/put_desc() interface but use
atomic_inc_zero(&config_desc->active_cnt) directly in
cscfg_csdev_enable_active_config()
Any thougt?
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
2025-03-24 13:59 ` Yeoreum Yun
@ 2025-03-24 18:21 ` Mike Leach
2025-03-24 19:02 ` Yeoreum Yun
0 siblings, 1 reply; 9+ messages in thread
From: Mike Leach @ 2025-03-24 18:21 UTC (permalink / raw)
To: Yeoreum Yun
Cc: Suzuki Poulose, james.clark@linaro.org,
alexander.shishkin@linux.intel.com, coresight@lists.linaro.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Hi,
On Mon, 24 Mar 2025 at 13:59, Yeoreum Yun <yeoreum.yun@arm.com> wrote:
>
> Hi Mike,
>
> Please ignore my foremer mail.. and please see my comments for your
> suggestion.
>
> > Hi
> >
> > On Fri, 14 Mar 2025 at 15:25, Yeo Reum Yun <YeoReum.Yun@arm.com> wrote:
> > >
> > > Hi, Mike.
> > >
> > > > > static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > > > > @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> > > > >
> > > > > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
> > > > > +{
> > > > > + if (enable)
> > > > > + return atomic_inc_not_zero(&config_desc->active_cnt);
> > > > > +
> > > >
> > > > Not sure why we have an "enable" parameter here - it completely
> > > > changes the meaning of the function - with no comment at the start.
> > >
> > > Sorry. But what I intended is to distinguish
> > > - activation of config
> > > - enable of activated config.
> > > Because, current coresight doesn't grab the module reference on enable of activate config,
> > > But It grabs that reference only in activation.
> > > That's why I used to "enable" parameter to distinguish this
> > > while I integrate with module_owner count.
> > >
> > > > > list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> > > > > if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > > > > - atomic_dec(&config_desc->active_cnt);
> > > > > atomic_dec(&cscfg_mgr->sys_active_cnt);
> > > > > - cscfg_owner_put(config_desc->load_owner);
> > > > > + cscfg_config_desc_put(config_desc);
> > > > > dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> > > > > break;
> > > > > }
> > > > > @@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > > unsigned long cfg_hash, int preset)
> > > > > {
> > > > > struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > > > > - const struct cscfg_config_desc *config_desc;
> > > > > + struct cscfg_config_desc *config_desc;
> > > > > unsigned long flags;
> > > > > int err = 0;
> > > > >
> > > > > @@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > > raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> > > > > list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> > > > > config_desc = config_csdev_item->config_desc;
> > > > > - if ((atomic_read(&config_desc->active_cnt)) &&
> > > > > - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > > > > + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > > > > + cscfg_config_desc_get(config_desc, true)) {
> > > > >
> > > > This obfuscates the logic of the comparisons without good reason. With
> > > > the true parameter, the function does no "get" operation but just
> > > > replicates the logic being replaced - checking the active_cnt is
> > > > non-zero.
> > > >
> > > > Restore this to the original logic to make it readable again
> > >
> > > It's not a replicates of comparsion logic, but if true,
> >
> > sorry - missed that point .
> >
> > > It get the reference of active_cnt but not get module reference.
> > > The fundemental fault in the UAF becase of just "atomic_read()"
> > > so, it should hold reference in here.
> > >
> > > So, If you think the cscfg_config_desc_get()'s parameter makes obfuscation,
> > > I think there're two way to modfiy.
> > >
> > > 1. cscfg_config_desc_get()/put() always grab/drop the module count.
> > > 2. remove cscfg_config_desc_get()/put() but just use atomic_XXX(&active_cnt) only
> > > with cscfg_owner_get()/put()
> > >
> > > Any thougt?
> > >
> > > Thanks!
> > >
> > >
> >
> > The get and put functions are asymmetrical w.r.t. owner.
> >
> > The put will put owner if active count decrements to 0,
> > The get if not on enable path will put owner unconditionally.
> >
> > This means that the caller has to work out the correct input conditions.
> >
> > Might be better if:-
> >
> > get_desc()
> > {
> > if (! desc->refcnt) {
> > if (!get_owner())
> > return false;
> > }
> > desc->refcnt++;
> > return true;
> > }
>
> I think This makes another problem when
> it races with _cscfg_deactivate_config().
>
> CPU0 CPU1
> (sysfs enable) load module
> cscfg_load_config_sets()
> activate config. // sysfs
> (sys_active_cnt == 1)
>
> // sysfs
> _cscfg_deactivate_config()
> (sys_active_cnt == 0)
> (config->active_cnt = 0)
> ...
> cscfg_csdev_enable_active_config()
> lock(csdev->cscfg_csdev_lock)
> // here get module reference??
> // even sys_active_cnt == 0 and
> // config->active_cnt == 1.
> get_desc()
> unlock(csdev->cscfg_csdev_lock)
>
>
> // access to config_desc which freed
> // while unloading module.
> cfs_csdev_enable_config
>
>
> Because, the desc->refcnt meaning of zero is different from the context.
> - while activate . it should get module reference if zero.
> - while enable active configuration, if zero, it should be failed.
>
> that means to prevent this race, the core key point is:
> when config->active_cnt == 0, it should be failed in cscfg_csdev_enable_active_config()
>
This is not a failure case, it simple means that this config should
not be activated for this device.
It is possible for a configuration to be active on the system, without
it being active for a particular coresight device.
Having a get/put interface for the config descriptor - which prevents
the config from being unloaded is fine, the key logic here is that we
are searching a list of possible enabled configurations for this
device and taking the necessary action to enable it if we find one -
and there can only ever be a single configuration enabled for a trace
session.
Therefore when the list of loaded configs for a device is > 1, then
all but one is allowed to be active - so the search code will validly
find instances where config->active_cnt == 0.
My objection to the original interface was not the get/put operations
to protect the module from unload, but the fact that the logic
deciding if a config needed to be enabled on the device was hidden
inside the get() operation.
My suggestion is to restore the logic that decides if there is a
config to enable on the device be clear in the enable function itself,
then use get/put as appropriate to prevent module unload.
Regards
Mike
> Because, according to context the handling the zero reference value is
> different, It seems,to integrate the get_desc() interface to handle
> above case together without extra arguments (in case of here is
> "enable").
>
> If this interface is really ugly and unhappy to you,
> I think It should remove get_desc()/put_desc().
> although we can add new interface for cscfg_config_desc_get() for enable
> path. but it makes people more confused.
>
> So my suggestion is:
> - sustain this patch's contents
> - or remove get_desc()/put_desc() interface but use
> atomic_inc_zero(&config_desc->active_cnt) directly in
> cscfg_csdev_enable_active_config()
>
> Any thougt?
>
> Thanks.
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config
2025-03-24 18:21 ` Mike Leach
@ 2025-03-24 19:02 ` Yeoreum Yun
0 siblings, 0 replies; 9+ messages in thread
From: Yeoreum Yun @ 2025-03-24 19:02 UTC (permalink / raw)
To: Mike Leach
Cc: Suzuki Poulose, james.clark@linaro.org,
alexander.shishkin@linux.intel.com, coresight@lists.linaro.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Hi Mike,
> Hi,
>
> On Mon, 24 Mar 2025 at 13:59, Yeoreum Yun <yeoreum.yun@arm.com> wrote:
> >
> > Hi Mike,
> >
> > Please ignore my foremer mail.. and please see my comments for your
> > suggestion.
> >
> > > Hi
> > >
> > > On Fri, 14 Mar 2025 at 15:25, Yeo Reum Yun <YeoReum.Yun@arm.com> wrote:
> > > >
> > > > Hi, Mike.
> > > >
> > > > > > static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > > > > > @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> > > > > >
> > > > > > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable)
> > > > > > +{
> > > > > > + if (enable)
> > > > > > + return atomic_inc_not_zero(&config_desc->active_cnt);
> > > > > > +
> > > > >
> > > > > Not sure why we have an "enable" parameter here - it completely
> > > > > changes the meaning of the function - with no comment at the start.
> > > >
> > > > Sorry. But what I intended is to distinguish
> > > > - activation of config
> > > > - enable of activated config.
> > > > Because, current coresight doesn't grab the module reference on enable of activate config,
> > > > But It grabs that reference only in activation.
> > > > That's why I used to "enable" parameter to distinguish this
> > > > while I integrate with module_owner count.
> > > >
> > > > > > list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> > > > > > if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > > > > > - atomic_dec(&config_desc->active_cnt);
> > > > > > atomic_dec(&cscfg_mgr->sys_active_cnt);
> > > > > > - cscfg_owner_put(config_desc->load_owner);
> > > > > > + cscfg_config_desc_put(config_desc);
> > > > > > dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> > > > > > break;
> > > > > > }
> > > > > > @@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > > > unsigned long cfg_hash, int preset)
> > > > > > {
> > > > > > struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > > > > > - const struct cscfg_config_desc *config_desc;
> > > > > > + struct cscfg_config_desc *config_desc;
> > > > > > unsigned long flags;
> > > > > > int err = 0;
> > > > > >
> > > > > > @@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > > > > > raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> > > > > > list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> > > > > > config_desc = config_csdev_item->config_desc;
> > > > > > - if ((atomic_read(&config_desc->active_cnt)) &&
> > > > > > - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > > > > > + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > > > > > + cscfg_config_desc_get(config_desc, true)) {
> > > > > >
> > > > > This obfuscates the logic of the comparisons without good reason. With
> > > > > the true parameter, the function does no "get" operation but just
> > > > > replicates the logic being replaced - checking the active_cnt is
> > > > > non-zero.
> > > > >
> > > > > Restore this to the original logic to make it readable again
> > > >
> > > > It's not a replicates of comparsion logic, but if true,
> > >
> > > sorry - missed that point .
> > >
> > > > It get the reference of active_cnt but not get module reference.
> > > > The fundemental fault in the UAF becase of just "atomic_read()"
> > > > so, it should hold reference in here.
> > > >
> > > > So, If you think the cscfg_config_desc_get()'s parameter makes obfuscation,
> > > > I think there're two way to modfiy.
> > > >
> > > > 1. cscfg_config_desc_get()/put() always grab/drop the module count.
> > > > 2. remove cscfg_config_desc_get()/put() but just use atomic_XXX(&active_cnt) only
> > > > with cscfg_owner_get()/put()
> > > >
> > > > Any thougt?
> > > >
> > > > Thanks!
> > > >
> > > >
> > >
> > > The get and put functions are asymmetrical w.r.t. owner.
> > >
> > > The put will put owner if active count decrements to 0,
> > > The get if not on enable path will put owner unconditionally.
> > >
> > > This means that the caller has to work out the correct input conditions.
> > >
> > > Might be better if:-
> > >
> > > get_desc()
> > > {
> > > if (! desc->refcnt) {
> > > if (!get_owner())
> > > return false;
> > > }
> > > desc->refcnt++;
> > > return true;
> > > }
> >
> > I think This makes another problem when
> > it races with _cscfg_deactivate_config().
> >
> > CPU0 CPU1
> > (sysfs enable) load module
> > cscfg_load_config_sets()
> > activate config. // sysfs
> > (sys_active_cnt == 1)
> >
> > // sysfs
> > _cscfg_deactivate_config()
> > (sys_active_cnt == 0)
> > (config->active_cnt = 0)
> > ...
> > cscfg_csdev_enable_active_config()
> > lock(csdev->cscfg_csdev_lock)
> > // here get module reference??
> > // even sys_active_cnt == 0 and
> > // config->active_cnt == 1.
> > get_desc()
> > unlock(csdev->cscfg_csdev_lock)
> >
> >
> > // access to config_desc which freed
> > // while unloading module.
> > cfs_csdev_enable_config
> >
> >
> > Because, the desc->refcnt meaning of zero is different from the context.
> > - while activate . it should get module reference if zero.
> > - while enable active configuration, if zero, it should be failed.
> >
> > that means to prevent this race, the core key point is:
> > when config->active_cnt == 0, it should be failed in cscfg_csdev_enable_active_config()
> >
>
> This is not a failure case, it simple means that this config should
> not be activated for this device.
> It is possible for a configuration to be active on the system, without
> it being active for a particular coresight device.
>
> Having a get/put interface for the config descriptor - which prevents
> the config from being unloaded is fine, the key logic here is that we
> are searching a list of possible enabled configurations for this
> device and taking the necessary action to enable it if we find one -
> and there can only ever be a single configuration enabled for a trace
> session.
>
> Therefore when the list of loaded configs for a device is > 1, then
> all but one is allowed to be active - so the search code will validly
> find instances where config->active_cnt == 0.
>
> My objection to the original interface was not the get/put operations
> to protect the module from unload, but the fact that the logic
> deciding if a config needed to be enabled on the device was hidden
> inside the get() operation.
> My suggestion is to restore the logic that decides if there is a
> config to enable on the device be clear in the enable function itself,
> then use get/put as appropriate to prevent module unload.
>
Make sense. Then I'll respin with the suggestion :)
Thanks for clarification!
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-03-24 19:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-09 17:19 [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config Yeoreum Yun
2025-03-12 6:36 ` Yeo Reum Yun
2025-03-14 14:50 ` Mike Leach
2025-03-14 15:24 ` Yeo Reum Yun
2025-03-21 16:40 ` Mike Leach
2025-03-24 13:17 ` Yeoreum Yun
2025-03-24 13:59 ` Yeoreum Yun
2025-03-24 18:21 ` Mike Leach
2025-03-24 19:02 ` Yeoreum Yun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).