[PATCH] arm64/gcs: Don't call gcs_free() during flush_gcs()

[PATCH] arm64/gcs: Don't call gcs_free() during flush_gcs()

[Mark Brown]

Currently we call gcs_free() during flush_gcs() to reset the thread state
for GCS. This includes unmapping any kernel allocated GCS, but this is
redundant when doing a flush_thread() since we are reinitialisng the thread
memory too. Inline the reinitialisaton of the thread struct.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/process.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index a5ca15daeb8a..5954cec19660 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -288,7 +288,9 @@ static void flush_gcs(void)
 	if (!system_supports_gcs())
 		return;
 
-	gcs_free(current);
+	current->thread.gcspr_el0 = 0;
+	current->thread.gcs_base = 0;
+	current->thread.gcs_size = 0;
 	current->thread.gcs_el0_mode = 0;
 	write_sysreg_s(GCSCRE0_EL1_nTR, SYS_GCSCRE0_EL1);
 	write_sysreg_s(0, SYS_GCSPR_EL0);

---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250609-arm64-gcs-flush-thread-8aeff2a71d5d

Best regards,
-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH] arm64/gcs: Don't call gcs_free() during flush_gcs()

[Catalin Marinas]

On Wed, Jun 11, 2025 at 05:28:13PM +0100, Mark Brown wrote:
> Currently we call gcs_free() during flush_gcs() to reset the thread state
> for GCS. This includes unmapping any kernel allocated GCS, but this is
> redundant when doing a flush_thread() since we are reinitialisng the thread
> memory too. Inline the reinitialisaton of the thread struct.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/process.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index a5ca15daeb8a..5954cec19660 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -288,7 +288,9 @@ static void flush_gcs(void)
>  	if (!system_supports_gcs())
>  		return;
>  
> -	gcs_free(current);
> +	current->thread.gcspr_el0 = 0;
> +	current->thread.gcs_base = 0;
> +	current->thread.gcs_size = 0;
>  	current->thread.gcs_el0_mode = 0;
>  	write_sysreg_s(GCSCRE0_EL1_nTR, SYS_GCSCRE0_EL1);
>  	write_sysreg_s(0, SYS_GCSPR_EL0);

I think this makes sense.

However, I thought there was another slightly misplaced call to
gcs_free() via arch_release_task_struct(). I wouldn't touch the user
memory with vm_munmap() when releasing a task structure. Is this needed
because the shadow stack is allocated automatically on thread creation,
so we need something to free it when the thread died?

Another caller of gcs_free() is deactivate_mm(). It's not clear to me
when we need to free the shadow stack on this path. On the exit_mm()
path for example we have mmput() -> exit_mmap() that takes care of
unmapping everything. Similarly on the exec_mmap() path.

-- 
Catalin

Re: [PATCH] arm64/gcs: Don't call gcs_free() during flush_gcs()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On Wed, Jun 11, 2025 at 06:34:15PM +0100, Catalin Marinas wrote:

> However, I thought there was another slightly misplaced call to
> gcs_free() via arch_release_task_struct(). I wouldn't touch the user
> memory with vm_munmap() when releasing a task structure. Is this needed
> because the shadow stack is allocated automatically on thread creation,
> so we need something to free it when the thread died?

Yeah, I've got another patch written but not sent for that (since it
doesn't actually overlap) but like you say I need to check that things
are joined up for threads that had a GCS created automatically for
compatibility before I send it out.

> Another caller of gcs_free() is deactivate_mm(). It's not clear to me
> when we need to free the shadow stack on this path. On the exit_mm()
> path for example we have mmput() -> exit_mmap() that takes care of
> unmapping everything. Similarly on the exec_mmap() path.

We need that one to clean up the GCS for threads that had it allocated
for compatibility, you can see the leak that results without it easily
with the glibc testsuite (or anything else that does threads, the glibc
tests just spot it).  Most of the checking for arch_release_task_struct()
is verifying that deactivate_mm() is guaranteed to be called eveywhere
it's relevant, I need to page that back in.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] arm64/gcs: Don't call gcs_free() during flush_gcs()

[Catalin Marinas]

On Thu, Jun 12, 2025 at 12:40:42PM +0100, Mark Brown wrote:
> On Wed, Jun 11, 2025 at 06:34:15PM +0100, Catalin Marinas wrote:
> > However, I thought there was another slightly misplaced call to
> > gcs_free() via arch_release_task_struct(). I wouldn't touch the user
> > memory with vm_munmap() when releasing a task structure. Is this needed
> > because the shadow stack is allocated automatically on thread creation,
> > so we need something to free it when the thread died?
> 
> Yeah, I've got another patch written but not sent for that (since it
> doesn't actually overlap) but like you say I need to check that things
> are joined up for threads that had a GCS created automatically for
> compatibility before I send it out.

OK. For this patch:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

