Re: [PATCH v2] coresight: prevent deactivate active config while enable the config

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

Hi Suzuki,

> Hi Levi
>
> On 23/12/2024 18:53, 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
> >
> > To address this, introduce sys_enable_cnt in cscfg_mgr to prevent
> > deactivate while there is enabled configuration.
>
> Thanks for the finding the problem and the detailed description + patch. I
> have some concerns on the fix, please find it below.
>
> >
>
>
> So we have 3 atomic counters now !
> cscfg_mgr->sys_active_cnt  // Global count
> config->active_cnt	   // Per config count,
>
> And another one which this one introduces.
>
> cscfg_mgr->sys_enable_cnt  // ?
>
>
> And config->active_cnt is always ever 0 or 1. i.e., it is not really a
> reference counter at the moment but it indicates whether it is active or
> not. Could  we not use that for tracking the references on the specific
> config ?
>
> i.e., every time we "enable_active_config" atomic_inc(config->active_cnt)
>
> and disable_active_config() always decrements the config. These could be
> wrapped in cscfg_get() cscfg_put() which would, inc/dec the refcounts
> and also drop the "module" reference when the active_cnt == 0

This action is done via _cscfg_activate_config() already but its
activation is done via "sysfs".
and the checking active_cnt, I think it would increase lots of complex.
because, if so, it should iterate all config in each csdev.
So, I believe it is the reason why the activation and module_cnt get via "sysfs"
to prevent iterating every config in csdev when config unload.

although, active_cnt in each config added to list in csdev be 0 or 1,
the module could be >= 1 (by sum of active_cnt which have the same
module owner). So I'm skeptical to use active_cnt like "reference cnt"
and That's why I decide to use "sys_enable_cnt"
>
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > ---
> > from v1 to v2:
> >      - modify commit message.
> > ---
> >   .../hwtracing/coresight/coresight-etm4x-core.c |  3 +++
> >   drivers/hwtracing/coresight/coresight-syscfg.c | 18 ++++++++++++++++--
> >   drivers/hwtracing/coresight/coresight-syscfg.h |  2 ++
> >   3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index 86893115df17..6218ef40acbc 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -986,6 +986,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);
>
> This looks like a separate "fix" from what you are trying to address. Please
> could split this ?

I don't think so, because without this calling, the "sys_enable_cnt"
never down, It makes error.

> Also, would like to hear what Mike has to say about this change.

IIRC, I followed his suggestion.

Thanks!