Hi,
On Mon, 24 Mar 2025 at 13:59, Yeoreum Yun yeoreum.yun@arm.com wrote:
Hi Mike,
Please ignore my foremer mail.. and please see my comments for your suggestion.
Hi
On Fri, 14 Mar 2025 at 15:25, Yeo Reum Yun YeoReum.Yun@arm.com wrote:
Hi, Mike.
static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner) @@ -867,6 +870,28 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev) } EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
+static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc, bool enable) +{
if (enable)return atomic_inc_not_zero(&config_desc->active_cnt);Not sure why we have an "enable" parameter here - it completely changes the meaning of the function - with no comment at the start.
Sorry. But what I intended is to distinguish - activation of config - enable of activated config. Because, current coresight doesn't grab the module reference on enable of activate config, But It grabs that reference only in activation. That's why I used to "enable" parameter to distinguish this while I integrate with module_owner count.
list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) { if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
atomic_dec(&config_desc->active_cnt); atomic_dec(&cscfg_mgr->sys_active_cnt);cscfg_owner_put(config_desc->load_owner);
cscfg_config_desc_put(config_desc); dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name); break; }@@ -1047,7 +1066,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev, unsigned long cfg_hash, int preset) { struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
const struct cscfg_config_desc *config_desc;
struct cscfg_config_desc *config_desc; unsigned long flags; int err = 0;@@ -1062,8 +1081,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev, raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags); list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) { config_desc = config_csdev_item->config_desc;
if ((atomic_read(&config_desc->active_cnt)) &&((unsigned long)config_desc->event_ea->var == cfg_hash)) {
if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&cscfg_config_desc_get(config_desc, true)) {This obfuscates the logic of the comparisons without good reason. With the true parameter, the function does no "get" operation but just replicates the logic being replaced - checking the active_cnt is non-zero.
Restore this to the original logic to make it readable again
It's not a replicates of comparsion logic, but if true,
sorry - missed that point .
It get the reference of active_cnt but not get module reference. The fundemental fault in the UAF becase of just "atomic_read()" so, it should hold reference in here.
So, If you think the cscfg_config_desc_get()'s parameter makes obfuscation, I think there're two way to modfiy.
1. cscfg_config_desc_get()/put() always grab/drop the module count. 2. remove cscfg_config_desc_get()/put() but just use atomic_XXX(&active_cnt) only with cscfg_owner_get()/put()Any thougt?
Thanks!
The get and put functions are asymmetrical w.r.t. owner.
The put will put owner if active count decrements to 0, The get if not on enable path will put owner unconditionally.
This means that the caller has to work out the correct input conditions.
Might be better if:-
get_desc() { if (! desc->refcnt) { if (!get_owner()) return false; } desc->refcnt++; return true; }
I think This makes another problem when it races with _cscfg_deactivate_config().
CPU0 CPU1 (sysfs enable) load module cscfg_load_config_sets() activate config. // sysfs (sys_active_cnt == 1)
// sysfs _cscfg_deactivate_config() (sys_active_cnt == 0) (config->active_cnt = 0)... cscfg_csdev_enable_active_config() lock(csdev->cscfg_csdev_lock) // here get module reference?? // even sys_active_cnt == 0 and // config->active_cnt == 1. get_desc() unlock(csdev->cscfg_csdev_lock)
// access to config_desc which freed // while unloading module. cfs_csdev_enable_config
Because, the desc->refcnt meaning of zero is different from the context.
- while activate . it should get module reference if zero.
- while enable active configuration, if zero, it should be failed.
that means to prevent this race, the core key point is: when config->active_cnt == 0, it should be failed in cscfg_csdev_enable_active_config()
This is not a failure case, it simple means that this config should not be activated for this device. It is possible for a configuration to be active on the system, without it being active for a particular coresight device.
Having a get/put interface for the config descriptor - which prevents the config from being unloaded is fine, the key logic here is that we are searching a list of possible enabled configurations for this device and taking the necessary action to enable it if we find one - and there can only ever be a single configuration enabled for a trace session.
Therefore when the list of loaded configs for a device is > 1, then all but one is allowed to be active - so the search code will validly find instances where config->active_cnt == 0.
My objection to the original interface was not the get/put operations to protect the module from unload, but the fact that the logic deciding if a config needed to be enabled on the device was hidden inside the get() operation. My suggestion is to restore the logic that decides if there is a config to enable on the device be clear in the enable function itself, then use get/put as appropriate to prevent module unload.
Regards
Mike
Because, according to context the handling the zero reference value is different, It seems,to integrate the get_desc() interface to handle above case together without extra arguments (in case of here is "enable").
If this interface is really ugly and unhappy to you, I think It should remove get_desc()/put_desc(). although we can add new interface for cscfg_config_desc_get() for enable path. but it makes people more confused.
So my suggestion is:
- sustain this patch's contents
- or remove get_desc()/put_desc() interface but use atomic_inc_zero(&config_desc->active_cnt) directly in cscfg_csdev_enable_active_config()
Any thougt?
Thanks.