| Message ID | 20241218084833.609876-1-yeoreum.yun@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] coresight: prevent deactivate active config while enable the config | expand |
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;
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; >
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.
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.
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.
Gentle ping in case of forgotten.
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} >
On 06/01/2025 13:41, Yeo Reum Yun wrote: > Hi Suzuki, > > Sorry this ping was my mistake. > As you said, v2 is the recent one. > You can ignore this. > Thanks, will respond on v2. Also, fyi, your emails not plain text ! Please fix your email client. Suzuki > Thanks! > ------------------------------------------------------------------------ > *보낸 사람:* Suzuki K Poulose <suzuki.poulose@arm.com> > *보낸 날짜:* Monday, January 6, 2025 1:37:40 PM > *받는 사람:* Yeo Reum Yun <YeoReum.Yun@arm.com>; mike.leach@linaro.org > <mike.leach@linaro.org>; james.clark@linaro.org > <james.clark@linaro.org>; alexander.shishkin@linux.intel.com > <alexander.shishkin@linux.intel.com> > *참조:* coresight@lists.linaro.org <coresight@lists.linaro.org>; linux- > arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; > linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; nd <nd@arm.com> > *제목:* Re: [PATCH 1/1] coresight: prevent deactivate active config > while enable the config > 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} >> >
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;
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(-)