> > Another caller of gcs_free() is deactivate_mm(). It's not clear to me
> > when we need to free the shadow stack on this path. On the exit_mm()
> > path for example we have mmput() -> exit_mmap() that takes care of
> > unmapping everything. Similarly on the exec_mmap() path.
> 
> We need that one to clean up the GCS for threads that had it allocated
> for compatibility, you can see the leak that results without it easily
> with the glibc testsuite (or anything else that does threads, the glibc
> tests just spot it).  Most of the checking for arch_release_task_struct()
> is verifying that deactivate_mm() is guaranteed to be called eveywhere
> it's relevant, I need to page that back in.

Makes sense. I think we should only keep gcs_free() in one place,
ideally deactivate_mm() as that's more related to mm rather than the
task_struct.

-- 
Catalin

Re: [PATCH] arm64/gcs: Don't call gcs_free() during flush_gcs()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

On Thu, Jun 12, 2025 at 03:47:44PM +0100, Catalin Marinas wrote:
> On Thu, Jun 12, 2025 at 12:40:42PM +0100, Mark Brown wrote:
> > On Wed, Jun 11, 2025 at 06:34:15PM +0100, Catalin Marinas wrote:

> > > Another caller of gcs_free() is deactivate_mm(). It's not clear to me
> > > when we need to free the shadow stack on this path. On the exit_mm()
> > > path for example we have mmput() -> exit_mmap() that takes care of
> > > unmapping everything. Similarly on the exec_mmap() path.

> > We need that one to clean up the GCS for threads that had it allocated
> > for compatibility, you can see the leak that results without it easily
> > with the glibc testsuite (or anything else that does threads, the glibc
> > tests just spot it).  Most of the checking for arch_release_task_struct()
> > is verifying that deactivate_mm() is guaranteed to be called eveywhere
> > it's relevant, I need to page that back in.

> Makes sense. I think we should only keep gcs_free() in one place,
> ideally deactivate_mm() as that's more related to mm rather than the
> task_struct.

Yes, me too - I just need to double check.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] arm64/gcs: Don't call gcs_free() during flush_gcs()

[Will Deacon]

On Thu, Jun 12, 2025 at 03:51:19PM +0100, Mark Brown wrote:
> On Thu, Jun 12, 2025 at 03:47:44PM +0100, Catalin Marinas wrote:
> > On Thu, Jun 12, 2025 at 12:40:42PM +0100, Mark Brown wrote:
> > > On Wed, Jun 11, 2025 at 06:34:15PM +0100, Catalin Marinas wrote:
> 
> > > > Another caller of gcs_free() is deactivate_mm(). It's not clear to me
> > > > when we need to free the shadow stack on this path. On the exit_mm()
> > > > path for example we have mmput() -> exit_mmap() that takes care of
> > > > unmapping everything. Similarly on the exec_mmap() path.
> 
> > > We need that one to clean up the GCS for threads that had it allocated
> > > for compatibility, you can see the leak that results without it easily
> > > with the glibc testsuite (or anything else that does threads, the glibc
> > > tests just spot it).  Most of the checking for arch_release_task_struct()
> > > is verifying that deactivate_mm() is guaranteed to be called eveywhere
> > > it's relevant, I need to page that back in.
> 
> > Makes sense. I think we should only keep gcs_free() in one place,
> > ideally deactivate_mm() as that's more related to mm rather than the
> > task_struct.
> 
> Yes, me too - I just need to double check.

Having looking a little at the code, I think that
arch_release_task_struct() might be better than deactivate_mm(). The
latter takes an 'mm' parameter which we ignore but I think happens to
be 'current->mm'and so things work. Given that, and that we don't do any
GCS management on the activate_mm() path, freeing the GCS in the
task-centric functions makes more sense to me.

Will

Re: [PATCH] arm64/gcs: Don't call gcs_free() during flush_gcs()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On Thu, Jun 12, 2025 at 05:20:28PM +0100, Will Deacon wrote:

> Having looking a little at the code, I think that
> arch_release_task_struct() might be better than deactivate_mm(). The
> latter takes an 'mm' parameter which we ignore but I think happens to
> be 'current->mm'and so things work. Given that, and that we don't do any
> GCS management on the activate_mm() path, freeing the GCS in the
> task-centric functions makes more sense to me.

The issue with that is that we only call arch_release_task_struct()
quite late, after the mm has been disassociated from the task.
do_exit() cleans up the mm with exit_mm() relatively early on, and
free_task() which is what calls arch_release_task_struct() is one of the
last things we do as we clean up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] arm64/gcs: Don't call gcs_free() during flush_gcs()

[Will Deacon]

On Wed, 11 Jun 2025 17:28:13 +0100, Mark Brown wrote:
> Currently we call gcs_free() during flush_gcs() to reset the thread state
> for GCS. This includes unmapping any kernel allocated GCS, but this is
> redundant when doing a flush_thread() since we are reinitialisng the thread
> memory too. Inline the reinitialisaton of the thread struct.
> 
> 

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64/gcs: Don't call gcs_free() during flush_gcs()
      https://git.kernel.org/arm64/c/d2be3270f40b

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev