diff mbox

[RFC,v2,25/27] x86/cet: Add PTRACE interface for CET

Message ID 20180710222639.8241-26-yu-cheng.yu@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yu, Yu-cheng July 10, 2018, 10:26 p.m. UTC
Add PTRACE interface for CET MSRs.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/include/asm/fpu/regset.h |  7 +++---
 arch/x86/kernel/fpu/regset.c      | 41 +++++++++++++++++++++++++++++++
 arch/x86/kernel/ptrace.c          | 16 ++++++++++++
 include/uapi/linux/elf.h          |  1 +
 4 files changed, 62 insertions(+), 3 deletions(-)

Comments

Ingo Molnar July 11, 2018, 10:20 a.m. UTC | #1
* Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:

> Add PTRACE interface for CET MSRs.

Please *always* describe new ABIs in the changelog, in a precise, well-documented 
way.

> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index e2ee403865eb..ac2bc3a18427 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -49,7 +49,9 @@ enum x86_regset {
>  	REGSET_IOPERM64 = REGSET_XFP,
>  	REGSET_XSTATE,
>  	REGSET_TLS,
> +	REGSET_CET64 = REGSET_TLS,
>  	REGSET_IOPERM32,
> +	REGSET_CET32,
>  };

Why does REGSET_CET64 alias on REGSET_TLS?

