| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250325151803.GD604566@e132581.arm.com>
Date: Tue, 25 Mar 2025 15:18:03 +0000
From: Leo Yan <leo.yan@....com>
To: Yeoreum Yun <yeoreum.yun@....com>
Cc: suzuki.poulose@....com, mike.leach@...aro.org, james.clark@...aro.org,
alexander.shishkin@...ux.intel.com, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] coresight: prevent deactivate active config while
enabling the config
Hi Levi,
On Mon, Mar 24, 2025 at 07:17:40PM +0000, 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
I am not sure if this flow can happen. CoreSight configfs feature is
integrated into the CoreSight core layer, and the other CoreSight
modules are dependent on it.
For example, if the ETM4x module is not removed, the kernel module
management will natually prevent the CoreSight core module from being
removed.
> To address this, use cscfg_config_desc's active_cnt as a reference count
> which will be holded when
> - activate the config.
> - enable the activated config.
> and put the module reference when config_active_cnt == 0.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun@....com>
> ---
> Since v3:
> - Remove enable arguments in cscfg_config_desc_get() (from Mike).
> - https://lore.kernel.org/all/20250109171956.3535294-1-yeoreum.yun@arm.com/
> ---
> .../hwtracing/coresight/coresight-config.h | 2 +-
> .../coresight/coresight-etm4x-core.c | 3 ++
> .../hwtracing/coresight/coresight-syscfg.c | 52 +++++++++++++------
> 3 files changed, 41 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index b9ebc9fcfb7f..90fd937d3bd8 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
> * @feats_csdev:references to the device features to enable.
> */
> struct cscfg_config_csdev {
> - const struct cscfg_config_desc *config_desc;
> + struct cscfg_config_desc *config_desc;
> struct coresight_device *csdev;
> bool enabled;
> struct list_head node;
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e5972f16abff..ef96028fa56b 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1020,6 +1020,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);
> +
In general, we need to split changes into several patches if each
addresses a different issue. From my understanding, the change above is
to fix missing to disable config when disable Sysfs mode.
If so, could we use a seperate patch for this change?
> cpus_read_unlock();
>
> /*
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index a70c1454b410..6d8c212ad434 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
> static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
> {
> struct cscfg_config_csdev *config_csdev, *tmp;
> + unsigned long flags;
>
> if (list_empty(&csdev->config_csdev_list))
> return;
>
> + raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
I think we should use spinlock to guard the condition checking
list_empty().
Here the race condition is the 'config_csdev_list' list and
configurations on the list. For atomicity, we should use lock to
protect any operations on the list (read, add, delete, etc).
A side topic, as here it adds locks for protecting 'config_csdev_list',
I am wandering why we do not do the same thing for
'feature_csdev_list' (See cscfg_remove_owned_csdev_features() and
cscfg_get_feat_csdev()).
> list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
> if (config_csdev->config_desc->load_owner == load_owner)
> list_del(&config_csdev->node);
> }
> + raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> }
>
> static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> @@ -867,6 +870,25 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
>
> +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc)
> +{
> + if (!atomic_fetch_inc(&config_desc->active_cnt)) {
> + /* must ensure that config cannot be unloaded in use */
> + if (unlikely(cscfg_owner_get(config_desc->load_owner))) {
> + atomic_dec(&config_desc->active_cnt);
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> +static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
> +{
> + if (!atomic_dec_return(&config_desc->active_cnt))
> + cscfg_owner_put(config_desc->load_owner);
> +}
> +
> /*
> * This activate configuration for either perf or sysfs. Perf can have multiple
> * active configs, selected per event, sysfs is limited to one.
> @@ -890,22 +912,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
> if (config_desc->available == false)
> return -EBUSY;
>
> - /* must ensure that config cannot be unloaded in use */
> - err = cscfg_owner_get(config_desc->load_owner);
> - if (err)
> + if (!cscfg_config_desc_get(config_desc)) {
> + err = -EINVAL;
> break;
> + }
> +
> /*
> * increment the global active count - control changes to
> * active configurations
> */
> atomic_inc(&cscfg_mgr->sys_active_cnt);
Seems to me, it is more reasonable to use 'sys_active_cnt' to acquire
the module reference instead of 'config_desc->active_cnt'. The reason
is 'sys_active_cnt' is a global counter.
> - /*
> - * mark the descriptor as active so enable config on a
> - * device instance will use it
> - */
> - atomic_inc(&config_desc->active_cnt);
> -
> err = 0;
> dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
> break;
> @@ -920,9 +937,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)
>
> list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> - atomic_dec(&config_desc->active_cnt);
> atomic_dec(&cscfg_mgr->sys_active_cnt);
> - cscfg_owner_put(config_desc->load_owner);
> + cscfg_config_desc_put(config_desc);
> dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> break;
> }
> @@ -1047,7 +1063,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 +1078,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> config_desc = config_csdev_item->config_desc;
> - if ((atomic_read(&config_desc->active_cnt)) &&
> - ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> + if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> + cscfg_config_desc_get(config_desc)) {
This seems to me not right. Why a config descriptor is get in multiple
places? One time getting a config descriptor is in
_cscfg_activate_config(), another is at here.
To be honest, I am not clear what is the difference between 'activate'
config and 'enable active' config. Literally, I think we only need to
acquire the config at its creating phase (maybe match to activate
config?).
> config_csdev_active = config_csdev_item;
> csdev->active_cscfg_ctxt = (void *)config_csdev_active;
> break;
> @@ -1097,7 +1113,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> err = -EBUSY;
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> }
> +
> + if (err)
> + cscfg_config_desc_put(config_desc);
> }
> +
> return err;
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> @@ -1136,8 +1156,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
> raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
>
> /* true if there was an enabled active config */
> - if (config_csdev)
> + if (config_csdev) {
> cscfg_csdev_disable_config(config_csdev);
> + cscfg_config_desc_put(config_csdev->config_desc);
> + }
> }
> EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);
>
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>
Powered by blists - more mailing lists