linux-kernel - Re: [PATCH v5 3/3] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aCR4x/+Wp8JoqG9P@e129823.arm.com>
Date: Wed, 14 May 2025 12:04:39 +0100
From: Yeoreum Yun <yeoreum.yun@....com>
To: Leo Yan <leo.yan@....com>
Cc: suzuki.poulose@....com, mike.leach@...aro.org, james.clark@...aro.org,
	alexander.shishkin@...ux.intel.com, coresight@...ts.linaro.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] coresight: prevent deactivate active config while
 enabling the config

Hi Leo,

> On Tue, May 13, 2025 at 06:06:22PM +0100, 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.
> > cscfg_csdev_enable_config
> >
> > To address this, use cscfg_config_desc's active_cnt as a reference count
> >  which will be holded when
> >     - activate the config.
> >     - enable the activated config.
> > and put the module reference when config_active_cnt == 0.
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@....com>
> > ---
> >  .../hwtracing/coresight/coresight-config.h    |  2 +-
> >  .../hwtracing/coresight/coresight-syscfg.c    | 49 +++++++++++++------
> >  2 files changed, 35 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> > index b9ebc9fcfb7f..90fd937d3bd8 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.h
> > +++ b/drivers/hwtracing/coresight/coresight-config.h
> > @@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
> >   * @feats_csdev:references to the device features to enable.
> >   */
> >  struct cscfg_config_csdev {
> > -	const struct cscfg_config_desc *config_desc;
> > +	struct cscfg_config_desc *config_desc;
> >  	struct coresight_device *csdev;
> >  	bool enabled;
> >  	struct list_head node;
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index 5d194b9269f5..6d8c212ad434 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -870,6 +870,25 @@ 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)
>
> I would like to change the return type to int, so the error is handled
> within the function.  As a result, the caller _cscfg_activate_config()
> does not need to explicitly return an error value.

I think it's not good since cscfg_config_desc_get() failed only when
try_module_get() failed and its return type is "bool".
also, If we make defines error type in here it would make different
error code compared before this patch in some function
(i.e) cscfg_csdev_enable_active_config() want to receive -EBUSY when
cscfg_config_desc_get() failed but _cscfg_activate_config() want to
return -EINVAL.

So I think it would be good to sustain current return type.

>
> Otherwise, the patch looks good to me.

Thanks ;)

--
Sincerely,
Yeoreum Yun