Received-SPF: pass (google.com: domain of linux-kernel+bounces-150431-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 18 Apr 2024 17:55:43 +0200
From: Beata Michalska <beata.michalska@arm.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	ionela.voinescu@arm.com, vanshikonda@os.amperecomputing.com,
	will@kernel.org, catalin.marinas@arm.com,
	vincent.guittot@linaro.org, sumitg@nvidia.com,
	yang@os.amperecomputing.com, lihuisong@huawei.com,
	viresh.kumar@linaro.org
Subject: Re: [PATCH v5 2/5] arm64: amu: Rule out potential use after free
Message-ID: <ZiFCfwEvdJxg6Hth@arm.com>
References: <20240417093848.1555462-1-beata.michalska@arm.com>
 <20240417093848.1555462-3-beata.michalska@arm.com>
 <20240418105052.zvkomz5yeayie4ph@bogus>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240418105052.zvkomz5yeayie4ph@bogus>

On Thu, Apr 18, 2024 at 11:50:52AM +0100, Sudeep Holla wrote:
> On Wed, Apr 17, 2024 at 10:38:45AM +0100, Beata Michalska wrote:
> > For the time being, the amu_fie_cpus cpumask is being exclusively used
> > by the AMU-related internals of FIE support and is guaranteed to be
> > valid on every access currently made. Still the mask is not being
> > invalidated on one of the error handling code paths, which leaves
> > a soft spot with potential risk of uaf for CPUMASK_OFFSTACK cases.
> > To make things sound, set the cpumaks pointer explicitly to NULL upon
> > failing to register the cpufreq notifier.
> > Note that, due to the quirks of CPUMASK_OFFSTACK, this change needs to
> > be wrapped with grim ifdefing (it would be better served by
> > incorporating this into free_cpumask_var ...)
> >
> 
> Yes it doesn't look neat.
> 
> > Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> > ---
> >  arch/arm64/kernel/topology.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index 1a2c72f3e7f8..3c814a278534 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -244,8 +244,12 @@ static int __init init_amu_fie(void)
> >
> >  	ret = cpufreq_register_notifier(&init_amu_fie_notifier,
> >  					CPUFREQ_POLICY_NOTIFIER);
> > -	if (ret)
> > +	if (ret) {
> >  		free_cpumask_var(amu_fie_cpus);
> > +#ifdef CONFIG_CPUMASK_OFFSTACK
> > +		amu_fie_cpus = NULL;
> > +#endif
> > +	}
> 
> Instead of this #ifdeffery, I was wondering if we can actually do the
> allocation in init_amu_fie_callback() the first time it gets called
> checking if amu_fie_cpus is NULL. init_amu_fie_callback() must get called
> only if the cpufreq_register_notifier() succeeds right ?
> 
Delayed allocation ... I guess this will do the trick.
> Also I don't see anyone calling amu_fie_setup(), so where do you think
> the possible use after free could occur for amu_fie_cpus. Just thinking
> out loud to check if I missed anything.
>
You haven't missed anything. Currently the uaf is purely theoretical as the code
that relies on that mask will only be executed if we have succeeded to register
the amu fie support: so far so good.
This change is required for following patches, where that mask is being used to
determine if given CPU has been setup with AMU counters, and as such there needs
to be a safe way to validate it (at any time) by arch_freq_get_on_cpu and
arch_cpu_idle_enter (patches 3/5 & 4/5): both will be called from outside the
FIE.  Without this change, if cpufreq_register_notifier fails in init_amu_fie,
we will have amu_fie_cpus pointing to released memory.

Hope that makes things more clear.

---
BR
Beata
> --
> Regards,
> Sudeep