Re: [PATCH v7 28/41] x86: Introduce userspace API for shadow stack
From: Borislav Petkov <bp@alien8.de>
Date: 2023-03-08 10:28:32
Also in:
linux-arch, linux-doc, linux-mm, lkml
On Mon, Feb 27, 2023 at 02:29:44PM -0800, Rick Edgecombe wrote:
From: "Kirill A. Shutemov" <redacted> Add three new arch_prctl() handles: - ARCH_SHSTK_ENABLE/DISABLE enables or disables the specified feature. Returns 0 on success or an error.
"... or a negative value on error."
- ARCH_SHSTK_LOCK prevents future disabling or enabling of the specified feature. Returns 0 on success or an error
ditto. What is the use case of the feature locking? I'm under the simple assumption that once shstk is enabled for an app, it remains so. I guess my question is rather, what's the use case for enabling shadow stack and then disabling it later for an app...?
The features are handled per-thread and inherited over fork(2)/clone(2), but reset on exec(). This is preparation patch. It does not implement any features.
That belongs under the "---" line I guess.
Tested-by: Pengfei Xu <redacted> Tested-by: John Allen <john.allen@amd.com> Tested-by: Kees Cook <redacted> Acked-by: Mike Rapoport (IBM) <rppt@kernel.org> Reviewed-by: Kees Cook <redacted> Signed-off-by: Kirill A. Shutemov <redacted> [tweaked with feedback from tglx] Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- v4: - Remove references to CET and replace with shadow stack (Peterz) v3: - Move shstk.c Makefile changes earlier (Kees) - Add #ifdef around features_locked and features (Kees) - Encapsulate features reset earlier in reset_thread_features() so features and features_locked are not referenced in code that would be compiled !CONFIG_X86_USER_SHADOW_STACK. (Kees) - Fix typo in commit log (Kees) - Switch arch_prctl() numbers to avoid conflict with LAM v2: - Only allow one enable/disable per call (tglx) - Return error code like a normal arch_prctl() (Alexander Potapenko) - Make CET only (tglx) --- arch/x86/include/asm/processor.h | 6 +++++ arch/x86/include/asm/shstk.h | 21 +++++++++++++++ arch/x86/include/uapi/asm/prctl.h | 6 +++++ arch/x86/kernel/Makefile | 2 ++ arch/x86/kernel/process_64.c | 7 ++++- arch/x86/kernel/shstk.c | 44 +++++++++++++++++++++++++++++++ 6 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 arch/x86/include/asm/shstk.h create mode 100644 arch/x86/kernel/shstk.c
...
+long shstk_prctl(struct task_struct *task, int option, unsigned long features)
+{
+ if (option == ARCH_SHSTK_LOCK) {
+ task->thread.features_locked |= features;
+ return 0;
+ }
+
+ /* Don't allow via ptrace */
+ if (task != current)
+ return -EINVAL;
+
+ /* Do not allow to change locked features */
+ if (features & task->thread.features_locked)
+ return -EPERM;
+
+ /* Only support enabling/disabling one feature at a time. */
+ if (hweight_long(features) > 1)
+ return -EINVAL;
+
+ if (option == ARCH_SHSTK_DISABLE) {
+ return -EINVAL;
+ }
{} braces left over from some previous version. Can go now.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette