Message-ID: <d4c68681d9e72a5b841bbbb1af1606b382079ad6.camel@intel.com>
Date: Wed, 11 Sep 2024 18:01:00 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Yu, Yu-cheng" <yu-cheng.yu@...el.com>, "Yang, Weijiang"
	<weijiang.yang@...el.com>, "kees@...nel.org" <kees@...nel.org>,
	"x86@...nel.org" <x86@...nel.org>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "broonie@...nel.org" <broonie@...nel.org>,
	"hpa@...or.com" <hpa@...or.com>, "mingo@...hat.com" <mingo@...hat.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "bp@...en8.de" <bp@...en8.de>,
	"rppt@...nel.org" <rppt@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"catalin.marinas@....com" <catalin.marinas@....com>, "debug@...osinc.com"
	<debug@...osinc.com>
Subject: Re: [PATCH] x86/shstk: Free the thread shadow stack before
 disassociating from the mm

On Tue, 2024-09-10 at 23:56 +0100, Mark Brown wrote:
> When using shadow stacks the kernel will transparently allocate a shadow
> stack for each thread. The intention is that this will be freed when the
> thread exits but currently this doesn't actually happen. The deallocation
> is done by shstk_free() which is called from exit_thread() and has a guard
> to check for !tsk->mm due to the use of vm_unmap(). This doesn't actually
> do anything since in do_exit() we call exit_mm() prior to thread_exit() and
> exit_mm() disassociates the task from the mm and clears tsk->mm. The result
> is that no shadow stacks will be freed until the process exits, leaking
> memory for any process which creates and destroys threads.
> 
> Fix this by moving the shstk_free() to a new exit_thread_early() call which
> runs immediately prior to exit_mm(). We don't do this right at the start of
> do_exit() due to the handling for klling init. This could trigger some
> incompatibility if there is code that looks at the shadow stack of a thread
> which has exited but this seems much less likely than the leaking of shadow
> stacks due to thread deletion.
> 
> Fixes: 18e66b695e78 ("x86/shstk: Add Kconfig option for shadow stack")
> Signed-off-by: Mark Brown <broonie@...nel.org>
> ---
> It is entirely possible I am missing something here, I don't have a
> system that allows me to test shadow stack support directly and have
> only checked this by inspection and tested with my arm64 GCS series.
> If this makes sense it'll need to become a dependency for GCS.

The common cleanup case is via deactivate_mm()->shstk_free(), which happens when
the MM is still attached. But there is also an exit_thread() caller in the fork
failure patch (see copy_process()).

So by my inspection, the exit_thread_early() is not needed because of the
deactivate_mm() path that happens earlier in do_exit() via exit_mm(). But since
this patch also removes the shstk_free() from the copy_process() error path, I
think we would need clarity that it is unneeded there.

A quick search through the arm series and I don't see deactivate_mm()
implementation, and instead a separate cleanup solution. Could that be the
reason why you saw the leak on arm? Considering the trickiness of the auto
allocated shadow stacks lifecycle, I think it would be great if all the
implementations had common logic. If possible at least.

BTW, two more notes on this whole area:
1. 99% sure glibc has some tests that catch leaks like hypothesized here, by
watching for memory grown after repeated thread exits. IIRC I introduced a
shadow stack leak at some point during development that failed the test.
2. Weijiang (CCed) is working on a fix for case in the opposite direction. An
error path that attempts to free the shadow stack twice and triggers a warning.

Powered by blists - more mailing lists