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

[Yeoreum Yun <yeoreum.yun@arm.com>]

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 .

No worries :) Thanks!

>
> > 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 it could be with atomic_fetch_add(),
although, it changes the order to get reference count
(config_dec->active_cnt, owner) cscfg_mutex will make sure
live of memory for csdev_config while unloading module
and give a change to get result of get_owner.

Thanks for sharing idea ;)

I'll respin it.

> 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