* [PATCH 1/1] coresight: prevent deactivate active config while enable the config
@ 2024-12-18 8:48 Yeoreum Yun
2024-12-23 16:03 ` James Clark
2024-12-31 14:37 ` Yeo Reum Yun
0 siblings, 2 replies; 9+ messages in thread
From: Yeoreum Yun @ 2024-12-18 8:48 UTC (permalink / raw)
To: suzuki.poulose, mike.leach, james.clark, alexander.shishkin
Cc: coresight, linux-arm-kernel, linux-kernel, nd, 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
(perf or 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, introduce sys_enable_cnt in cscfg_mgr to prevent
deactivate while there is enabled configuration.
Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
.../hwtracing/coresight/coresight-etm4x-core.c | 3 +++
drivers/hwtracing/coresight/coresight-syscfg.c | 18 ++++++++++++++++--
drivers/hwtracing/coresight/coresight-syscfg.h | 2 ++
3 files changed, 21 insertions(+), 2 deletions(-)
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..dfa7dcbaf25d 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.c
+++ b/drivers/hwtracing/coresight/coresight-syscfg.c
@@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
cscfg_mgr->sysfs_active_config = cfg_hash;
} else {
/* disable if matching current value */
- if (cscfg_mgr->sysfs_active_config == cfg_hash) {
+ if (cscfg_mgr->sysfs_active_config == cfg_hash &&
+ !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
_cscfg_deactivate_config(cfg_hash);
cscfg_mgr->sysfs_active_config = 0;
} else
@@ -1055,6 +1056,12 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
if (!atomic_read(&cscfg_mgr->sys_active_cnt))
return 0;
+ /*
+ * increment sys_enable_cnt first to prevent deactivate the config
+ * while enable active config.
+ */
+ atomic_inc(&cscfg_mgr->sys_enable_cnt);
+
/*
* Look for matching configuration - set the active configuration
* context if found.
@@ -1098,6 +1105,10 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
}
}
+
+ if (!config_csdev_active || err)
+ atomic_dec(&cscfg_mgr->sys_enable_cnt);
+
return err;
}
EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
@@ -1129,8 +1140,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
if (config_csdev) {
if (!config_csdev->enabled)
config_csdev = NULL;
- else
+ else {
config_csdev->enabled = false;
+ atomic_dec(&cscfg_mgr->sys_enable_cnt);
+ }
}
csdev->active_cscfg_ctxt = NULL;
raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
@@ -1179,6 +1192,7 @@ static int cscfg_create_device(void)
INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
atomic_set(&cscfg_mgr->sys_active_cnt, 0);
+ atomic_set(&cscfg_mgr->sys_enable_cnt, 0);
cscfg_mgr->load_state = CSCFG_NONE;
/* setup the device */
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
index 66e2db890d82..2fc397919985 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.h
+++ b/drivers/hwtracing/coresight/coresight-syscfg.h
@@ -38,6 +38,7 @@ enum cscfg_load_ops {
* @config_desc_list: List of system configuration descriptors to load into registered devices.
* @load_order_list: Ordered list of owners for dynamically loaded configurations.
* @sys_active_cnt: Total number of active config descriptor references.
+ * @sys_enable_cnt: Total number of enable of active config descriptor references.
* @cfgfs_subsys: configfs subsystem used to manage configurations.
* @sysfs_active_config:Active config hash used if CoreSight controlled from sysfs.
* @sysfs_active_preset:Active preset index used if CoreSight controlled from sysfs.
@@ -50,6 +51,7 @@ struct cscfg_manager {
struct list_head config_desc_list;
struct list_head load_order_list;
atomic_t sys_active_cnt;
+ atomic_t sys_enable_cnt;
struct configfs_subsystem cfgfs_subsys;
u32 sysfs_active_config;
int sysfs_active_preset;
--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] coresight: prevent deactivate active config while enable the config
2024-12-18 8:48 [PATCH 1/1] coresight: prevent deactivate active config while enable the config Yeoreum Yun
@ 2024-12-23 16:03 ` James Clark
2024-12-23 18:29 ` Yeoreum Yun
2024-12-24 10:13 ` Yeoreum Yun
2024-12-31 14:37 ` Yeo Reum Yun
1 sibling, 2 replies; 9+ messages in thread
From: James Clark @ 2024-12-23 16:03 UTC (permalink / raw)
To: Yeoreum Yun, Mike Leach
Cc: coresight, linux-arm-kernel, linux-kernel, nd, suzuki.poulose,
mike.leach, alexander.shishkin
On 18/12/2024 8:48 am, Yeoreum Yun 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
> (perf or 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)
Assuming the left side does Perf, are there some steps missing? To get
to enable_active_config() you first need to pass through etm_setup_aux()
-> cscfg_activate_config(). That would also increment sys_active_cnt
which would leave it at 2 if there were two concurrent sessions. Then it
would end up as 1 here after deactivate, rather than 0.
It's not explicitly mentioned in the sequence but I'm assuming the left
and right are the same config, but I suppose it could be an issue with
different configs too.
> cscfg_unload_config_sets()
> unload module
On the left cscfg_activate_config() also bumps the module refcount, so
unload wouldn't cause a UAF here as far as I can see.
>
> // access to config_desc which freed
> // while unloading module.
> cfs_csdev_enable_config
>
> To address this, introduce sys_enable_cnt in cscfg_mgr to prevent
> deactivate while there is enabled configuration.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---
> .../hwtracing/coresight/coresight-etm4x-core.c | 3 +++
> drivers/hwtracing/coresight/coresight-syscfg.c | 18 ++++++++++++++++--
> drivers/hwtracing/coresight/coresight-syscfg.h | 2 ++
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
> 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..dfa7dcbaf25d 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
> cscfg_mgr->sysfs_active_config = cfg_hash;
> } else {
> /* disable if matching current value */
> - if (cscfg_mgr->sysfs_active_config == cfg_hash) {
> + if (cscfg_mgr->sysfs_active_config == cfg_hash &&
> + !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
> _cscfg_deactivate_config(cfg_hash);
So is sys_enable_cnt a global value? If a fix is needed doesn't it need
to be a per-config refcount?
Say you have two active configs, sys_enable_cnt is now 2, how do you
disable one without it always skipping here when the other config is
enabled?
> cscfg_mgr->sysfs_active_config = 0;
> } else
> @@ -1055,6 +1056,12 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> if (!atomic_read(&cscfg_mgr->sys_active_cnt))
> return 0;
>
> + /*
> + * increment sys_enable_cnt first to prevent deactivate the config
> + * while enable active config.
> + */
> + atomic_inc(&cscfg_mgr->sys_enable_cnt);
> +
> /*
> * Look for matching configuration - set the active configuration
> * context if found.
> @@ -1098,6 +1105,10 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> }
> }
> +
> + if (!config_csdev_active || err)
> + atomic_dec(&cscfg_mgr->sys_enable_cnt);
> +
> return err;
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> @@ -1129,8 +1140,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
> if (config_csdev) {
> if (!config_csdev->enabled)
> config_csdev = NULL;
> - else
> + else {
> config_csdev->enabled = false;
> + atomic_dec(&cscfg_mgr->sys_enable_cnt);
> + }
> }
> csdev->active_cscfg_ctxt = NULL;
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> @@ -1179,6 +1192,7 @@ static int cscfg_create_device(void)
> INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
> INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
> atomic_set(&cscfg_mgr->sys_active_cnt, 0);
> + atomic_set(&cscfg_mgr->sys_enable_cnt, 0);
> cscfg_mgr->load_state = CSCFG_NONE;
>
> /* setup the device */
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> index 66e2db890d82..2fc397919985 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> @@ -38,6 +38,7 @@ enum cscfg_load_ops {
> * @config_desc_list: List of system configuration descriptors to load into registered devices.
> * @load_order_list: Ordered list of owners for dynamically loaded configurations.
> * @sys_active_cnt: Total number of active config descriptor references.
> + * @sys_enable_cnt: Total number of enable of active config descriptor references.
When these are next to each other it makes me wonder why active_cnt
isn't enough to prevent unloading? Enabled is always a subset of active,
so as long as you gate unloads or modifications on the existing active
count it seems fine?
> * @cfgfs_subsys: configfs subsystem used to manage configurations.
> * @sysfs_active_config:Active config hash used if CoreSight controlled from sysfs.
> * @sysfs_active_preset:Active preset index used if CoreSight controlled from sysfs.
> @@ -50,6 +51,7 @@ struct cscfg_manager {
> struct list_head config_desc_list;
> struct list_head load_order_list;
> atomic_t sys_active_cnt;
> + atomic_t sys_enable_cnt;
> struct configfs_subsystem cfgfs_subsys;
> u32 sysfs_active_config;
> int sysfs_active_preset;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] coresight: prevent deactivate active config while enable the config
2024-12-23 16:03 ` James Clark
@ 2024-12-23 18:29 ` Yeoreum Yun
2024-12-24 10:13 ` Yeoreum Yun
1 sibling, 0 replies; 9+ messages in thread
From: Yeoreum Yun @ 2024-12-23 18:29 UTC (permalink / raw)
To: James Clark
Cc: Mike Leach, coresight, linux-arm-kernel, linux-kernel, nd,
suzuki.poulose, alexander.shishkin
Hi James.
>
>
> On 18/12/2024 8:48 am, Yeoreum Yun 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
> > (perf or 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)
>
> Assuming the left side does Perf, are there some steps missing? To get to
> enable_active_config() you first need to pass through etm_setup_aux() ->
> cscfg_activate_config(). That would also increment sys_active_cnt which
> would leave it at 2 if there were two concurrent sessions. Then it would end
> up as 1 here after deactivate, rather than 0.
>
> It's not explicitly mentioned in the sequence but I'm assuming the left and
> right are the same config, but I suppose it could be an issue with different
> configs too.
Sorry! This happens only in sysfs mode. not perf.
I've forgotten update the diagram...
>
> > cscfg_unload_config_sets()
> > unload module
>
> On the left cscfg_activate_config() also bumps the module refcount, so
> unload wouldn't cause a UAF here as far as I can see.
>
In case of perf yes. However sysfs is done via configfs interface.
IOW, while cscfg_csdev_enable_active_config() iterating to find out the
config, it's possible to deactivate config via confis interface,
and unload moudle.
Above diagram explains this when coresight is configured with sysfs &
configfs interface NOT perf.
Sorry!
> >
> > // access to config_desc which freed
> > // while unloading module.
> > cfs_csdev_enable_config
> >
> > To address this, introduce sys_enable_cnt in cscfg_mgr to prevent
> > deactivate while there is enabled configuration.
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > ---
> > .../hwtracing/coresight/coresight-etm4x-core.c | 3 +++
> > drivers/hwtracing/coresight/coresight-syscfg.c | 18 ++++++++++++++++--
> > drivers/hwtracing/coresight/coresight-syscfg.h | 2 ++
> > 3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > 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..dfa7dcbaf25d 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
> > cscfg_mgr->sysfs_active_config = cfg_hash;
> > } else {
> > /* disable if matching current value */
> > - if (cscfg_mgr->sysfs_active_config == cfg_hash) {
> > + if (cscfg_mgr->sysfs_active_config == cfg_hash &&
> > + !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
> > _cscfg_deactivate_config(cfg_hash);
>
> So is sys_enable_cnt a global value? If a fix is needed doesn't it need to
> be a per-config refcount?
>
> Say you have two active configs, sys_enable_cnt is now 2, how do you disable
> one without it always skipping here when the other config is enabled?
>
> > cscfg_mgr->sysfs_active_config = 0;
> > } else
> > @@ -1055,6 +1056,12 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > if (!atomic_read(&cscfg_mgr->sys_active_cnt))
> > return 0;
> > + /*
> > + * increment sys_enable_cnt first to prevent deactivate the config
> > + * while enable active config.
> > + */
> > + atomic_inc(&cscfg_mgr->sys_enable_cnt);
> > +
> > /*
> > * Look for matching configuration - set the active configuration
> > * context if found.
> > @@ -1098,6 +1105,10 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> > raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> > }
> > }
> > +
> > + if (!config_csdev_active || err)
> > + atomic_dec(&cscfg_mgr->sys_enable_cnt);
> > +
> > return err;
> > }
> > EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> > @@ -1129,8 +1140,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
> > if (config_csdev) {
> > if (!config_csdev->enabled)
> > config_csdev = NULL;
> > - else
> > + else {
> > config_csdev->enabled = false;
> > + atomic_dec(&cscfg_mgr->sys_enable_cnt);
> > + }
> > }
> > csdev->active_cscfg_ctxt = NULL;
> > raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> > @@ -1179,6 +1192,7 @@ static int cscfg_create_device(void)
> > INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
> > INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
> > atomic_set(&cscfg_mgr->sys_active_cnt, 0);
> > + atomic_set(&cscfg_mgr->sys_enable_cnt, 0);
> > cscfg_mgr->load_state = CSCFG_NONE;
> > /* setup the device */
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> > index 66e2db890d82..2fc397919985 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> > @@ -38,6 +38,7 @@ enum cscfg_load_ops {
> > * @config_desc_list: List of system configuration descriptors to load into registered devices.
> > * @load_order_list: Ordered list of owners for dynamically loaded configurations.
> > * @sys_active_cnt: Total number of active config descriptor references.
> > + * @sys_enable_cnt: Total number of enable of active config descriptor references.
>
> When these are next to each other it makes me wonder why active_cnt isn't
> enough to prevent unloading? Enabled is always a subset of active, so as
> long as you gate unloads or modifications on the existing active count it
> seems fine?
>
> > * @cfgfs_subsys: configfs subsystem used to manage configurations.
> > * @sysfs_active_config:Active config hash used if CoreSight controlled from sysfs.
> > * @sysfs_active_preset:Active preset index used if CoreSight controlled from sysfs.
> > @@ -50,6 +51,7 @@ struct cscfg_manager {
> > struct list_head config_desc_list;
> > struct list_head load_order_list;
> > atomic_t sys_active_cnt;
> > + atomic_t sys_enable_cnt;
> > struct configfs_subsystem cfgfs_subsys;
> > u32 sysfs_active_config;
> > int sysfs_active_preset;
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] coresight: prevent deactivate active config while enable the config
2024-12-23 16:03 ` James Clark
2024-12-23 18:29 ` Yeoreum Yun
@ 2024-12-24 10:13 ` Yeoreum Yun
2024-12-24 11:41 ` James Clark
1 sibling, 1 reply; 9+ messages in thread
From: Yeoreum Yun @ 2024-12-24 10:13 UTC (permalink / raw)
To: James Clark
Cc: Mike Leach, coresight, linux-arm-kernel, linux-kernel, nd,
suzuki.poulose, alexander.shishkin
Hi James.
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index a70c1454b410..dfa7dcbaf25d 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
> > cscfg_mgr->sysfs_active_config = cfg_hash;
> > } else {
> > /* disable if matching current value */
> > - if (cscfg_mgr->sysfs_active_config == cfg_hash) {
> > + if (cscfg_mgr->sysfs_active_config == cfg_hash &&
> > + !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
> > _cscfg_deactivate_config(cfg_hash);
>
> So is sys_enable_cnt a global value? If a fix is needed doesn't it need to
> be a per-config refcount?
>
> Say you have two active configs, sys_enable_cnt is now 2, how do you disable
> one without it always skipping here when the other config is enabled?
Sorry to miss this one!.
Because when one configuration is enabled,
cscfg_mgr->sysfs_active_config becomes !NULL, so it wouldn't happen
there is no two activate configurations. so sys_enable_cnt wouldn't be
2.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] coresight: prevent deactivate active config while enable the config
2024-12-24 10:13 ` Yeoreum Yun
@ 2024-12-24 11:41 ` James Clark
2024-12-24 13:07 ` Yeoreum Yun
0 siblings, 1 reply; 9+ messages in thread
From: James Clark @ 2024-12-24 11:41 UTC (permalink / raw)
To: Yeoreum Yun, Mike Leach
Cc: coresight, linux-arm-kernel, linux-kernel, nd, suzuki.poulose,
alexander.shishkin
On 24/12/2024 10:13 am, Yeoreum Yun wrote:
> Hi James.
>>> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
>>> index a70c1454b410..dfa7dcbaf25d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
>>> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
>>> @@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
>>> cscfg_mgr->sysfs_active_config = cfg_hash;
>>> } else {
>>> /* disable if matching current value */
>>> - if (cscfg_mgr->sysfs_active_config == cfg_hash) {
>>> + if (cscfg_mgr->sysfs_active_config == cfg_hash &&
>>> + !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
>>> _cscfg_deactivate_config(cfg_hash);
>>
>> So is sys_enable_cnt a global value? If a fix is needed doesn't it need to
>> be a per-config refcount?
>>
>> Say you have two active configs, sys_enable_cnt is now 2, how do you disable
>> one without it always skipping here when the other config is enabled?
>
> Sorry to miss this one!.
> Because when one configuration is enabled,
> cscfg_mgr->sysfs_active_config becomes !NULL, so it wouldn't happen
> there is no two activate configurations. so sys_enable_cnt wouldn't be
> 2.
>
>
>
Maybe "sys_enabled" is a better name then. Count implies that it can be
more than one. And the doc could be updated to say it's only ever 0 or 1.
But what about my other point about enabled always being a subset of
active? Can we not change "sys_active_cnt" to a more generic "refcount",
then both activation and enabling steps increment that same refcount,
because they are both technically users of the config. Then you can
solve the problem without adding another separate counter. I think
that's potentially easier to understand.
Although the easiest is just locking every function with the mutex (or a
spinlock if it also needs to be used from Perf). Obviously all these
atomics are harder to get right than that, and this isn't performance
sensitive in any way.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] coresight: prevent deactivate active config while enable the config
2024-12-24 11:41 ` James Clark
@ 2024-12-24 13:07 ` Yeoreum Yun
0 siblings, 0 replies; 9+ messages in thread
From: Yeoreum Yun @ 2024-12-24 13:07 UTC (permalink / raw)
To: James Clark
Cc: Mike Leach, coresight, linux-arm-kernel, linux-kernel, nd,
suzuki.poulose, alexander.shishkin
Hi James,
> > Hi James.
> > > > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > > index a70c1454b410..dfa7dcbaf25d 100644
> > > > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > > > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > > @@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
> > > > cscfg_mgr->sysfs_active_config = cfg_hash;
> > > > } else {
> > > > /* disable if matching current value */
> > > > - if (cscfg_mgr->sysfs_active_config == cfg_hash) {
> > > > + if (cscfg_mgr->sysfs_active_config == cfg_hash &&
> > > > + !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
> > > > _cscfg_deactivate_config(cfg_hash);
> > >
> > > So is sys_enable_cnt a global value? If a fix is needed doesn't it need to
> > > be a per-config refcount?
> > >
> > > Say you have two active configs, sys_enable_cnt is now 2, how do you disable
> > > one without it always skipping here when the other config is enabled?
> >
> > Sorry to miss this one!.
> > Because when one configuration is enabled,
> > cscfg_mgr->sysfs_active_config becomes !NULL, so it wouldn't happen
> > there is no two activate configurations. so sys_enable_cnt wouldn't be
> > 2.
> >
> >
> >
>
> Maybe "sys_enabled" is a better name then. Count implies that it can be more
> than one. And the doc could be updated to say it's only ever 0 or 1.
I think I'm not fully explained. sys_enable_cnt could be more than 2
if it runs with perf and sysfs both.
Because, perf uses cscfg_active_config() to activate configuration.
but, "sysfs" only can activate 1 configuration because it enables with
cscfg_config_sysfs_activate(). so the sys_enable_cnt could be more than
However, sys_enable_cnt can be increased only 1 by sysfs interface.
Maybe It could be hinder to disable by enabled activated by the sysfs.
However, That's wouldn't be critical.
>
> But what about my other point about enabled always being a subset of active?
> Can we not change "sys_active_cnt" to a more generic "refcount", then both
> activation and enabling steps increment that same refcount, because they are
> both technically users of the config. Then you can solve the problem without
> adding another separate counter. I think that's potentially easier to
> understand.
Actually, I think merging this two count (or with module ref too),
seems increasing commplexity right now.
To make clear It would be good However to fix bug for above case,
I think it's enough to add sys_enable_cnt and I think it doesn't loss
its name meaning and it seems more to be backportable.
> Although the easiest is just locking every function with the mutex (or a
> spinlock if it also needs to be used from Perf). Obviously all these atomics
> are harder to get right than that, and this isn't performance sensitive in
> any way.
Agree, That's why there's one option to merge cscfg_mutex with
coresight_mutex. But I think this is too much to fix this problem.
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] coresight: prevent deactivate active config while enable the config
2024-12-18 8:48 [PATCH 1/1] coresight: prevent deactivate active config while enable the config Yeoreum Yun
2024-12-23 16:03 ` James Clark
@ 2024-12-31 14:37 ` Yeo Reum Yun
2025-01-06 13:37 ` Suzuki K Poulose
1 sibling, 1 reply; 9+ messages in thread
From: Yeo Reum Yun @ 2024-12-31 14:37 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, nd
Gentle ping in case of forgotten.
________________________________________
From: Yeoreum Yun <yeoreum.yun@arm.com>
Sent: 18 December 2024 08:48
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; nd; Yeo Reum Yun
Subject: [PATCH 1/1] coresight: prevent deactivate active config while enable 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
(perf or 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, introduce sys_enable_cnt in cscfg_mgr to prevent
deactivate while there is enabled configuration.
Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
.../hwtracing/coresight/coresight-etm4x-core.c | 3 +++
drivers/hwtracing/coresight/coresight-syscfg.c | 18 ++++++++++++++++--
drivers/hwtracing/coresight/coresight-syscfg.h | 2 ++
3 files changed, 21 insertions(+), 2 deletions(-)
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..dfa7dcbaf25d 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.c
+++ b/drivers/hwtracing/coresight/coresight-syscfg.c
@@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
cscfg_mgr->sysfs_active_config = cfg_hash;
} else {
/* disable if matching current value */
- if (cscfg_mgr->sysfs_active_config == cfg_hash) {
+ if (cscfg_mgr->sysfs_active_config == cfg_hash &&
+ !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
_cscfg_deactivate_config(cfg_hash);
cscfg_mgr->sysfs_active_config = 0;
} else
@@ -1055,6 +1056,12 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
if (!atomic_read(&cscfg_mgr->sys_active_cnt))
return 0;
+ /*
+ * increment sys_enable_cnt first to prevent deactivate the config
+ * while enable active config.
+ */
+ atomic_inc(&cscfg_mgr->sys_enable_cnt);
+
/*
* Look for matching configuration - set the active configuration
* context if found.
@@ -1098,6 +1105,10 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
}
}
+
+ if (!config_csdev_active || err)
+ atomic_dec(&cscfg_mgr->sys_enable_cnt);
+
return err;
}
EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
@@ -1129,8 +1140,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
if (config_csdev) {
if (!config_csdev->enabled)
config_csdev = NULL;
- else
+ else {
config_csdev->enabled = false;
+ atomic_dec(&cscfg_mgr->sys_enable_cnt);
+ }
}
csdev->active_cscfg_ctxt = NULL;
raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
@@ -1179,6 +1192,7 @@ static int cscfg_create_device(void)
INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
atomic_set(&cscfg_mgr->sys_active_cnt, 0);
+ atomic_set(&cscfg_mgr->sys_enable_cnt, 0);
cscfg_mgr->load_state = CSCFG_NONE;
/* setup the device */
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
index 66e2db890d82..2fc397919985 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.h
+++ b/drivers/hwtracing/coresight/coresight-syscfg.h
@@ -38,6 +38,7 @@ enum cscfg_load_ops {
* @config_desc_list: List of system configuration descriptors to load into registered devices.
* @load_order_list: Ordered list of owners for dynamically loaded configurations.
* @sys_active_cnt: Total number of active config descriptor references.
+ * @sys_enable_cnt: Total number of enable of active config descriptor references.
* @cfgfs_subsys: configfs subsystem used to manage configurations.
* @sysfs_active_config:Active config hash used if CoreSight controlled from sysfs.
* @sysfs_active_preset:Active preset index used if CoreSight controlled from sysfs.
@@ -50,6 +51,7 @@ struct cscfg_manager {
struct list_head config_desc_list;
struct list_head load_order_list;
atomic_t sys_active_cnt;
+ atomic_t sys_enable_cnt;
struct configfs_subsystem cfgfs_subsys;
u32 sysfs_active_config;
int sysfs_active_preset;
--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] coresight: prevent deactivate active config while enable the config
2024-12-31 14:37 ` Yeo Reum Yun
@ 2025-01-06 13:37 ` Suzuki K Poulose
[not found] ` <GV1PR08MB1052189E5AE025F139F0A3526FB102@GV1PR08MB10521.eurprd08.prod.outlook.com>
0 siblings, 1 reply; 9+ messages in thread
From: Suzuki K Poulose @ 2025-01-06 13:37 UTC (permalink / raw)
To: Yeo Reum Yun, 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, nd
Hi Yeo,
On 31/12/2024 14:37, Yeo Reum Yun wrote:
> Gentle ping in case of forgotten.
>
Gentle reminder: Please avoid top posting.
I thought there is a v2 of this one ? I am now confused.
Kind regards
Suzuki
> ________________________________________
> From: Yeoreum Yun <yeoreum.yun@arm.com>
> Sent: 18 December 2024 08:48
> 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; nd; Yeo Reum Yun
> Subject: [PATCH 1/1] coresight: prevent deactivate active config while enable 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
> (perf or 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, introduce sys_enable_cnt in cscfg_mgr to prevent
> deactivate while there is enabled configuration.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---
> .../hwtracing/coresight/coresight-etm4x-core.c | 3 +++
> drivers/hwtracing/coresight/coresight-syscfg.c | 18 ++++++++++++++++--
> drivers/hwtracing/coresight/coresight-syscfg.h | 2 ++
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
> 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..dfa7dcbaf25d 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
> cscfg_mgr->sysfs_active_config = cfg_hash;
> } else {
> /* disable if matching current value */
> - if (cscfg_mgr->sysfs_active_config == cfg_hash) {
> + if (cscfg_mgr->sysfs_active_config == cfg_hash &&
> + !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
> _cscfg_deactivate_config(cfg_hash);
> cscfg_mgr->sysfs_active_config = 0;
> } else
> @@ -1055,6 +1056,12 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> if (!atomic_read(&cscfg_mgr->sys_active_cnt))
> return 0;
>
> + /*
> + * increment sys_enable_cnt first to prevent deactivate the config
> + * while enable active config.
> + */
> + atomic_inc(&cscfg_mgr->sys_enable_cnt);
> +
> /*
> * Look for matching configuration - set the active configuration
> * context if found.
> @@ -1098,6 +1105,10 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> }
> }
> +
> + if (!config_csdev_active || err)
> + atomic_dec(&cscfg_mgr->sys_enable_cnt);
> +
> return err;
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> @@ -1129,8 +1140,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
> if (config_csdev) {
> if (!config_csdev->enabled)
> config_csdev = NULL;
> - else
> + else {
> config_csdev->enabled = false;
> + atomic_dec(&cscfg_mgr->sys_enable_cnt);
> + }
> }
> csdev->active_cscfg_ctxt = NULL;
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> @@ -1179,6 +1192,7 @@ static int cscfg_create_device(void)
> INIT_LIST_HEAD(&cscfg_mgr->config_desc_list);
> INIT_LIST_HEAD(&cscfg_mgr->load_order_list);
> atomic_set(&cscfg_mgr->sys_active_cnt, 0);
> + atomic_set(&cscfg_mgr->sys_enable_cnt, 0);
> cscfg_mgr->load_state = CSCFG_NONE;
>
> /* setup the device */
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.h b/drivers/hwtracing/coresight/coresight-syscfg.h
> index 66e2db890d82..2fc397919985 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.h
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.h
> @@ -38,6 +38,7 @@ enum cscfg_load_ops {
> * @config_desc_list: List of system configuration descriptors to load into registered devices.
> * @load_order_list: Ordered list of owners for dynamically loaded configurations.
> * @sys_active_cnt: Total number of active config descriptor references.
> + * @sys_enable_cnt: Total number of enable of active config descriptor references.
> * @cfgfs_subsys: configfs subsystem used to manage configurations.
> * @sysfs_active_config:Active config hash used if CoreSight controlled from sysfs.
> * @sysfs_active_preset:Active preset index used if CoreSight controlled from sysfs.
> @@ -50,6 +51,7 @@ struct cscfg_manager {
> struct list_head config_desc_list;
> struct list_head load_order_list;
> atomic_t sys_active_cnt;
> + atomic_t sys_enable_cnt;
> struct configfs_subsystem cfgfs_subsys;
> u32 sysfs_active_config;
> int sysfs_active_preset;
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-01-06 15:23 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-18 8:48 [PATCH 1/1] coresight: prevent deactivate active config while enable the config Yeoreum Yun
2024-12-23 16:03 ` James Clark
2024-12-23 18:29 ` Yeoreum Yun
2024-12-24 10:13 ` Yeoreum Yun
2024-12-24 11:41 ` James Clark
2024-12-24 13:07 ` Yeoreum Yun
2024-12-31 14:37 ` Yeo Reum Yun
2025-01-06 13:37 ` Suzuki K Poulose
[not found] ` <GV1PR08MB1052189E5AE025F139F0A3526FB102@GV1PR08MB10521.eurprd08.prod.outlook.com>
2025-01-06 15:23 ` Suzuki K Poulose
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox