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

Commit Message

Edgecombe, Rick P March 17, 2022, 7:20 p.m. UTC

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. Also, change the name pattern to be like
REGSET32_FOO, instead of REGSET_FOO32, to better emphasize that the bit
size is the bitsize of the architecture, not the register itself.

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/[e-mail]/

Signed-off-by: Rick Edgecombe <[e-mail]>
Acked-by: Kees Cook <[e-mail]>

---

v2:
 - Rename REGSET_FOO32 to REGSET32_FOO (Eric Biederman)
 - Drop Kees' Reviewed-by to Acked-by, due to changing enum value names
 
 arch/x86/kernel/ptrace.c | 66 +++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 24 deletions(-)

Comments

Kees Cook March 17, 2022, 9:33 p.m. UTC | #1

On Thu, Mar 17, 2022 at 12:20:11PM -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. Also, change the name pattern to be like
> REGSET32_FOO, instead of REGSET_FOO32, to better emphasize that the bit
> size is the bitsize of the architecture, not the register itself.
> 
> 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/[e-mail]/
> 
> Signed-off-by: Rick Edgecombe <[e-mail]>
> Acked-by: Kees Cook <[e-mail]>
> 
> ---
> 
> v2:
>  - Rename REGSET_FOO32 to REGSET32_FOO (Eric Biederman)
>  - Drop Kees' Reviewed-by to Acked-by, due to changing enum value names

I think of "Ack" to mean "I am a maintainer of this area and someone can
carry this instead of it going via my tree". While I certainly poke and
ptrace and x86 a lot, I probably wouldn't Ack in this part of the
kernel. But it does seem "Reviewed-by" is a stronger signal[1].

Regardless, v2 looks good to me still. :)

Reviewed-by: Kees Cook <[e-mail]>

-Kees

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Edgecombe, Rick P March 17, 2022, 9:54 p.m. UTC | #2

On Thu, 2022-03-17 at 14:33 -0700, Kees Cook wrote:
> I think of "Ack" to mean "I am a maintainer of this area and someone
> can
> carry this instead of it going via my tree". While I certainly poke
> and
> ptrace and x86 a lot, I probably wouldn't Ack in this part of the
> kernel. But it does seem "Reviewed-by" is a stronger signal[1].

I thought I remembered reading in some kernel docs somewhere to demote
reviewed-by's to acks if anything changed, so some credit goes in for
the past work on it. Cannot find anything now, thanks for the
clarification.

Patch

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 8d2f2f995539..c6a4a4d0d804 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,
-	REGSET_IOPERM32,
+enum x86_regset_32 {
+	REGSET32_GENERAL,
+	REGSET32_FP,
+	REGSET32_XFP,
+	REGSET32_XSTATE,
+	REGSET32_TLS,
+	REGSET32_IOPERM,
 };
 
+enum x86_regset_64 {
+	REGSET64_GENERAL,
+	REGSET64_FP,
+	REGSET64_IOPERM,
+	REGSET64_XSTATE,
+};
+
+#define REGSET_GENERAL \
+({ \
+	BUILD_BUG_ON((int)REGSET32_GENERAL != (int)REGSET64_GENERAL); \
+	REGSET32_GENERAL; \
+})
+
+#define REGSET_FP \
+({ \
+	BUILD_BUG_ON((int)REGSET32_FP != (int)REGSET64_FP); \
+	REGSET32_FP; \
+})
+
 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,
+					   REGSET32_XFP,
 					   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,
+					     REGSET32_XFP,
 					     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,
+					   REGSET32_XFP, 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,
+					     REGSET32_XFP, 0,
 					     sizeof(struct user32_fxsr_struct),
 					     datap);
 
@@ -1216,25 +1234,25 @@  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] = {
+	[REGSET64_GENERAL] = {
 		.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] = {
+	[REGSET64_FP] = {
 		.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] = {
+	[REGSET64_XSTATE] = {
 		.core_note_type = NT_X86_XSTATE,
 		.size = sizeof(u64), .align = sizeof(u64),
 		.active = xstateregs_active, .regset_get = xstateregs_get,
 		.set = xstateregs_set
 	},
-	[REGSET_IOPERM64] = {
+	[REGSET64_IOPERM] = {
 		.core_note_type = NT_386_IOPERM,
 		.n = IO_BITMAP_LONGS,
 		.size = sizeof(long), .align = sizeof(long),
@@ -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] = {
+	[REGSET32_GENERAL] = {
 		.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] = {
+	[REGSET32_FP] = {
 		.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] = {
+	[REGSET32_XFP] = {
 		.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] = {
+	[REGSET32_XSTATE] = {
 		.core_note_type = NT_X86_XSTATE,
 		.size = sizeof(u64), .align = sizeof(u64),
 		.active = xstateregs_active, .regset_get = xstateregs_get,
 		.set = xstateregs_set
 	},
-	[REGSET_TLS] = {
+	[REGSET32_TLS] = {
 		.core_note_type = NT_386_TLS,
 		.n = GDT_ENTRY_TLS_ENTRIES, .bias = GDT_ENTRY_TLS_MIN,
 		.size = sizeof(struct user_desc),
@@ -1289,7 +1307,7 @@  static struct user_regset x86_32_regsets[] __ro_after_init = {
 		.active = regset_tls_active,
 		.regset_get = regset_tls_get, .set = regset_tls_set
 	},
-	[REGSET_IOPERM32] = {
+	[REGSET32_IOPERM] = {
 		.core_note_type = NT_386_IOPERM,
 		.n = IO_BITMAP_BYTES / sizeof(u32),
 		.size = sizeof(u32), .align = sizeof(u32),
@@ -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[REGSET64_XSTATE].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[REGSET32_XSTATE].n = size / sizeof(u64);
 #endif
 	xstate_fx_sw_bytes[USER_XSTATE_XCR0_WORD] = xstate_mask;
 }