>  struct pt_regs_offset {
> @@ -1276,6 +1278,13 @@ static struct user_regset x86_64_regsets[] __ro_after_init = {
>  		.size = sizeof(long), .align = sizeof(long),
>  		.active = ioperm_active, .get = ioperm_get
>  	},
> +	[REGSET_CET64] = {
> +		.core_note_type = NT_X86_CET,
> +		.n = sizeof(struct cet_user_state) / sizeof(u64),
> +		.size = sizeof(u64), .align = sizeof(u64),
> +		.active = cetregs_active, .get = cetregs_get,
> +		.set = cetregs_set
> +	},

Ok, could we first please make this part of the regset code more readable and 
start the series with a standalone clean-up patch that changes these initializers 
to something more readable:

	[REGSET_CET64] = {
		.core_note_type	= NT_X86_CET,
		.n		= sizeof(struct cet_user_state) / sizeof(u64),
		.size		= sizeof(u64),
		.align		= sizeof(u64),
		.active		= cetregs_active,
		.get		= cetregs_get,
		.set		= cetregs_set
	},

? (I'm demonstrating the cleanup based on REGSET_CET64, but this should be done on 
every other entry first.)


> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -401,6 +401,7 @@ typedef struct elf64_shdr {
>  #define NT_386_TLS	0x200		/* i386 TLS slots (struct user_desc) */
>  #define NT_386_IOPERM	0x201		/* x86 io permission bitmap (1=deny) */
>  #define NT_X86_XSTATE	0x202		/* x86 extended state using xsave */
> +#define NT_X86_CET	0x203		/* x86 cet state */

Acronyms in comments should be in capital letters.

Also, I think I asked this before: why does "Control Flow Enforcement" abbreviate 
to "CET" (which is a well-known acronym for "Central European Time"), not to CFE?

Thanks,

	Ingo
Yu, Yu-cheng July 11, 2018, 3:40 p.m. UTC | #2
On Wed, 2018-07-11 at 12:20 +0200, Ingo Molnar wrote:
> * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> 
> > 
> > Add PTRACE interface for CET MSRs.
> Please *always* describe new ABIs in the changelog, in a precise,
> well-documented 
> way.

Ok!

> > 
> > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > index e2ee403865eb..ac2bc3a18427 100644
> > --- a/arch/x86/kernel/ptrace.c
> > +++ b/arch/x86/kernel/ptrace.c
> > @@ -49,7 +49,9 @@ enum x86_regset {
> >  	REGSET_IOPERM64 = REGSET_XFP,
> >  	REGSET_XSTATE,
> >  	REGSET_TLS,
> > +	REGSET_CET64 = REGSET_TLS,
> >  	REGSET_IOPERM32,
> > +	REGSET_CET32,
> >  };
> Why does REGSET_CET64 alias on REGSET_TLS?

In x86_64_regsets[], there is no [REGSET_TLS].  The core dump code
cannot handle holes in the array.

> 
> > 
> >  struct pt_regs_offset {
> > @@ -1276,6 +1278,13 @@ static struct user_regset x86_64_regsets[]
> > __ro_after_init = {
> >  		.size = sizeof(long), .align = sizeof(long),
> >  		.active = ioperm_active, .get = ioperm_get
> >  	},
> > +	[REGSET_CET64] = {
> > +		.core_note_type = NT_X86_CET,
> > +		.n = sizeof(struct cet_user_state) / sizeof(u64),
> > +		.size = sizeof(u64), .align = sizeof(u64),
> > +		.active = cetregs_active, .get = cetregs_get,
> > +		.set = cetregs_set
> > +	},
> Ok, could we first please make this part of the regset code more
> readable and 
> start the series with a standalone clean-up patch that changes these
> initializers 
> to something more readable:
> 
> 	[REGSET_CET64] = {
> 		.core_note_type	= NT_X86_CET,
> 		.n		= sizeof(struct cet_user_state) /
> sizeof(u64),
> 		.size		= sizeof(u64),
> 		.align		= sizeof(u64),
> 		.active		= cetregs_active,
> 		.get		= cetregs_get,
> 		.set		= cetregs_set
> 	},
> 
> ? (I'm demonstrating the cleanup based on REGSET_CET64, but this
> should be done on 
> every other entry first.)
> 

I will fix it.

> 
> > 
> > --- a/include/uapi/linux/elf.h
> > +++ b/include/uapi/linux/elf.h
> > @@ -401,6 +401,7 @@ typedef struct elf64_shdr {
> >  #define NT_386_TLS	0x200		/* i386 TLS slots
> > (struct user_desc) */
> >  #define NT_386_IOPERM	0x201		/* x86 io
> > permission bitmap (1=deny) */
> >  #define NT_X86_XSTATE	0x202		/* x86 extended
> > state using xsave */
> > +#define NT_X86_CET	0x203		/* x86 cet state */
> Acronyms in comments should be in capital letters.
> 
> Also, I think I asked this before: why does "Control Flow
> Enforcement" abbreviate 
> to "CET" (which is a well-known acronym for "Central European Time"),
> not to CFE?
> 

I don't know if I can change that, will find out.

Thanks,
Yu-cheng
Ingo Molnar July 12, 2018, 2:03 p.m. UTC | #3
* Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:

> > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > > index e2ee403865eb..ac2bc3a18427 100644
> > > --- a/arch/x86/kernel/ptrace.c
> > > +++ b/arch/x86/kernel/ptrace.c
> > > @@ -49,7 +49,9 @@ enum x86_regset {
> > >  	REGSET_IOPERM64 = REGSET_XFP,
> > >  	REGSET_XSTATE,
> > >  	REGSET_TLS,
> > > +	REGSET_CET64 = REGSET_TLS,
> > >  	REGSET_IOPERM32,
> > > +	REGSET_CET32,
> > >  };
> > Why does REGSET_CET64 alias on REGSET_TLS?
> 
> In x86_64_regsets[], there is no [REGSET_TLS].  The core dump code
> cannot handle holes in the array.

Is there a fundamental (ABI) reason for that?

> > to "CET" (which is a well-known acronym for "Central European Time"),
> > not to CFE?
> > 
> 
> I don't know if I can change that, will find out.

So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal 
names, except for the Intel feature bit and any MSR enumeration which can be CET 
if Intel named it that way, and a short comment explaining the acronym difference.

Or something like that.

Thanks,

	Ingo
Yu, Yu-cheng July 12, 2018, 10:37 p.m. UTC | #4
On Thu, 2018-07-12 at 16:03 +0200, Ingo Molnar wrote:
> * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> 
> > 
> > > 
> > > > 
> > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > > > index e2ee403865eb..ac2bc3a18427 100644
> > > > --- a/arch/x86/kernel/ptrace.c
> > > > +++ b/arch/x86/kernel/ptrace.c
> > > > @@ -49,7 +49,9 @@ enum x86_regset {
> > > >  	REGSET_IOPERM64 = REGSET_XFP,
> > > >  	REGSET_XSTATE,
> > > >  	REGSET_TLS,
> > > > +	REGSET_CET64 = REGSET_TLS,
> > > >  	REGSET_IOPERM32,
> > > > +	REGSET_CET32,
> > > >  };
> > > Why does REGSET_CET64 alias on REGSET_TLS?
> > In x86_64_regsets[], there is no [REGSET_TLS].  The core dump code
> > cannot handle holes in the array.
> Is there a fundamental (ABI) reason for that?

What I did was, ran Linux with 'slub_debug', and forced a core dump
(kill -abrt <pid>), then there was a red zone warning in the dmesg.
My feeling is there could be issues in the core dump code.  These
enum's are only local to arch/x86/kernel/ptrace.c and not exported.
I am not aware this is in the ABI.

> 
> > 
> > > 
> > > to "CET" (which is a well-known acronym for "Central European Time"),
> > > not to CFE?
> > > 
> > I don't know if I can change that, will find out.
> So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal 
> names, except for the Intel feature bit and any MSR enumeration which can be CET 
> if Intel named it that way, and a short comment explaining the acronym difference.
> 
> Or something like that.

Ok, I will make changes in the next version and probably revise
from that if still not optimal.

Yu-cheng
Thomas Gleixner July 12, 2018, 11:08 p.m. UTC | #5
On Thu, 12 Jul 2018, Yu-cheng Yu wrote:
> On Thu, 2018-07-12 at 16:03 +0200, Ingo Molnar wrote:
> > * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > > > > index e2ee403865eb..ac2bc3a18427 100644
> > > > > --- a/arch/x86/kernel/ptrace.c
> > > > > +++ b/arch/x86/kernel/ptrace.c
> > > > > @@ -49,7 +49,9 @@ enum x86_regset {
> > > > >  	REGSET_IOPERM64 = REGSET_XFP,
> > > > >  	REGSET_XSTATE,
> > > > >  	REGSET_TLS,
> > > > > +	REGSET_CET64 = REGSET_TLS,
> > > > >  	REGSET_IOPERM32,
> > > > > +	REGSET_CET32,
> > > > >  };
> > > > Why does REGSET_CET64 alias on REGSET_TLS?
> > > In x86_64_regsets[], there is no [REGSET_TLS].  The core dump code
> > > cannot handle holes in the array.
> > Is there a fundamental (ABI) reason for that?
> 
> What I did was, ran Linux with 'slub_debug', and forced a core dump
> (kill -abrt <pid>), then there was a red zone warning in the dmesg.
> My feeling is there could be issues in the core dump code.  These

Kernel development is not about feelings.

Either you can track down the root cause or you cannot. There is no place
for feelings and no place in between. And if you cannot track down the root
cause and explain it proper then the resulting patch is just papering over
the symptoms and will come back to hunt you (or others) sooner than later.

No if, no could, no feelings. Facts is what matters. Really.

Thanks,

	tglx
Pavel Machek July 13, 2018, 6:28 a.m. UTC | #6
> > > to "CET" (which is a well-known acronym for "Central European Time"),
> > > not to CFE?
> > > 
> > 
> > I don't know if I can change that, will find out.
> 
> So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal 
> names, except for the Intel feature bit and any MSR enumeration which can be CET 
> if Intel named it that way, and a short comment explaining the acronym difference.
> 
> Or something like that.

Actually, I don't think CFT is much better -- there's limited number
of TLAs (*). "ENFORCE_FLOW"? "FLOWE"? "EFLOW"?

									Pavel

(*) Three letter accronyms.
Ingo Molnar July 13, 2018, 1:33 p.m. UTC | #7
* Pavel Machek <pavel@ucw.cz> wrote:

> 
> > > > to "CET" (which is a well-known acronym for "Central European Time"),
> > > > not to CFE?
> > > > 
> > > 
> > > I don't know if I can change that, will find out.
> > 
> > So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal 
> > names, except for the Intel feature bit and any MSR enumeration which can be CET 
> > if Intel named it that way, and a short comment explaining the acronym difference.
> > 
> > Or something like that.
> 
> Actually, I don't think CFT is much better -- there's limited number
> of TLAs (*). "ENFORCE_FLOW"? "FLOWE"? "EFLOW"?

Erm, I wanted to say 'CFE', i.e. the abbreviation of 'Control Flow Enforcement'.

But I guess I can live with CET as well ...

Thanks,

	Ingo
Yu, Yu-cheng July 13, 2018, 4:07 p.m. UTC | #8
On Fri, 2018-07-13 at 01:08 +0200, Thomas Gleixner wrote:
> On Thu, 12 Jul 2018, Yu-cheng Yu wrote:
> > 
> > On Thu, 2018-07-12 at 16:03 +0200, Ingo Molnar wrote:
> > > 
> > > * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > > > 
> > > > > 
> > > > > > 
> > > > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > > > > > index e2ee403865eb..ac2bc3a18427 100644
> > > > > > --- a/arch/x86/kernel/ptrace.c
> > > > > > +++ b/arch/x86/kernel/ptrace.c
> > > > > > @@ -49,7 +49,9 @@ enum x86_regset {
> > > > > >  	REGSET_IOPERM64 = REGSET_XFP,
> > > > > >  	REGSET_XSTATE,
> > > > > >  	REGSET_TLS,
> > > > > > +	REGSET_CET64 = REGSET_TLS,
> > > > > >  	REGSET_IOPERM32,
> > > > > > +	REGSET_CET32,
> > > > > >  };
> > > > > Why does REGSET_CET64 alias on REGSET_TLS?
> > > > In x86_64_regsets[], there is no [REGSET_TLS].  The core dump code
> > > > cannot handle holes in the array.
> > > Is there a fundamental (ABI) reason for that?
> > What I did was, ran Linux with 'slub_debug', and forced a core dump
> > (kill -abrt <pid>), then there was a red zone warning in the dmesg.
> > My feeling is there could be issues in the core dump code.  These
> Kernel development is not about feelings.

I got that :-)

> 
> Either you can track down the root cause or you cannot. There is no place
> for feelings and no place in between. And if you cannot track down the root
> cause and explain it proper then the resulting patch is just papering over
> the symptoms and will come back to hunt you (or others) sooner than later.
> 
> No if, no could, no feelings. Facts is what matters. Really.

In kernel/ptrace.c,

find_regset(const struct user_regset_view *view, unsigned int type)
{
	const struct user_regset *regset;
	int n;

	for (n = 0; n < view->n; ++n) {
		regset = view->regsets + n;
		if (regset->core_note_type == type)
			return regset;
	}

	return NULL;
}

If there is a hole in the regset array, the empty slot's
regset->core_note_type is not defined.

We can add some comments near those enum's.

Yu-cheng
Pavel Machek July 14, 2018, 6:27 a.m. UTC | #9
On Fri 2018-07-13 15:33:58, Ingo Molnar wrote:
> 
> * Pavel Machek <pavel@ucw.cz> wrote:
> 
> > 
> > > > > to "CET" (which is a well-known acronym for "Central European Time"),
> > > > > not to CFE?
> > > > > 
> > > > 
> > > > I don't know if I can change that, will find out.
> > > 
> > > So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal 
> > > names, except for the Intel feature bit and any MSR enumeration which can be CET 
> > > if Intel named it that way, and a short comment explaining the acronym difference.
> > > 
> > > Or something like that.
> > 
> > Actually, I don't think CFT is much better -- there's limited number
> > of TLAs (*). "ENFORCE_FLOW"? "FLOWE"? "EFLOW"?
> 
> Erm, I wanted to say 'CFE', i.e. the abbreviation of 'Control Flow Enforcement'.
> 
> But I guess I can live with CET as well ...

Yeah, and I am trying to say that perhaps we should use something
longer than three letters. It will make code longer but easier to
read.
									Pavel
diff mbox

Patch

diff --git a/arch/x86/include/asm/fpu/regset.h b/arch/x86/include/asm/fpu/regset.h
index d5bdffb9d27f..edad0d889084 100644
--- a/arch/x86/include/asm/fpu/regset.h
+++ b/arch/x86/include/asm/fpu/regset.h
@@ -7,11 +7,12 @@ 
 
 #include <linux/regset.h>
 
-extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active;
+extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active,
+				cetregs_active;
 extern user_regset_get_fn fpregs_get, xfpregs_get, fpregs_soft_get,
-				xstateregs_get;
+				xstateregs_get, cetregs_get;
 extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set,
-				 xstateregs_set;
+				 xstateregs_set, cetregs_set;
 
 /*
  * xstateregs_active == regset_fpregs_active. Please refer to the comment
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index bc02f5144b95..7008eb084d36 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -160,6 +160,47 @@  int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 	return ret;
 }
 
+int cetregs_active(struct task_struct *target, const struct user_regset *regset)
+{
+#ifdef CONFIG_X86_INTEL_CET
+	if (target->thread.cet.shstk_enabled || target->thread.cet.ibt_enabled)
+		return regset->n;
+#endif
+	return 0;
+}
+
+int cetregs_get(struct task_struct *target, const struct user_regset *regset,
+		unsigned int pos, unsigned int count,
+		void *kbuf, void __user *ubuf)
+{
+	struct fpu *fpu = &target->thread.fpu;
+	struct cet_user_state *cetregs;
+
+	if (!boot_cpu_has(X86_FEATURE_SHSTK))
+		return -ENODEV;
+
+	cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_MASK_SHSTK_USER);
+
+	fpu__prepare_read(fpu);
+	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, cetregs, 0, -1);
+}
+
+int cetregs_set(struct task_struct *target, const struct user_regset *regset,
+		  unsigned int pos, unsigned int count,
+		  const void *kbuf, const void __user *ubuf)
+{
+	struct fpu *fpu = &target->thread.fpu;
+	struct cet_user_state *cetregs;
+
+	if (!boot_cpu_has(X86_FEATURE_SHSTK))
+		return -ENODEV;
+
+	cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_MASK_SHSTK_USER);
+
+	fpu__prepare_write(fpu);
+	return user_regset_copyin(&pos, &count, &kbuf, &ubuf, cetregs, 0, -1);
+}
+
 #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
 
 /*
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index e2ee403865eb..ac2bc3a18427 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -49,7 +49,9 @@  enum x86_regset {
 	REGSET_IOPERM64 = REGSET_XFP,
 	REGSET_XSTATE,
 	REGSET_TLS,
+	REGSET_CET64 = REGSET_TLS,
 	REGSET_IOPERM32,
+	REGSET_CET32,
 };
 
 struct pt_regs_offset {
@@ -1276,6 +1278,13 @@  static struct user_regset x86_64_regsets[] __ro_after_init = {
 		.size = sizeof(long), .align = sizeof(long),
 		.active = ioperm_active, .get = ioperm_get
 	},
+	[REGSET_CET64] = {
+		.core_note_type = NT_X86_CET,
+		.n = sizeof(struct cet_user_state) / sizeof(u64),
+		.size = sizeof(u64), .align = sizeof(u64),
+		.active = cetregs_active, .get = cetregs_get,
+		.set = cetregs_set
+	},
 };
 
 static const struct user_regset_view user_x86_64_view = {
@@ -1331,6 +1340,13 @@  static struct user_regset x86_32_regsets[] __ro_after_init = {
 		.size = sizeof(u32), .align = sizeof(u32),
 		.active = ioperm_active, .get = ioperm_get
 	},
+	[REGSET_CET32] = {
+		.core_note_type = NT_X86_CET,
+		.n = sizeof(struct cet_user_state) / sizeof(u64),
+		.size = sizeof(u64), .align = sizeof(u64),
+		.active = cetregs_active, .get = cetregs_get,
+		.set = cetregs_set
+	},
 };
 
 static const struct user_regset_view user_x86_32_view = {
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index dc93982b9664..0898ba719fd7 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -401,6 +401,7 @@  typedef struct elf64_shdr {
 #define NT_386_TLS	0x200		/* i386 TLS slots (struct user_desc) */
 #define NT_386_IOPERM	0x201		/* x86 io permission bitmap (1=deny) */
 #define NT_X86_XSTATE	0x202		/* x86 extended state using xsave */
+#define NT_X86_CET	0x203		/* x86 cet state */
 #define NT_S390_HIGH_GPRS	0x300	/* s390 upper register halves */
 #define NT_S390_TIMER	0x301		/* s390 timer register */
 #define NT_S390_TODCMP	0x302		/* s390 TOD clock comparator register */