| Message ID | 20220929222936.14584-31-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadowstacks for userspace | expand |
On Thu, Sep 29, 2022 at 03:29:27PM -0700, Rick Edgecombe wrote: > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > > Applications and loaders can have logic to decide whether to enable CET. > They usually don't report whether CET has been enabled or not, so there > is no way to verify whether an application actually is protected by CET > features. > > Add two lines in /proc/$PID/arch_status to report enabled and locked > features. > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > [Switched to CET, added to commit log] > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > --- > > v2: > - New patch > > arch/x86/kernel/Makefile | 2 ++ > arch/x86/kernel/fpu/xstate.c | 47 --------------------------- > arch/x86/kernel/proc.c | 63 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 65 insertions(+), 47 deletions(-) > create mode 100644 arch/x86/kernel/proc.c This is two patches: one to create proc.c, the other to add CET support. I found where the "arch_status" conversation was: https://lore.kernel.org/all/CALCETrUjF9PBmkzH1J86vw4ZW785DP7FtcT+gcSrx29=BUnjoQ@mail.gmail.com/ Andy, what did you mean "make sure that everything in it is namespaced"? Everything already has a field name. And arch_status doesn't exactly solve having compat fields -- it still needs to be handled manually? Anyway... we have arch_status, so I guess it's fine. > [...] > +int proc_pid_arch_status(struct seq_file *m, struct pid_namespace *ns, > + struct pid *pid, struct task_struct *task) > +{ > + /* > + * Report AVX512 state if the processor and build option supported. > + */ > + if (cpu_feature_enabled(X86_FEATURE_AVX512F)) > + avx512_status(m, task); > + > + seq_puts(m, "Thread_features:\t"); > + dump_features(m, task->thread.features); > + seq_putc(m, '\n'); > + > + seq_puts(m, "Thread_features_locked:\t"); > + dump_features(m, task->thread.features_locked); > + seq_putc(m, '\n'); Why are these always present instead of ifdefed? -Kees
On Mon, Oct 3, 2022, at 3:37 PM, Kees Cook wrote: > On Thu, Sep 29, 2022 at 03:29:27PM -0700, Rick Edgecombe wrote: >> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> >> >> Applications and loaders can have logic to decide whether to enable CET. >> They usually don't report whether CET has been enabled or not, so there >> is no way to verify whether an application actually is protected by CET >> features. >> >> Add two lines in /proc/$PID/arch_status to report enabled and locked >> features. >> >> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> [Switched to CET, added to commit log] >> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> >> >> --- >> >> v2: >> - New patch >> >> arch/x86/kernel/Makefile | 2 ++ >> arch/x86/kernel/fpu/xstate.c | 47 --------------------------- >> arch/x86/kernel/proc.c | 63 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 65 insertions(+), 47 deletions(-) >> create mode 100644 arch/x86/kernel/proc.c > > This is two patches: one to create proc.c, the other to add CET support. > > I found where the "arch_status" conversation was: > https://lore.kernel.org/all/CALCETrUjF9PBmkzH1J86vw4ZW785DP7FtcT+gcSrx29=BUnjoQ@mail.gmail.com/ > > Andy, what did you mean "make sure that everything in it is namespaced"? > Everything already has a field name. And arch_status doesn't exactly > solve having compat fields -- it still needs to be handled manually? > Anyway... we have arch_status, so I guess it's fine. I think I meant that, since it's "arch_status" not "x86_status", the fields should have names like "x86.Thread_features". Otherwise if another architecture adds a Thread_features field, then anything running under something like qemu userspace emulation could be confused. Assuming that's what I meant, I think my comment still stands :)
On Mon, Oct 03, 2022 at 03:45:50PM -0700, Andy Lutomirski wrote: > > > On Mon, Oct 3, 2022, at 3:37 PM, Kees Cook wrote: > > On Thu, Sep 29, 2022 at 03:29:27PM -0700, Rick Edgecombe wrote: > >> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > >> > >> Applications and loaders can have logic to decide whether to enable CET. > >> They usually don't report whether CET has been enabled or not, so there > >> is no way to verify whether an application actually is protected by CET > >> features. > >> > >> Add two lines in /proc/$PID/arch_status to report enabled and locked > >> features. > >> > >> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > >> [Switched to CET, added to commit log] > >> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > >> > >> --- > >> > >> v2: > >> - New patch > >> > >> arch/x86/kernel/Makefile | 2 ++ > >> arch/x86/kernel/fpu/xstate.c | 47 --------------------------- > >> arch/x86/kernel/proc.c | 63 ++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 65 insertions(+), 47 deletions(-) > >> create mode 100644 arch/x86/kernel/proc.c > > > > This is two patches: one to create proc.c, the other to add CET support. > > > > I found where the "arch_status" conversation was: > > https://lore.kernel.org/all/CALCETrUjF9PBmkzH1J86vw4ZW785DP7FtcT+gcSrx29=BUnjoQ@mail.gmail.com/ > > > > Andy, what did you mean "make sure that everything in it is namespaced"? > > Everything already has a field name. And arch_status doesn't exactly > > solve having compat fields -- it still needs to be handled manually? > > Anyway... we have arch_status, so I guess it's fine. > > I think I meant that, since it's "arch_status" not "x86_status", the fields should have names like "x86.Thread_features". Otherwise if another architecture adds a Thread_features field, then anything running under something like qemu userspace emulation could be confused. > > Assuming that's what I meant, I think my comment still stands :) Ah, but that would be needed for compat things too in "arch_status", and could just as well live in "status". How about moving both of these into "status", with appropriate names? x86_64.Thread_features: ... i386.LDT_or_something: ... ? Does anything consume arch_status yet? Looks like probably not: https://codesearch.debian.net/search?q=%5Cbarch_status%5Cb&literal=0&perpkg=1
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 8950d1f71226..b87b8a0a3749 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -141,6 +141,8 @@ obj-$(CONFIG_AMD_MEM_ENCRYPT) += sev.o obj-$(CONFIG_X86_SHADOW_STACK) += shstk.o +obj-$(CONFIG_PROC_FS) += proc.o + ### # 64 bit specific files ifeq ($(CONFIG_X86_64),y) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index 5e6a4867fd05..9258fc1169cc 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -10,8 +10,6 @@ #include <linux/mman.h> #include <linux/nospec.h> #include <linux/pkeys.h> -#include <linux/seq_file.h> -#include <linux/proc_fs.h> #include <linux/vmalloc.h> #include <asm/fpu/api.h> @@ -1746,48 +1744,3 @@ long fpu_xstate_prctl(int option, unsigned long arg2) return -EINVAL; } } - -#ifdef CONFIG_PROC_PID_ARCH_STATUS -/* - * Report the amount of time elapsed in millisecond since last AVX512 - * use in the task. - */ -static void avx512_status(struct seq_file *m, struct task_struct *task) -{ - unsigned long timestamp = READ_ONCE(task->thread.fpu.avx512_timestamp); - long delta; - - if (!timestamp) { - /* - * Report -1 if no AVX512 usage - */ - delta = -1; - } else { - delta = (long)(jiffies - timestamp); - /* - * Cap to LONG_MAX if time difference > LONG_MAX - */ - if (delta < 0) - delta = LONG_MAX; - delta = jiffies_to_msecs(delta); - } - - seq_put_decimal_ll(m, "AVX512_elapsed_ms:\t", delta); - seq_putc(m, '\n'); -} - -/* - * Report architecture specific information - */ -int proc_pid_arch_status(struct seq_file *m, struct pid_namespace *ns, - struct pid *pid, struct task_struct *task) -{ - /* - * Report AVX512 state if the processor and build option supported. - */ - if (cpu_feature_enabled(X86_FEATURE_AVX512F)) - avx512_status(m, task); - - return 0; -} -#endif /* CONFIG_PROC_PID_ARCH_STATUS */ diff --git a/arch/x86/kernel/proc.c b/arch/x86/kernel/proc.c new file mode 100644 index 000000000000..fa9cbe13c298 --- /dev/null +++ b/arch/x86/kernel/proc.c @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <linux/seq_file.h> +#include <linux/proc_fs.h> +#include <uapi/asm/prctl.h> + +/* + * Report the amount of time elapsed in millisecond since last AVX512 + * use in the task. + */ +static void avx512_status(struct seq_file *m, struct task_struct *task) +{ + unsigned long timestamp = READ_ONCE(task->thread.fpu.avx512_timestamp); + long delta; + + if (!timestamp) { + /* + * Report -1 if no AVX512 usage + */ + delta = -1; + } else { + delta = (long)(jiffies - timestamp); + /* + * Cap to LONG_MAX if time difference > LONG_MAX + */ + if (delta < 0) + delta = LONG_MAX; + delta = jiffies_to_msecs(delta); + } + + seq_put_decimal_ll(m, "AVX512_elapsed_ms:\t", delta); + seq_putc(m, '\n'); +} + +static void dump_features(struct seq_file *m, unsigned long features) +{ + if (features & CET_SHSTK) + seq_puts(m, "shstk "); + if (features & CET_WRSS) + seq_puts(m, "wrss "); +} + +/* + * Report architecture specific information + */ +int proc_pid_arch_status(struct seq_file *m, struct pid_namespace *ns, + struct pid *pid, struct task_struct *task) +{ + /* + * Report AVX512 state if the processor and build option supported. + */ + if (cpu_feature_enabled(X86_FEATURE_AVX512F)) + avx512_status(m, task); + + seq_puts(m, "Thread_features:\t"); + dump_features(m, task->thread.features); + seq_putc(m, '\n'); + + seq_puts(m, "Thread_features_locked:\t"); + dump_features(m, task->thread.features_locked); + seq_putc(m, '\n'); + + return 0; +}