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: <CAJ9a7Vgz+L+UYf8Yqyu9J5hp3AB3WPSKutA4AvR-OFdu8b-dPA@mail.gmail.com>
Date: Fri, 21 Mar 2025 16:40:53 +0000
From: Mike Leach <mike.leach@...aro.org>
To: Yeo Reum Yun <YeoReum.Yun@....com>
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

On Fri, 14 Mar 2025 at 15:25, Yeo Reum Yun <YeoReum.Yun@....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;
}

put_desc()
{
   desc->refcnt--;
  if (! desc->refcnt)
    put_owner()
}

and then change the enable_active_cfg matching logic to

if ( (desc->refcnt) && (desc->hash == hash) && get_desc()) {
     < set active cfg>
}



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK