linux-kernel - Re: [PATCH v3 1/1] coresight: prevent deactivate active config while enabling the config

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <GV1PR08MB10521BB7C93822F5124F2D66EFBD22@GV1PR08MB10521.eurprd08.prod.outlook.com>
Date: Fri, 14 Mar 2025 15:24:58 +0000
From: Yeo Reum Yun <YeoReum.Yun@....com>
To: Mike Leach <mike.leach@...aro.org>
CC: Suzuki Poulose <Suzuki.Poulose@....com>, "james.clark@...aro.org"
	<james.clark@...aro.org>, "alexander.shishkin@...ux.intel.com"
	<alexander.shishkin@...ux.intel.com>, "coresight@...ts.linaro.org"
	<coresight@...ts.linaro.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/1] coresight: prevent deactivate active config while
 enabling the config

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,
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!