| Message ID | 20240417093848.1555462-3-beata.michalska@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for AArch64 AMUv1-based arch_freq_get_on_cpu | expand |
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 ? 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. -- Regards, Sudeep
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
On Thu, Apr 18, 2024 at 05:55:43PM +0200, Beata Michalska wrote: > 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. I prefer that if we can't find any other alternative. Do you see any issues with that ? That said I am fine if Will/Catalin is happy with this. > > 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. Yes it is better to handle it even if it is theoretical. I assume you get some compiler error if you assign unconditionally and if(IS_ENABLED()) also doesn't work in this case as it would still give error ? -- Regards, Sudeep
On Wed, Apr 24, 2024 at 11:25:27AM +0100, Sudeep Holla wrote: > On Thu, Apr 18, 2024 at 05:55:43PM +0200, Beata Michalska wrote: > > 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. > > I prefer that if we can't find any other alternative. Do you see any issues > with that ? That said I am fine if Will/Catalin is happy with this. > We could actually move it up further to amu_fie_setup and potentially save on memory if none of the present CPUs have valid AMU counters. This is unlikely but still. So it could look like: --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@@ -297,7 -194,7 +297,8 @@@ static void amu_fie_setup(const struct int cpu; /* We are already set since the last insmod of cpufreq driver */ ++ if (cpumask_available(amu_fie_cpus) && -- if (unlikely(cpumask_subset(cpus, amu_fie_cpus))) ++ unlikely(cpumask_subset(cpus, amu_fie_cpus))) return; for_each_cpu(cpu, cpus) { @@@ -305,6 -202,6 +306,10 @@@ return; } ++ if (!cpumask_available(amu_fie_cpus) && ++ !zalloc_cpumask_var(&amu_fie_cpus, GFP_KERNEL)) ++ return; ++ In both cases we risk not setting up AMUs for FIE for all or some CPUs if we fail to allocate the memory but I guess we are already there. @Ionela: What do you think? > > > 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. > > Yes it is better to handle it even if it is theoretical. > > I assume you get some compiler error if you assign unconditionally and > if(IS_ENABLED()) also doesn't work in this case as it would still give > error ? Yes, the #if is needed to exclude it from compilation if !CPUMASK_OFFSTACK. --- BR Beata > > -- > Regards, > Sudeep
Hi Beata, On Thursday 25 Apr 2024 at 16:27:37 (+0200), Beata Michalska wrote: > On Wed, Apr 24, 2024 at 11:25:27AM +0100, Sudeep Holla wrote: > > On Thu, Apr 18, 2024 at 05:55:43PM +0200, Beata Michalska wrote: > > > 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. > > > > I prefer that if we can't find any other alternative. Do you see any issues > > with that ? That said I am fine if Will/Catalin is happy with this. > > > We could actually move it up further to amu_fie_setup and potentially save on > memory if none of the present CPUs have valid AMU counters. This is unlikely but > still. So it could look like: > > --- a/arch/arm64/kernel/topology.c > +++ b/arch/arm64/kernel/topology.c > @@@ -297,7 -194,7 +297,8 @@@ static void amu_fie_setup(const struct > int cpu; > > /* We are already set since the last insmod of cpufreq driver */ > ++ if (cpumask_available(amu_fie_cpus) && > -- if (unlikely(cpumask_subset(cpus, amu_fie_cpus))) > ++ unlikely(cpumask_subset(cpus, amu_fie_cpus))) > return; > > for_each_cpu(cpu, cpus) { > @@@ -305,6 -202,6 +306,10 @@@ > return; > } > > ++ if (!cpumask_available(amu_fie_cpus) && > ++ !zalloc_cpumask_var(&amu_fie_cpus, GFP_KERNEL)) > ++ return; > ++ > > In both cases we risk not setting up AMUs for FIE for all or some CPUs > if we fail to allocate the memory but I guess we are already there. > @Ionela: What do you think? It looks good to me. Many thanks for the fix. Ionela. > > > > > 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. > > > > Yes it is better to handle it even if it is theoretical. > > > > I assume you get some compiler error if you assign unconditionally and > > if(IS_ENABLED()) also doesn't work in this case as it would still give > > error ? > Yes, the #if is needed to exclude it from compilation if !CPUMASK_OFFSTACK. > > --- > BR > Beata > > > > -- > > Regards, > > Sudeep
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 + } return ret; }
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 ...) Signed-off-by: Beata Michalska <beata.michalska@arm.com> --- arch/arm64/kernel/topology.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)