* [PATCH v2] coresight: prevent deactivate active config while enable the config
@ 2024-12-23 18:53 Yeoreum Yun
2024-12-31 14:41 ` Yeo Reum Yun
2025-01-07 12:07 ` Suzuki K Poulose
0 siblings, 2 replies; 6+ messages in thread
From: Yeoreum Yun @ 2024-12-23 18:53 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, introduce sys_enable_cnt in cscfg_mgr to prevent
deactivate while there is enabled configuration.
Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
from v1 to v2:
- modify commit message.
---
.../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] 6+ messages in thread* Re: [PATCH v2] coresight: prevent deactivate active config while enable the config 2024-12-23 18:53 [PATCH v2] coresight: prevent deactivate active config while enable the config Yeoreum Yun @ 2024-12-31 14:41 ` Yeo Reum Yun 2025-01-07 12:07 ` Suzuki K Poulose 1 sibling, 0 replies; 6+ messages in thread From: Yeo Reum Yun @ 2024-12-31 14:41 UTC (permalink / raw) To: Suzuki Poulose, mike.leach, james.clark, alexander.shishkin Cc: coresight, linux-arm-kernel, linux-kernel Gentle ping in case of forgotten. ________________________________________ From: Yeoreum Yun <yeoreum.yun@arm.com> Sent: 23 December 2024 18:53 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 v2] 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 (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> --- from v1 to v2: - modify commit message. --- .../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] 6+ messages in thread
* Re: [PATCH v2] coresight: prevent deactivate active config while enable the config 2024-12-23 18:53 [PATCH v2] coresight: prevent deactivate active config while enable the config Yeoreum Yun 2024-12-31 14:41 ` Yeo Reum Yun @ 2025-01-07 12:07 ` Suzuki K Poulose 2025-01-07 13:01 ` Yeoreum Yun 1 sibling, 1 reply; 6+ messages in thread From: Suzuki K Poulose @ 2025-01-07 12:07 UTC (permalink / raw) To: Yeoreum Yun, mike.leach, james.clark, alexander.shishkin Cc: coresight, linux-arm-kernel, linux-kernel Hi Levi On 23/12/2024 18:53, 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 > (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. Thanks for the finding the problem and the detailed description + patch. I have some concerns on the fix, please find it below. > So we have 3 atomic counters now ! cscfg_mgr->sys_active_cnt // Global count config->active_cnt // Per config count, And another one which this one introduces. cscfg_mgr->sys_enable_cnt // ? And config->active_cnt is always ever 0 or 1. i.e., it is not really a reference counter at the moment but it indicates whether it is active or not. Could we not use that for tracking the references on the specific config ? i.e., every time we "enable_active_config" atomic_inc(config->active_cnt) and disable_active_config() always decrements the config. These could be wrapped in cscfg_get() cscfg_put() which would, inc/dec the refcounts and also drop the "module" reference when the active_cnt == 0 > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com> > --- > from v1 to v2: > - modify commit message. > --- > .../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); This looks like a separate "fix" from what you are trying to address. Please could split this ? Also, would like to hear what Mike has to say about this change. Suzuki > + > 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] 6+ messages in thread
* Re: [PATCH v2] coresight: prevent deactivate active config while enable the config 2025-01-07 12:07 ` Suzuki K Poulose @ 2025-01-07 13:01 ` Yeoreum Yun 2025-01-08 11:08 ` Suzuki K Poulose 0 siblings, 1 reply; 6+ messages in thread From: Yeoreum Yun @ 2025-01-07 13:01 UTC (permalink / raw) To: Suzuki K Poulose Cc: mike.leach, james.clark, alexander.shishkin, coresight, linux-arm-kernel, linux-kernel Hi Suzuki, > Hi Levi > > On 23/12/2024 18:53, 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 > > (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. > > Thanks for the finding the problem and the detailed description + patch. I > have some concerns on the fix, please find it below. > > > > > > So we have 3 atomic counters now ! > cscfg_mgr->sys_active_cnt // Global count > config->active_cnt // Per config count, > > And another one which this one introduces. > > cscfg_mgr->sys_enable_cnt // ? > > > And config->active_cnt is always ever 0 or 1. i.e., it is not really a > reference counter at the moment but it indicates whether it is active or > not. Could we not use that for tracking the references on the specific > config ? > > i.e., every time we "enable_active_config" atomic_inc(config->active_cnt) > > and disable_active_config() always decrements the config. These could be > wrapped in cscfg_get() cscfg_put() which would, inc/dec the refcounts > and also drop the "module" reference when the active_cnt == 0 This action is done via _cscfg_activate_config() already but its activation is done via "sysfs". and the checking active_cnt, I think it would increase lots of complex. because, if so, it should iterate all config in each csdev. So, I believe it is the reason why the activation and module_cnt get via "sysfs" to prevent iterating every config in csdev when config unload. although, active_cnt in each config added to list in csdev be 0 or 1, the module could be >= 1 (by sum of active_cnt which have the same module owner). So I'm skeptical to use active_cnt like "reference cnt" and That's why I decide to use "sys_enable_cnt" > > > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com> > > --- > > from v1 to v2: > > - modify commit message. > > --- > > .../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); > > This looks like a separate "fix" from what you are trying to address. Please > could split this ? I don't think so, because without this calling, the "sys_enable_cnt" never down, It makes error. > Also, would like to hear what Mike has to say about this change. IIRC, I followed his suggestion. Thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] coresight: prevent deactivate active config while enable the config 2025-01-07 13:01 ` Yeoreum Yun @ 2025-01-08 11:08 ` Suzuki K Poulose 2025-01-08 12:08 ` Yeoreum Yun 0 siblings, 1 reply; 6+ messages in thread From: Suzuki K Poulose @ 2025-01-08 11:08 UTC (permalink / raw) To: Yeoreum Yun Cc: mike.leach, james.clark, alexander.shishkin, coresight, linux-arm-kernel, linux-kernel On 07/01/2025 13:01, Yeoreum Yun wrote: > Hi Suzuki, > >> Hi Levi >> >> On 23/12/2024 18:53, 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 >>> (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. >> >> Thanks for the finding the problem and the detailed description + patch. I >> have some concerns on the fix, please find it below. >> >>> >> >> >> So we have 3 atomic counters now ! >> cscfg_mgr->sys_active_cnt // Global count >> config->active_cnt // Per config count, >> >> And another one which this one introduces. >> >> cscfg_mgr->sys_enable_cnt // ? >> >> >> And config->active_cnt is always ever 0 or 1. i.e., it is not really a >> reference counter at the moment but it indicates whether it is active or >> not. Could we not use that for tracking the references on the specific >> config ? >> >> i.e., every time we "enable_active_config" atomic_inc(config->active_cnt) >> >> and disable_active_config() always decrements the config. These could be >> wrapped in cscfg_get() cscfg_put() which would, inc/dec the refcounts >> and also drop the "module" reference when the active_cnt == 0 > > This action is done via _cscfg_activate_config() already but its > activation is done via "sysfs". > and the checking active_cnt, I think it would increase lots of complex. I don't understand. We have this today : cscfg_config_desc which is getting de-activated. This has reference to the module (owner). If someone is using this config_desc in running session (be it perf or sysfg), it must be refcounted. As such I don't see that the cscfg is refcounted anywhere. (The active_cnt indicates if this has been activated, but not used in a session). If we have a refcount on the "cscfg_config_desc" for each active session, we could prevent/delay unloading the module until the last one drops. Something like this is what I was checking ? coresight: cscfg: Hold refcount on the config for active sessions Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- .../hwtracing/coresight/coresight-config.h | 2 +- .../hwtracing/coresight/coresight-syscfg.c | 24 +++++++++++++------ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h index 6ba013975741..84cdde6f0e4d 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-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index 11138a9762b0..53baeaaf907f 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -914,15 +914,20 @@ static int _cscfg_activate_config(unsigned long cfg_hash) return err; } +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); +} + static void _cscfg_deactivate_config(unsigned long cfg_hash) { struct cscfg_config_desc *config_desc; 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 +1052,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 +1067,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev, 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 && + atomic_inc_not_zero(&config_desc->active_cnt)) { config_csdev_active = config_csdev_item; csdev->active_cscfg_ctxt = (void *)config_csdev_active; break; @@ -1091,12 +1096,15 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev, * Set enabled if OK, err if not. */ spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags); - if (csdev->active_cscfg_ctxt) + if (csdev->active_cscfg_ctxt == config_csdev_active) config_csdev_active->enabled = true; else err = -EBUSY; spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags); } + if (err) + cscfg_config_desc_put(config_desc); + } return err; } @@ -1136,8 +1144,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev) 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); -- > because, if so, it should iterate all config in each csdev. > So, I believe it is the reason why the activation and module_cnt get via "sysfs" > to prevent iterating every config in csdev when config unload. > > although, active_cnt in each config added to list in csdev be 0 or 1, > the module could be >= 1 (by sum of active_cnt which have the same > module owner). So I'm skeptical to use active_cnt like "reference cnt" > and That's why I decide to use "sys_enable_cnt" >> >>> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com> >>> --- >>> from v1 to v2: >>> - modify commit message. >>> --- >>> .../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); >> >> This looks like a separate "fix" from what you are trying to address. Please >> could split this ? > > I don't think so, because without this calling, the "sys_enable_cnt" > never down, It makes error. My point is, we need to "disable_active_config" irrespective of this race, in the sysfs path. So this should be a fix apart from fixing the race. Suzuki > >> Also, would like to hear what Mike has to say about this change. > > IIRC, I followed his suggestion. > > Thanks! > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] coresight: prevent deactivate active config while enable the config 2025-01-08 11:08 ` Suzuki K Poulose @ 2025-01-08 12:08 ` Yeoreum Yun 0 siblings, 0 replies; 6+ messages in thread From: Yeoreum Yun @ 2025-01-08 12:08 UTC (permalink / raw) To: Suzuki K Poulose Cc: mike.leach, james.clark, alexander.shishkin, coresight, linux-arm-kernel, linux-kernel Hi Suzuki, > On 07/01/2025 13:01, Yeoreum Yun wrote: > > Hi Suzuki, > > > > > Hi Levi > > > > > > On 23/12/2024 18:53, 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 > > > > (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. > > > > > > Thanks for the finding the problem and the detailed description + patch. I > > > have some concerns on the fix, please find it below. > > > > > > > > > > > > > > > > So we have 3 atomic counters now ! > > > cscfg_mgr->sys_active_cnt // Global count > > > config->active_cnt // Per config count, > > > > > > And another one which this one introduces. > > > > > > cscfg_mgr->sys_enable_cnt // ? > > > > > > > > > And config->active_cnt is always ever 0 or 1. i.e., it is not really a > > > reference counter at the moment but it indicates whether it is active or > > > not. Could we not use that for tracking the references on the specific > > > config ? > > > > > > i.e., every time we "enable_active_config" atomic_inc(config->active_cnt) > > > > > > and disable_active_config() always decrements the config. These could be > > > wrapped in cscfg_get() cscfg_put() which would, inc/dec the refcounts > > > and also drop the "module" reference when the active_cnt == 0 > > > > This action is done via _cscfg_activate_config() already but its > > activation is done via "sysfs". > > and the checking active_cnt, I think it would increase lots of complex. > > I don't understand. We have this today : > > cscfg_config_desc which is getting de-activated. This has reference to > the module (owner). If someone is using this config_desc in running session > (be it perf or sysfg), it must be refcounted. As such I don't > see that the cscfg is refcounted anywhere. (The active_cnt indicates if this > has been activated, but not used in a session). If we have a refcount on the > "cscfg_config_desc" for each active session, we could > prevent/delay unloading the module until the last one drops. > > > > > Something like this is what I was checking ? Thanks for your code. I understand what you mean now. But I think below code would need to add together: diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index 11138a9762b0..fe1adcf45f06 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; + spin_lock_irqsave(&csdev->cscfg_csdv_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); } + spin_unlock_irqrestore(&csdev->cscfg_csdv_lock, flags); } Otherwise, If atomic_inc_not_zero(&config_desc->active_cnt) failed and cscfg_remove_owned_csdev_configs() is called as part of unloading module before list_next_entry(), it could see LIST_POISON1. Do you mind to send patch again with your suggestion including above patch too? Thanks. > > > coresight: cscfg: Hold refcount on the config for active > sessions > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > .../hwtracing/coresight/coresight-config.h | 2 +- > .../hwtracing/coresight/coresight-syscfg.c | 24 +++++++++++++------ > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-config.h > b/drivers/hwtracing/coresight/coresight-config.h > index 6ba013975741..84cdde6f0e4d 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-syscfg.c > b/drivers/hwtracing/coresight/coresight-syscfg.c > index 11138a9762b0..53baeaaf907f 100644 > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > @@ -914,15 +914,20 @@ static int _cscfg_activate_config(unsigned long > cfg_hash) > return err; > } > > +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); > +} > + > static void _cscfg_deactivate_config(unsigned long cfg_hash) > { > struct cscfg_config_desc *config_desc; > > 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 +1052,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 +1067,8 @@ int cscfg_csdev_enable_active_config(struct > coresight_device *csdev, > 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 && > + atomic_inc_not_zero(&config_desc->active_cnt)) { > config_csdev_active = config_csdev_item; > csdev->active_cscfg_ctxt = (void *)config_csdev_active; > break; > @@ -1091,12 +1096,15 @@ int cscfg_csdev_enable_active_config(struct > coresight_device *csdev, > * Set enabled if OK, err if not. > */ > spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags); > - if (csdev->active_cscfg_ctxt) > + if (csdev->active_cscfg_ctxt == config_csdev_active) > config_csdev_active->enabled = true; > else > err = -EBUSY; > spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags); > } > + if (err) > + cscfg_config_desc_put(config_desc); > + > } > return err; > } > @@ -1136,8 +1144,10 @@ void cscfg_csdev_disable_active_config(struct > coresight_device *csdev) > 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); > > -- > > > > > > > > > because, if so, it should iterate all config in each csdev. > > So, I believe it is the reason why the activation and module_cnt get via "sysfs" > > to prevent iterating every config in csdev when config unload. > > > > although, active_cnt in each config added to list in csdev be 0 or 1, > > the module could be >= 1 (by sum of active_cnt which have the same > > module owner). So I'm skeptical to use active_cnt like "reference cnt" > > and That's why I decide to use "sys_enable_cnt" > > > > > > > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com> > > > > --- > > > > from v1 to v2: > > > > - modify commit message. > > > > --- > > > > .../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); > > > > > > This looks like a separate "fix" from what you are trying to address. Please > > > could split this ? > > > > I don't think so, because without this calling, the "sys_enable_cnt" > > never down, It makes error. > > My point is, we need to "disable_active_config" irrespective of this race, > in the sysfs path. So this should be a fix apart from fixing the race. > > Suzuki > > > > > > > > Also, would like to hear what Mike has to say about this change. > > > > IIRC, I followed his suggestion. > > > > Thanks! > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-08 12:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-12-23 18:53 [PATCH v2] coresight: prevent deactivate active config while enable the config Yeoreum Yun 2024-12-31 14:41 ` Yeo Reum Yun 2025-01-07 12:07 ` Suzuki K Poulose 2025-01-07 13:01 ` Yeoreum Yun 2025-01-08 11:08 ` Suzuki K Poulose 2025-01-08 12:08 ` Yeoreum Yun
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome