[PATCH 0/3] Regset cleanups

[PATCH 0/3] Regset cleanups

[Rick Edgecombe]

Hi,

I’m looking for ack’s from Intel reviewer’s before this is ready for official
submission to x86 maintainers. Patch 3 is in core code, so also including
relevant MAINTAINERS file people for that one. If you are not an Intel reviewer,
feel free to ignore this until it has had more review. Glad for any feedback all
the same. I’m also, wondering if this is something that could go through the x86
tree all together or I should split it out.



While working on CET ptrace support, I found some suggested cleanups [0] [1] on
past postings of that patch. So this small series is doing those cleanups and
some related changes.

Way back then, it was noticed that CET ptrace patches were aliasing names in the
enum that indexes the regsets. It turns out this was partly because of a
limitation in core dump code that reads the registers for dumping. But excluding
gaps in the regset array also allows them to be smaller, so just fixing the core
dump code doesn’t remove all need for the specially crafted enum. So series
changes the way the enums are defined such that enum has to be less carefully
crafted, and also fixes the core dump code.

Patch 1 is improving the enums in x86 ptrace code.

Patch 2 is some x86 ptrace code formatting changes suggested by Ingo. [0]

Patch 3 is the fix to the core dump code. Just to be clear, there is no actual
bug fixed. It would only overflow an array if the regset views were not laid out
just so. But the regsets appear to be laid out so that the brittle code is not
broken, from a quick scan of the archs.

Testing consisted of doing some core dumps and seeing that notes were in the
same position, and verifying that the enum’s generated the same ints using
printks.

Thanks,

Rick

[0] https://lore.kernel.org/lkml/20180711102035.GB8574@gmail.com/
[1] https://lore.kernel.org/lkml/A7775E11-8837-4727-921A-C88566FA01AF@amacapital.net/

Rick Edgecombe (3):
  x86: Separate out x86_regset for 32 and 64 bit
  x86: Improve formatting of user_regset arrays
  elf: Don't write past end of notes for regset gap

 arch/x86/kernel/ptrace.c | 165 ++++++++++++++++++++++++---------------
 fs/binfmt_elf.c          |  15 ++--
 2 files changed, 111 insertions(+), 69 deletions(-)

base-commit: 09688c0166e76ce2fb85e86b9d99be8b0084cdf9
-- 
2.17.1

[PATCH 1/3] x86: Separate out x86_regset for 32 and 64 bit

[Rick Edgecombe]

In ptrace, the x86_32_regsets and x86_64_regsets are constructed such that
there are no gaps in the arrays. This appears to be for two reasons. One,
the code in fill_thread_core_info() can't handle the gaps. This will be
addressed in a future patch. And two, not having gaps shrinks the size of
the array in memory.

Both regset arrays draw their indices from a shared enum x86_regset, but 32
bit and 64 bit don't all support the same regsets. In the case of
IA32_EMULATION they can be compiled in at the same time. So this enum has
to be laid out in a special way such that there are no gaps for both
x86_32_regsets and x86_64_regsets. This involves creating aliases for
enum’s that are only in one view or the other, or creating multiple
versions like in the case of REGSET_IOPERM32/REGSET_IOPERM64.

Simplify the construction of these arrays by just fully separating out the
enums for 32 bit and 64 bit. Add some bitsize-free defines for
REGSET_GENERAL and REGSET_FP since they are the only two referred to in
bitsize generic code.

This should have no functional change and is only changing how constants
are generated and named. The enum is local to this file, so it does not
introduce any burden on code calling from other places in the kernel now
having to worry about whether to use a 32 bit or 64 bit enum name.

[1] https://lore.kernel.org/lkml/20180717162502.32274-1-yu-cheng.yu@intel.com/

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/x86/kernel/ptrace.c | 60 ++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 8d2f2f995539..7a4988d13c43 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -45,16 +45,34 @@
 
 #include "tls.h"
 
-enum x86_regset {
-	REGSET_GENERAL,
-	REGSET_FP,
-	REGSET_XFP,
-	REGSET_IOPERM64 = REGSET_XFP,
-	REGSET_XSTATE,
-	REGSET_TLS,
+enum x86_regset_32 {
+	REGSET_GENERAL32,
+	REGSET_FP32,
+	REGSET_XFP32,
+	REGSET_XSTATE32,
+	REGSET_TLS32,
 	REGSET_IOPERM32,
 };
 
+enum x86_regset_64 {
+	REGSET_GENERAL64,
+	REGSET_FP64,
+	REGSET_IOPERM64,
+	REGSET_XSTATE64,
+};
+
+#define REGSET_GENERAL \
+({ \
+	BUILD_BUG_ON((int)REGSET_GENERAL32 != (int)REGSET_GENERAL64); \
+	REGSET_GENERAL32; \
+})
+
+#define REGSET_FP \
+({ \
+	BUILD_BUG_ON((int)REGSET_FP32 != (int)REGSET_FP64); \
+	REGSET_FP32; \
+})
+
 struct pt_regs_offset {
 	const char *name;
 	int offset;
@@ -789,13 +807,13 @@ long arch_ptrace(struct task_struct *child, long request,
 #ifdef CONFIG_X86_32
 	case PTRACE_GETFPXREGS:	/* Get the child extended FPU state. */
 		return copy_regset_to_user(child, &user_x86_32_view,
-					   REGSET_XFP,
+					   REGSET_XFP32,
 					   0, sizeof(struct user_fxsr_struct),
 					   datap) ? -EIO : 0;
 
 	case PTRACE_SETFPXREGS:	/* Set the child extended FPU state. */
 		return copy_regset_from_user(child, &user_x86_32_view,
-					     REGSET_XFP,
+					     REGSET_XFP32,
 					     0, sizeof(struct user_fxsr_struct),
 					     datap) ? -EIO : 0;
 #endif
@@ -1087,13 +1105,13 @@ static long ia32_arch_ptrace(struct task_struct *child, compat_long_t request,
 
 	case PTRACE_GETFPXREGS:	/* Get the child extended FPU state. */
 		return copy_regset_to_user(child, &user_x86_32_view,
-					   REGSET_XFP, 0,
+					   REGSET_XFP32, 0,
 					   sizeof(struct user32_fxsr_struct),
 					   datap);
 
 	case PTRACE_SETFPXREGS:	/* Set the child extended FPU state. */
 		return copy_regset_from_user(child, &user_x86_32_view,
-					     REGSET_XFP, 0,
+					     REGSET_XFP32, 0,
 					     sizeof(struct user32_fxsr_struct),
 					     datap);
 
@@ -1216,19 +1234,19 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 #ifdef CONFIG_X86_64
 
 static struct user_regset x86_64_regsets[] __ro_after_init = {
-	[REGSET_GENERAL] = {
+	[REGSET_GENERAL64] = {
 		.core_note_type = NT_PRSTATUS,
 		.n = sizeof(struct user_regs_struct) / sizeof(long),
 		.size = sizeof(long), .align = sizeof(long),
 		.regset_get = genregs_get, .set = genregs_set
 	},
-	[REGSET_FP] = {
+	[REGSET_FP64] = {
 		.core_note_type = NT_PRFPREG,
 		.n = sizeof(struct fxregs_state) / sizeof(long),
 		.size = sizeof(long), .align = sizeof(long),
 		.active = regset_xregset_fpregs_active, .regset_get = xfpregs_get, .set = xfpregs_set
 	},
-	[REGSET_XSTATE] = {
+	[REGSET_XSTATE64] = {
 		.core_note_type = NT_X86_XSTATE,
 		.size = sizeof(u64), .align = sizeof(u64),
 		.active = xstateregs_active, .regset_get = xstateregs_get,
@@ -1257,31 +1275,31 @@ static const struct user_regset_view user_x86_64_view = {
 
 #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
 static struct user_regset x86_32_regsets[] __ro_after_init = {
-	[REGSET_GENERAL] = {
+	[REGSET_GENERAL32] = {
 		.core_note_type = NT_PRSTATUS,
 		.n = sizeof(struct user_regs_struct32) / sizeof(u32),
 		.size = sizeof(u32), .align = sizeof(u32),
 		.regset_get = genregs32_get, .set = genregs32_set
 	},
-	[REGSET_FP] = {
+	[REGSET_FP32] = {
 		.core_note_type = NT_PRFPREG,
 		.n = sizeof(struct user_i387_ia32_struct) / sizeof(u32),
 		.size = sizeof(u32), .align = sizeof(u32),
 		.active = regset_fpregs_active, .regset_get = fpregs_get, .set = fpregs_set
 	},
-	[REGSET_XFP] = {
+	[REGSET_XFP32] = {
 		.core_note_type = NT_PRXFPREG,
 		.n = sizeof(struct fxregs_state) / sizeof(u32),
 		.size = sizeof(u32), .align = sizeof(u32),
 		.active = regset_xregset_fpregs_active, .regset_get = xfpregs_get, .set = xfpregs_set
 	},
-	[REGSET_XSTATE] = {
+	[REGSET_XSTATE32] = {
 		.core_note_type = NT_X86_XSTATE,
 		.size = sizeof(u64), .align = sizeof(u64),
 		.active = xstateregs_active, .regset_get = xstateregs_get,
 		.set = xstateregs_set
 	},
-	[REGSET_TLS] = {
+	[REGSET_TLS32] = {
 		.core_note_type = NT_386_TLS,
 		.n = GDT_ENTRY_TLS_ENTRIES, .bias = GDT_ENTRY_TLS_MIN,
 		.size = sizeof(struct user_desc),
@@ -1312,10 +1330,10 @@ u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 void __init update_regset_xstate_info(unsigned int size, u64 xstate_mask)
 {
 #ifdef CONFIG_X86_64
-	x86_64_regsets[REGSET_XSTATE].n = size / sizeof(u64);
+	x86_64_regsets[REGSET_XSTATE64].n = size / sizeof(u64);
 #endif
 #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
-	x86_32_regsets[REGSET_XSTATE].n = size / sizeof(u64);
+	x86_32_regsets[REGSET_XSTATE32].n = size / sizeof(u64);
 #endif
 	xstate_fx_sw_bytes[USER_XSTATE_XCR0_WORD] = xstate_mask;
 }
-- 
2.17.1

[PATCH 2/3] x86: Improve formatting of user_regset arrays

[Rick Edgecombe]

Back in 2018, Ingo Molnar suggested[0] to improve the formatting of the
struct user_regset arrays. They have multiple member initializations per
line and some lines exceed 100 chars. Reformat them like he suggested.

[0] https://lore.kernel.org/lkml/20180711102035.GB8574@gmail.com/

Suggested-by: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/x86/kernel/ptrace.c | 105 ++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 41 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 7a4988d13c43..30355f8d635e 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1235,28 +1235,37 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 
 static struct user_regset x86_64_regsets[] __ro_after_init = {
 	[REGSET_GENERAL64] = {
-		.core_note_type = NT_PRSTATUS,
-		.n = sizeof(struct user_regs_struct) / sizeof(long),
-		.size = sizeof(long), .align = sizeof(long),
-		.regset_get = genregs_get, .set = genregs_set
+		.core_note_type	= NT_PRSTATUS,
+		.n		= sizeof(struct user_regs_struct) / sizeof(long),
+		.size		= sizeof(long),
+		.align		= sizeof(long),
+		.regset_get	= genregs_get,
+		.set		= genregs_set
 	},
 	[REGSET_FP64] = {
 		.core_note_type = NT_PRFPREG,
-		.n = sizeof(struct fxregs_state) / sizeof(long),
-		.size = sizeof(long), .align = sizeof(long),
-		.active = regset_xregset_fpregs_active, .regset_get = xfpregs_get, .set = xfpregs_set
+		.n		= sizeof(struct fxregs_state) / sizeof(long),
+		.size		= sizeof(long),
+		.align		= sizeof(long),
+		.active		= regset_xregset_fpregs_active,
+		.regset_get	= xfpregs_get,
+		.set		= xfpregs_set
 	},
 	[REGSET_XSTATE64] = {
-		.core_note_type = NT_X86_XSTATE,
-		.size = sizeof(u64), .align = sizeof(u64),
-		.active = xstateregs_active, .regset_get = xstateregs_get,
-		.set = xstateregs_set
+		.core_note_type	= NT_X86_XSTATE,
+		.size		= sizeof(u64),
+		.align		= sizeof(u64),
+		.active		= xstateregs_active,
+		.regset_get	= xstateregs_get,
+		.set		= xstateregs_set
 	},
 	[REGSET_IOPERM64] = {
-		.core_note_type = NT_386_IOPERM,
-		.n = IO_BITMAP_LONGS,
-		.size = sizeof(long), .align = sizeof(long),
-		.active = ioperm_active, .regset_get = ioperm_get
+		.core_note_type	= NT_386_IOPERM,
+		.n		= IO_BITMAP_LONGS,
+		.size		= sizeof(long),
+		.align		= sizeof(long),
+		.active		= ioperm_active,
+		.regset_get	= ioperm_get
 	},
 };
 
@@ -1276,42 +1285,56 @@ static const struct user_regset_view user_x86_64_view = {
 #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
 static struct user_regset x86_32_regsets[] __ro_after_init = {
 	[REGSET_GENERAL32] = {
-		.core_note_type = NT_PRSTATUS,
-		.n = sizeof(struct user_regs_struct32) / sizeof(u32),
-		.size = sizeof(u32), .align = sizeof(u32),
-		.regset_get = genregs32_get, .set = genregs32_set
+		.core_note_type	= NT_PRSTATUS,
+		.n		= sizeof(struct user_regs_struct32) / sizeof(u32),
+		.size		= sizeof(u32),
+		.align		= sizeof(u32),
+		.regset_get	= genregs32_get,
+		.set		= genregs32_set
 	},
 	[REGSET_FP32] = {
-		.core_note_type = NT_PRFPREG,
-		.n = sizeof(struct user_i387_ia32_struct) / sizeof(u32),
-		.size = sizeof(u32), .align = sizeof(u32),
-		.active = regset_fpregs_active, .regset_get = fpregs_get, .set = fpregs_set
+		.core_note_type	= NT_PRFPREG,
+		.n		= sizeof(struct user_i387_ia32_struct) / sizeof(u32),
+		.size		= sizeof(u32),
+		.align		= sizeof(u32),
+		.active		= regset_fpregs_active,
+		.regset_get	= fpregs_get,
+		.set		= fpregs_set
 	},
 	[REGSET_XFP32] = {
-		.core_note_type = NT_PRXFPREG,
-		.n = sizeof(struct fxregs_state) / sizeof(u32),
-		.size = sizeof(u32), .align = sizeof(u32),
-		.active = regset_xregset_fpregs_active, .regset_get = xfpregs_get, .set = xfpregs_set
+		.core_note_type	= NT_PRXFPREG,
+		.n		= sizeof(struct fxregs_state) / sizeof(u32),
+		.size		= sizeof(u32),
+		.align		= sizeof(u32),
+		.active		= regset_xregset_fpregs_active,
+		.regset_get	= xfpregs_get,
+		.set		= xfpregs_set
 	},
 	[REGSET_XSTATE32] = {
-		.core_note_type = NT_X86_XSTATE,
-		.size = sizeof(u64), .align = sizeof(u64),
-		.active = xstateregs_active, .regset_get = xstateregs_get,
-		.set = xstateregs_set
+		.core_note_type	= NT_X86_XSTATE,
+		.size		= sizeof(u64),
+		.align		= sizeof(u64),
+		.active		= xstateregs_active,
+		.regset_get	= xstateregs_get,
+		.set		= xstateregs_set
 	},
 	[REGSET_TLS32] = {
-		.core_note_type = NT_386_TLS,
-		.n = GDT_ENTRY_TLS_ENTRIES, .bias = GDT_ENTRY_TLS_MIN,
-		.size = sizeof(struct user_desc),
-		.align = sizeof(struct user_desc),
-		.active = regset_tls_active,
-		.regset_get = regset_tls_get, .set = regset_tls_set
+		.core_note_type	= NT_386_TLS,
+		.n		= GDT_ENTRY_TLS_ENTRIES,
+		.bias		= GDT_ENTRY_TLS_MIN,
+		.size		= sizeof(struct user_desc),
+		.align		= sizeof(struct user_desc),
+		.active		= regset_tls_active,
+		.regset_get	= regset_tls_get,
+		.set		= regset_tls_set
 	},
 	[REGSET_IOPERM32] = {
-		.core_note_type = NT_386_IOPERM,
-		.n = IO_BITMAP_BYTES / sizeof(u32),
-		.size = sizeof(u32), .align = sizeof(u32),
-		.active = ioperm_active, .regset_get = ioperm_get
+		.core_note_type	= NT_386_IOPERM,
+		.n		= IO_BITMAP_BYTES / sizeof(u32),
+		.size		= sizeof(u32),
+		.align		= sizeof(u32),
+		.active		= ioperm_active,
+		.regset_get	= ioperm_get
 	},
 };
 
-- 
2.17.1

[PATCH 3/3] elf: Don't write past end of notes for regset gap

[Rick Edgecombe]

In fill_thread_core_info() the ptrace accessible registers are collected
to be written out as notes in a core file. The note array is allocated
from a size calculated by iterating the user regset view, and counting the
regsets that have a non-zero core_note_type. However, this only allows for
there to be non-zero core_note_type at the end of the regset view. If
there are any gaps in the middle, fill_thread_core_info() will overflow the
note allocation, as it iterates over the size of the view and the
allocation would be smaller than that.

There doesn't appear to be any arch that has gaps such that they exceed
the notes allocation, but the code is brittle and tries to support
something it doesn't. It could be fixed by increasing the allocation size,
but instead just have the note collecting code utilize the array better.
This way the allocation can stay smaller.

Even in the case of no arch's that have gaps in their regset views, this
introduces a change in the resulting indicies of t->notes. It does not
introduce any changes to the core file itself, because any blank notes are
skipped in write_note_info().

This fix is derrived from an earlier one[0] by Yu-cheng Yu.

[0] https://lore.kernel.org/lkml/20180717162502.32274-1-yu-cheng.yu@intel.com/

Co-developed-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 fs/binfmt_elf.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index d61543fbd652..a48f85e3c017 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1757,7 +1757,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 				 const struct user_regset_view *view,
 				 long signr, size_t *total)
 {
-	unsigned int i;
+	unsigned int note_iter, view_iter;
 
 	/*
 	 * NT_PRSTATUS is the one special case, because the regset data
@@ -1777,11 +1777,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 
 	/*
 	 * Each other regset might generate a note too.  For each regset
-	 * that has no core_note_type or is inactive, we leave t->notes[i]
-	 * all zero and we'll know to skip writing it later.
+	 * that has no core_note_type or is inactive, skip it.
 	 */
-	for (i = 1; i < view->n; ++i) {
-		const struct user_regset *regset = &view->regsets[i];
+	note_iter = 1;
+	for (view_iter = 1; view_iter < view->n; ++view_iter) {
+		const struct user_regset *regset = &view->regsets[view_iter];
 		int note_type = regset->core_note_type;
 		bool is_fpreg = note_type == NT_PRFPREG;
 		void *data;
@@ -1800,10 +1800,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 		if (is_fpreg)
 			SET_PR_FPVALID(&t->prstatus);
 
-		fill_note(&t->notes[i], is_fpreg ? "CORE" : "LINUX",
+		fill_note(&t->notes[note_iter], is_fpreg ? "CORE" : "LINUX",
 			  note_type, ret, data);
 
-		*total += notesize(&t->notes[i]);
+		*total += notesize(&t->notes[note_iter]);
+		note_iter++;
 	}
 
 	return 1;
-- 
2.17.1

Re: [PATCH 3/3] elf: Don't write past end of notes for regset gap

[Kees Cook]

On Tue, Mar 15, 2022 at 01:17:06PM -0700, Rick Edgecombe wrote:
> In fill_thread_core_info() the ptrace accessible registers are collected
> to be written out as notes in a core file. The note array is allocated
> from a size calculated by iterating the user regset view, and counting the
> regsets that have a non-zero core_note_type. However, this only allows for
> there to be non-zero core_note_type at the end of the regset view. If
> there are any gaps in the middle, fill_thread_core_info() will overflow the
> note allocation, as it iterates over the size of the view and the
> allocation would be smaller than that.
> 
> There doesn't appear to be any arch that has gaps such that they exceed
> the notes allocation, but the code is brittle and tries to support
> something it doesn't. It could be fixed by increasing the allocation size,
> but instead just have the note collecting code utilize the array better.
> This way the allocation can stay smaller.
> 
> Even in the case of no arch's that have gaps in their regset views, this
> introduces a change in the resulting indicies of t->notes. It does not
> introduce any changes to the core file itself, because any blank notes are
> skipped in write_note_info().

Hm, yes, fill_note_info() does an initial count & allocate. Then
fill_thread_core_info() writes them.

> 
> This fix is derrived from an earlier one[0] by Yu-cheng Yu.
> 
> [0] https://lore.kernel.org/lkml/20180717162502.32274-1-yu-cheng.yu@intel.com/
> 
> Co-developed-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>  fs/binfmt_elf.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index d61543fbd652..a48f85e3c017 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1757,7 +1757,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
>  				 const struct user_regset_view *view,
>  				 long signr, size_t *total)
>  {
> -	unsigned int i;
> +	unsigned int note_iter, view_iter;
>  
>  	/*
>  	 * NT_PRSTATUS is the one special case, because the regset data
> @@ -1777,11 +1777,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
>  
>  	/*
>  	 * Each other regset might generate a note too.  For each regset
> -	 * that has no core_note_type or is inactive, we leave t->notes[i]
> -	 * all zero and we'll know to skip writing it later.
> +	 * that has no core_note_type or is inactive, skip it.
>  	 */
> -	for (i = 1; i < view->n; ++i) {
> -		const struct user_regset *regset = &view->regsets[i];
> +	note_iter = 1;
> +	for (view_iter = 1; view_iter < view->n; ++view_iter) {
> +		const struct user_regset *regset = &view->regsets[view_iter];
>  		int note_type = regset->core_note_type;
>  		bool is_fpreg = note_type == NT_PRFPREG;
>  		void *data;
> @@ -1800,10 +1800,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
>  		if (is_fpreg)
>  			SET_PR_FPVALID(&t->prstatus);
>  

info->thread_notes contains the count. Since fill_thread_core_info()
passes a info member by reference, it might make sense to just pass info
itself, then the size can be written and a bounds-check can be added
here:

		if (WARN_ON_ONCE(i >= info->thread_notes))
			continue;

> -		fill_note(&t->notes[i], is_fpreg ? "CORE" : "LINUX",
> +		fill_note(&t->notes[note_iter], is_fpreg ? "CORE" : "LINUX",
>  			  note_type, ret, data);
>  
> -		*total += notesize(&t->notes[i]);
> +		*total += notesize(&t->notes[note_iter]);
> +		note_iter++;
>  	}
>  
>  	return 1;
> -- 
> 2.17.1
> 

If that can get adjusted, I'd be happy to carry this patch separately in
for-next/execve (or I can Ack it and it can go with the others here in
the series).

(And in a perfect world, I'd *love* a KUnit test to exercise this logic,
but I don't think we're there yet with function mocking, etc.)

-- 
Kees Cook

Re: [PATCH 2/3] x86: Improve formatting of user_regset arrays

[Kees Cook]

On Tue, Mar 15, 2022 at 01:17:05PM -0700, Rick Edgecombe wrote:
> Back in 2018, Ingo Molnar suggested[0] to improve the formatting of the
> struct user_regset arrays. They have multiple member initializations per
> line and some lines exceed 100 chars. Reformat them like he suggested.
> 
> [0] https://lore.kernel.org/lkml/20180711102035.GB8574@gmail.com/
> 
> Suggested-by: Ingo Molnar <mingo@redhat.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

Much easier to read; yes!

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 1/3] x86: Separate out x86_regset for 32 and 64 bit

[Kees Cook]

On Tue, Mar 15, 2022 at 01:17:04PM -0700, Rick Edgecombe wrote:
> In ptrace, the x86_32_regsets and x86_64_regsets are constructed such that
> there are no gaps in the arrays. This appears to be for two reasons. One,
> the code in fill_thread_core_info() can't handle the gaps. This will be
> addressed in a future patch. And two, not having gaps shrinks the size of
> the array in memory.
> 
> Both regset arrays draw their indices from a shared enum x86_regset, but 32
> bit and 64 bit don't all support the same regsets. In the case of
> IA32_EMULATION they can be compiled in at the same time. So this enum has
> to be laid out in a special way such that there are no gaps for both
> x86_32_regsets and x86_64_regsets. This involves creating aliases for
> enum’s that are only in one view or the other, or creating multiple
> versions like in the case of REGSET_IOPERM32/REGSET_IOPERM64.
> 
> Simplify the construction of these arrays by just fully separating out the
> enums for 32 bit and 64 bit. Add some bitsize-free defines for
> REGSET_GENERAL and REGSET_FP since they are the only two referred to in
> bitsize generic code.
> 
> This should have no functional change and is only changing how constants
> are generated and named. The enum is local to this file, so it does not
> introduce any burden on code calling from other places in the kernel now
> having to worry about whether to use a 32 bit or 64 bit enum name.
> 
> [1] https://lore.kernel.org/lkml/20180717162502.32274-1-yu-cheng.yu@intel.com/
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

Have you verified there's no binary difference in machine code output?

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 3/3] elf: Don't write past end of notes for regset gap

[Edgecombe, Rick P]

On Tue, 2022-03-15 at 13:37 -0700, Kees Cook wrote:
> >        /*
> >         * Each other regset might generate a note too.  For each
> > regset
> > -      * that has no core_note_type or is inactive, we leave t-
> > >notes[i]
> > -      * all zero and we'll know to skip writing it later.
> > +      * that has no core_note_type or is inactive, skip it.
> >         */
> > -     for (i = 1; i < view->n; ++i) {
> > -             const struct user_regset *regset = &view->regsets[i];
> > +     note_iter = 1;
> > +     for (view_iter = 1; view_iter < view->n; ++view_iter) {
> > +             const struct user_regset *regset = &view-
> > >regsets[view_iter];
> >                int note_type = regset->core_note_type;
> >                bool is_fpreg = note_type == NT_PRFPREG;
> >                void *data;
> > @@ -1800,10 +1800,11 @@ static int fill_thread_core_info(struct
> > elf_thread_core_info *t,
> >                if (is_fpreg)
> >                        SET_PR_FPVALID(&t->prstatus);
> >   
> 
> info->thread_notes contains the count. Since fill_thread_core_info()
> passes a info member by reference, it might make sense to just pass
> info
> itself, then the size can be written and a bounds-check can be added
> here:
> 
>                 if (WARN_ON_ONCE(i >= info->thread_notes))
>                         continue;

Hi Kees,

Thanks for the quick response.

Are you saying in addition to utilizing the allocation better, also
catch if the allocation is still too small? Or do this check instead of
the change in how to utilize the array, and then maintain the
restriction on having gaps in the regsets?

If it's the former, it seems a bit excessive since the allocation and
usage are only one function call away from each other and the logic is
now such that it can't overflow. I can add it if you want.

If it's to just warn on the gaps, it could also be done directly like:
/* Don't expect gaps in regset views */
WARN_ON(!regset->regset_get);

And it might be a little clearer of a hint about this expectation of
the arch's.

Let me know what you prefer and I can make the change.

Re: [PATCH 2/3] x86: Improve formatting of user_regset arrays

[Edgecombe, Rick P]

On Tue, 2022-03-15 at 13:38 -0700, Kees Cook wrote:
> Reviewed-by: Kees Cook <keescook@chromium.org>

Thanks!

Re: [PATCH 1/3] x86: Separate out x86_regset for 32 and 64 bit

[Edgecombe, Rick P]

On Tue, 2022-03-15 at 13:41 -0700, Kees Cook wrote:
> Have you verified there's no binary difference in machine code
> output?

There actually was a different in the binaries. I investigated a bit,
and it seemed at least part of it was due to the line numbers changing
the WARN_ON()s. But otherwise, I assumed some compiler optimization
must have been bumped.

Re: [PATCH 1/3] x86: Separate out x86_regset for 32 and 64 bit

[Eric W. Biederman]

Rick Edgecombe <rick.p.edgecombe@intel.com> writes:

> In ptrace, the x86_32_regsets and x86_64_regsets are constructed such that
> there are no gaps in the arrays. This appears to be for two reasons. One,
> the code in fill_thread_core_info() can't handle the gaps. This will be
> addressed in a future patch. And two, not having gaps shrinks the size of
> the array in memory.
>
> Both regset arrays draw their indices from a shared enum x86_regset, but 32
> bit and 64 bit don't all support the same regsets. In the case of
> IA32_EMULATION they can be compiled in at the same time. So this enum has
> to be laid out in a special way such that there are no gaps for both
> x86_32_regsets and x86_64_regsets. This involves creating aliases for
> enum’s that are only in one view or the other, or creating multiple
> versions like in the case of REGSET_IOPERM32/REGSET_IOPERM64.
>
> Simplify the construction of these arrays by just fully separating out the
> enums for 32 bit and 64 bit. Add some bitsize-free defines for
> REGSET_GENERAL and REGSET_FP since they are the only two referred to in
> bitsize generic code.
>
> This should have no functional change and is only changing how constants
> are generated and named. The enum is local to this file, so it does not
> introduce any burden on code calling from other places in the kernel now
> having to worry about whether to use a 32 bit or 64 bit enum name.
>
> [1] https://lore.kernel.org/lkml/20180717162502.32274-1-yu-cheng.yu@intel.com/
>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>  arch/x86/kernel/ptrace.c | 60 ++++++++++++++++++++++++++--------------
>  1 file changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 8d2f2f995539..7a4988d13c43 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -45,16 +45,34 @@
>  
>  #include "tls.h"
>  
> -enum x86_regset {
> -	REGSET_GENERAL,
> -	REGSET_FP,
> -	REGSET_XFP,
> -	REGSET_IOPERM64 = REGSET_XFP,
> -	REGSET_XSTATE,
> -	REGSET_TLS,
> +enum x86_regset_32 {
> +	REGSET_GENERAL32,
> +	REGSET_FP32,
> +	REGSET_XFP32,
> +	REGSET_XSTATE32,
> +	REGSET_TLS32,
>  	REGSET_IOPERM32,
>  };
>  
> +enum x86_regset_64 {
> +	REGSET_GENERAL64,
> +	REGSET_FP64,
> +	REGSET_IOPERM64,
> +	REGSET_XSTATE64,
> +};

So I am looking at this and am wondering if the enums should be:

enum x86_32_regset {
	REGSET32_GENERAL,
        REGSET32_FP,
        REGSET32_XFP,
        REGSET32_XSTATE,
        REGSET32_TLS,
        REGSET32_IOPERM32,
};

enum x86_64_regset {
	REGSET64_GENERAL,
        REGSET64_FP,
        REGSET64_IOPERM64,
        REGSET64_XSTATE,
};


That is named in such a way that it emphasizes that the difference is
the architecture.  Otherwise it reads like the difference is the size of
the registers in the regset.  I am pretty certain that in your
REGSET_FP32 and REGSET_FP64 all of the registers are 80 bits long.

Eric


> +
> +#define REGSET_GENERAL \
> +({ \
> +	BUILD_BUG_ON((int)REGSET_GENERAL32 != (int)REGSET_GENERAL64); \
> +	REGSET_GENERAL32; \
> +})
> +
> +#define REGSET_FP \

> +	BUILD_BUG_ON((int)REGSET_FP32 != (int)REGSET_FP64); \
> +	REGSET_FP32; \
> +})
> +
>  struct pt_regs_offset {
>  	const char *name;
>  	int offset;
> @@ -789,13 +807,13 @@ long arch_ptrace(struct task_struct *child, long request,
>  #ifdef CONFIG_X86_32
>  	case PTRACE_GETFPXREGS:	/* Get the child extended FPU state. */
>  		return copy_regset_to_user(child, &user_x86_32_view,
> -					   REGSET_XFP,
> +					   REGSET_XFP32,
>  					   0, sizeof(struct user_fxsr_struct),
>  					   datap) ? -EIO : 0;
>  
>  	case PTRACE_SETFPXREGS:	/* Set the child extended FPU state. */
>  		return copy_regset_from_user(child, &user_x86_32_view,
> -					     REGSET_XFP,
> +					     REGSET_XFP32,
>  					     0, sizeof(struct user_fxsr_struct),
>  					     datap) ? -EIO : 0;
>  #endif
> @@ -1087,13 +1105,13 @@ static long ia32_arch_ptrace(struct task_struct *child, compat_long_t request,
>  
>  	case PTRACE_GETFPXREGS:	/* Get the child extended FPU state. */
>  		return copy_regset_to_user(child, &user_x86_32_view,
> -					   REGSET_XFP, 0,
> +					   REGSET_XFP32, 0,
>  					   sizeof(struct user32_fxsr_struct),
>  					   datap);
>  
>  	case PTRACE_SETFPXREGS:	/* Set the child extended FPU state. */
>  		return copy_regset_from_user(child, &user_x86_32_view,
> -					     REGSET_XFP, 0,
> +					     REGSET_XFP32, 0,
>  					     sizeof(struct user32_fxsr_struct),
>  					     datap);
>  
> @@ -1216,19 +1234,19 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
>  #ifdef CONFIG_X86_64
>  
>  static struct user_regset x86_64_regsets[] __ro_after_init = {
> -	[REGSET_GENERAL] = {
> +	[REGSET_GENERAL64] = {
>  		.core_note_type = NT_PRSTATUS,
>  		.n = sizeof(struct user_regs_struct) / sizeof(long),
>  		.size = sizeof(long), .align = sizeof(long),
>  		.regset_get = genregs_get, .set = genregs_set
>  	},
> -	[REGSET_FP] = {
> +	[REGSET_FP64] = {
>  		.core_note_type = NT_PRFPREG,
>  		.n = sizeof(struct fxregs_state) / sizeof(long),
>  		.size = sizeof(long), .align = sizeof(long),
>  		.active = regset_xregset_fpregs_active, .regset_get = xfpregs_get, .set = xfpregs_set
>  	},
> -	[REGSET_XSTATE] = {
> +	[REGSET_XSTATE64] = {
>  		.core_note_type = NT_X86_XSTATE,
>  		.size = sizeof(u64), .align = sizeof(u64),
>  		.active = xstateregs_active, .regset_get = xstateregs_get,
> @@ -1257,31 +1275,31 @@ static const struct user_regset_view user_x86_64_view = {
>  
>  #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
>  static struct user_regset x86_32_regsets[] __ro_after_init = {
> -	[REGSET_GENERAL] = {
> +	[REGSET_GENERAL32] = {
>  		.core_note_type = NT_PRSTATUS,
>  		.n = sizeof(struct user_regs_struct32) / sizeof(u32),
>  		.size = sizeof(u32), .align = sizeof(u32),
>  		.regset_get = genregs32_get, .set = genregs32_set
>  	},
> -	[REGSET_FP] = {
> +	[REGSET_FP32] = {
>  		.core_note_type = NT_PRFPREG,
>  		.n = sizeof(struct user_i387_ia32_struct) / sizeof(u32),
>  		.size = sizeof(u32), .align = sizeof(u32),
>  		.active = regset_fpregs_active, .regset_get = fpregs_get, .set = fpregs_set
>  	},
> -	[REGSET_XFP] = {
> +	[REGSET_XFP32] = {
>  		.core_note_type = NT_PRXFPREG,
>  		.n = sizeof(struct fxregs_state) / sizeof(u32),
>  		.size = sizeof(u32), .align = sizeof(u32),
>  		.active = regset_xregset_fpregs_active, .regset_get = xfpregs_get, .set = xfpregs_set
>  	},
> -	[REGSET_XSTATE] = {
> +	[REGSET_XSTATE32] = {
>  		.core_note_type = NT_X86_XSTATE,
>  		.size = sizeof(u64), .align = sizeof(u64),
>  		.active = xstateregs_active, .regset_get = xstateregs_get,
>  		.set = xstateregs_set
>  	},
> -	[REGSET_TLS] = {
> +	[REGSET_TLS32] = {
>  		.core_note_type = NT_386_TLS,
>  		.n = GDT_ENTRY_TLS_ENTRIES, .bias = GDT_ENTRY_TLS_MIN,
>  		.size = sizeof(struct user_desc),
> @@ -1312,10 +1330,10 @@ u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
>  void __init update_regset_xstate_info(unsigned int size, u64 xstate_mask)
>  {
>  #ifdef CONFIG_X86_64
> -	x86_64_regsets[REGSET_XSTATE].n = size / sizeof(u64);
> +	x86_64_regsets[REGSET_XSTATE64].n = size / sizeof(u64);
>  #endif
>  #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
> -	x86_32_regsets[REGSET_XSTATE].n = size / sizeof(u64);
> +	x86_32_regsets[REGSET_XSTATE32].n = size / sizeof(u64);
>  #endif
>  	xstate_fx_sw_bytes[USER_XSTATE_XCR0_WORD] = xstate_mask;
>  }

Re: [PATCH 1/3] x86: Separate out x86_regset for 32 and 64 bit

[Edgecombe, Rick P]

On Tue, 2022-03-15 at 18:01 -0500, Eric W. Biederman wrote:
> So I am looking at this and am wondering if the enums should be:
> 
> enum x86_32_regset {
>         REGSET32_GENERAL,
>         REGSET32_FP,
>         REGSET32_XFP,
>         REGSET32_XSTATE,
>         REGSET32_TLS,
>         REGSET32_IOPERM32,
> };
> 
> enum x86_64_regset {
>         REGSET64_GENERAL,
>         REGSET64_FP,
>         REGSET64_IOPERM64,
>         REGSET64_XSTATE,
> };
> 
> 
> That is named in such a way that it emphasizes that the difference is
> the architecture.  Otherwise it reads like the difference is the size
> of
> the registers in the regset.  I am pretty certain that in your
> REGSET_FP32 and REGSET_FP64 all of the registers are 80 bits long.

Yes, that makes sense. I had just copied the format
of REGSET_IOPERM32/REGSET_IOPERM64, but I'll change it like you suggest
here.

Thanks,

Rick

Re: [PATCH 3/3] elf: Don't write past end of notes for regset gap

[Kees Cook]

On Tue, Mar 15, 2022 at 09:48:29PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2022-03-15 at 13:37 -0700, Kees Cook wrote:
> > >        /*
> > >         * Each other regset might generate a note too.  For each
> > > regset
> > > -      * that has no core_note_type or is inactive, we leave t-
> > > >notes[i]
> > > -      * all zero and we'll know to skip writing it later.
> > > +      * that has no core_note_type or is inactive, skip it.
> > >         */
> > > -     for (i = 1; i < view->n; ++i) {
> > > -             const struct user_regset *regset = &view->regsets[i];
> > > +     note_iter = 1;
> > > +     for (view_iter = 1; view_iter < view->n; ++view_iter) {
> > > +             const struct user_regset *regset = &view-
> > > >regsets[view_iter];
> > >                int note_type = regset->core_note_type;
> > >                bool is_fpreg = note_type == NT_PRFPREG;
> > >                void *data;
> > > @@ -1800,10 +1800,11 @@ static int fill_thread_core_info(struct
> > > elf_thread_core_info *t,
> > >                if (is_fpreg)
> > >                        SET_PR_FPVALID(&t->prstatus);
> > >   
> > 
> > info->thread_notes contains the count. Since fill_thread_core_info()
> > passes a info member by reference, it might make sense to just pass
> > info
> > itself, then the size can be written and a bounds-check can be added
> > here:
> > 
> >                 if (WARN_ON_ONCE(i >= info->thread_notes))
> >                         continue;
> 
> Hi Kees,
> 
> Thanks for the quick response.
> 
> Are you saying in addition to utilizing the allocation better, also
> catch if the allocation is still too small? Or do this check instead of
> the change in how to utilize the array, and then maintain the
> restriction on having gaps in the regsets?

What I want is to have writers of dynamically-sized arrays able to do
the bounds check in the same place the write happens, so passing "info"
makes sense to me.

-- 
Kees Cook

Re: [PATCH 1/3] x86: Separate out x86_regset for 32 and 64 bit

[Kees Cook]

On Tue, Mar 15, 2022 at 09:53:13PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2022-03-15 at 13:41 -0700, Kees Cook wrote:
> > Have you verified there's no binary difference in machine code
> > output?
> 
> There actually was a different in the binaries. I investigated a bit,
> and it seemed at least part of it was due to the line numbers changing
> the WARN_ON()s. But otherwise, I assumed some compiler optimization
> must have been bumped.

Right, you can ignore all the debugging line number changes.
"diffoscope" should help see the difference by section. As long as the
actual object code isn't changing, you should be good.

-- 
Kees Cook

Re: [PATCH 1/3] x86: Separate out x86_regset for 32 and 64 bit

[Edgecombe, Rick P]

On Tue, 2022-03-15 at 19:48 -0700, Kees Cook wrote:
> On Tue, Mar 15, 2022 at 09:53:13PM +0000, Edgecombe, Rick P wrote:
> > On Tue, 2022-03-15 at 13:41 -0700, Kees Cook wrote:
> > > Have you verified there's no binary difference in machine code
> > > output?
> > 
> > There actually was a different in the binaries. I investigated a
> > bit,
> > and it seemed at least part of it was due to the line numbers
> > changing
> > the WARN_ON()s. But otherwise, I assumed some compiler optimization
> > must have been bumped.
> 
> Right, you can ignore all the debugging line number changes.
> "diffoscope" should help see the difference by section. As long as
> the
> actual object code isn't changing, you should be good.

What I did originally was objdump -D ptrace.o and diff that. Then I
slowly reduced changes to see what was generating the difference. When
I maintained the line numbers from the original version, and simply
converted the enum to defines, it still generated slightly different
code in places that didn't seem to connected to the changes. So I
figured the compiler was doing something, and relied on checking that
the actual constants didn't change in value.

This morning I tried again to figure out what was causing the
difference. If I strip debug symbols, remove the BUILD_BUG_ON()s and
reformat the enums such that the line numbers are the same below the
enums then the objdump output is identical.

I think what is happening in this debug stripped test, is that in the
call's to put_user(), it calls might_fault(), which has a __LINE__.

But even adding a comment to the base file has surprisingly wide
effects. It caused the __bug_table section table to get code generated
with different instructions, not just line numbers constants changing.

So I think there should be no functional change, but the binaries are
not identical.

Re: [PATCH 1/3] x86: Separate out x86_regset for 32 and 64 bit

[Edgecombe, Rick P]

On Wed, 2022-03-16 at 12:06 -0700, Edgecombe, Richard P wrote:
> But even adding a comment to the base file has surprisingly wide
> effects. It caused the __bug_table section table to get code
> generated
> with different instructions, not just line numbers constants
> changing.

Err... I suppose this is probably because they are not actually
instructions and so should be expected, duh.

Re: [PATCH 1/3] x86: Separate out x86_regset for 32 and 64 bit

[Kees Cook]

On Wed, Mar 16, 2022 at 07:06:48PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2022-03-15 at 19:48 -0700, Kees Cook wrote:
> > On Tue, Mar 15, 2022 at 09:53:13PM +0000, Edgecombe, Rick P wrote:
> > > On Tue, 2022-03-15 at 13:41 -0700, Kees Cook wrote:
> > > > Have you verified there's no binary difference in machine code
> > > > output?
> > > 
> > > There actually was a different in the binaries. I investigated a
> > > bit,
> > > and it seemed at least part of it was due to the line numbers
> > > changing
> > > the WARN_ON()s. But otherwise, I assumed some compiler optimization
> > > must have been bumped.
> > 
> > Right, you can ignore all the debugging line number changes.
> > "diffoscope" should help see the difference by section. As long as
> > the
> > actual object code isn't changing, you should be good.
> 
> What I did originally was objdump -D ptrace.o and diff that. Then I
> slowly reduced changes to see what was generating the difference. When
> I maintained the line numbers from the original version, and simply
> converted the enum to defines, it still generated slightly different
> code in places that didn't seem to connected to the changes. So I
> figured the compiler was doing something, and relied on checking that
> the actual constants didn't change in value.
> 
> This morning I tried again to figure out what was causing the
> difference. If I strip debug symbols, remove the BUILD_BUG_ON()s and
> reformat the enums such that the line numbers are the same below the
> enums then the objdump output is identical.
> 
> I think what is happening in this debug stripped test, is that in the
> call's to put_user(), it calls might_fault(), which has a __LINE__.
> 
> But even adding a comment to the base file has surprisingly wide
> effects. It caused the __bug_table section table to get code generated
> with different instructions, not just line numbers constants changing.
> 
> So I think there should be no functional change, but the binaries are
> not identical.

Right, that's fine: the instructions should be the same, just with
various different offsets.

-- 
Kees Cook