[PATCH RESEND -tip v2 1/7] x86_32/segment: Always return correctly zero-extended values from savesegment_*()

[PATCH RESEND -tip v2 1/7] x86_32/segment: Always return correctly zero-extended values from savesegment_*()

[Uros Bizjak]

On 32-bit x86, reading segment registers into a 32-bit type can expose
undefined upper bits on older processors (e.g. Intel Quark X1000,
Pentium, and earlier), where bits 31:16 are not defined.

Introduce __seg_return_t as an intermediate type for __savesegment_*():
u16 on CONFIG_X86_32 and unsigned long otherwise.

As a result, __savesegment_*() now returns correctly zero-extended
value in all cases, avoiding propagation of undefined high bits on
32-bit systems while preserving existing behavior on 64-bit.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/include/asm/segment.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
index dbd90fede5e7..070ed87063e0 100644
--- a/arch/x86/include/asm/segment.h
+++ b/arch/x86/include/asm/segment.h
@@ -341,13 +341,23 @@ static inline void __loadsegment_fs(u16 value)
 
 #define loadsegment(seg, val) __loadsegment_##seg(val)
 
+#ifdef CONFIG_X86_32
+/*
+ * Bits 31:16 are undefined for Intel Quark X1000 processors,
+ * Pentium, and earlier processors.
+ */
+typedef u16 __seg_return_t;
+#else
+typedef unsigned long __seg_return_t;
+#endif
+
 /*
  * Save a segment register away:
  */
 #define SAVE_SEGMENT(seg)				\
 static inline unsigned long __savesegment_##seg(void)	\
 {							\
-	unsigned long v;				\
+	__seg_return_t v;				\
 	asm volatile("movl %%" #seg ",%k0" : "=r" (v));	\
 	return v;					\
 }
-- 
2.53.0

[PATCH RESEND -tip v2 2/7] x86/insn-eval: Fix signedness bug in segment selector handling

[Uros Bizjak]

get_segment_selector() returns a short, while callers use the pattern:

	short sel = get_segment_selector(...);
	if (sel < 0)
		return -ERR;

Segment selectors are 16-bit values, but storing them in a signed
16-bit type means values with the MSB set (>= 0x8000) become negative.
This causes valid selectors to be misinterpreted as errors by the
'sel < 0' check.

Change get_segment_selector() to return int and update all call sites
to use 'int sel' to avoid unintended sign extension and keep error
handling via negative return values correct.

Additionally, remove the explicit & 0xffff masking when reading
segment registers. The compiler already zero-extends
unsigned 16-bit values when loading them into a
wider type, so the masking is redundant.

With this change, valid segment selectors are no longer
confused with error returns.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/lib/insn-eval.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index e03eeec55cfe..b8847ce0b282 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -340,18 +340,18 @@ static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff)
  *
  * -EINVAL on error.
  */
-static short get_segment_selector(struct pt_regs *regs, int seg_reg_idx)
+static int get_segment_selector(struct pt_regs *regs, int seg_reg_idx)
 {
-	unsigned short sel;
+	unsigned int sel;
 
 #ifdef CONFIG_X86_64
 	switch (seg_reg_idx) {
 	case INAT_SEG_REG_IGNORE:
 		return 0;
 	case INAT_SEG_REG_CS:
-		return (unsigned short)(regs->cs & 0xffff);
+		return regs->cs;
 	case INAT_SEG_REG_SS:
-		return (unsigned short)(regs->ss & 0xffff);
+		return regs->ss;
 	case INAT_SEG_REG_DS:
 		savesegment(ds, sel);
 		return sel;
@@ -373,9 +373,9 @@ static short get_segment_selector(struct pt_regs *regs, int seg_reg_idx)
 	if (v8086_mode(regs)) {
 		switch (seg_reg_idx) {
 		case INAT_SEG_REG_CS:
-			return (unsigned short)(regs->cs & 0xffff);
+			return regs->cs;
 		case INAT_SEG_REG_SS:
-			return (unsigned short)(regs->ss & 0xffff);
+			return regs->ss;
 		case INAT_SEG_REG_DS:
 			return vm86regs->ds;
 		case INAT_SEG_REG_ES:
@@ -392,15 +392,15 @@ static short get_segment_selector(struct pt_regs *regs, int seg_reg_idx)
 
 	switch (seg_reg_idx) {
 	case INAT_SEG_REG_CS:
-		return (unsigned short)(regs->cs & 0xffff);
+		return regs->cs;
 	case INAT_SEG_REG_SS:
-		return (unsigned short)(regs->ss & 0xffff);
+		return regs->ss;
 	case INAT_SEG_REG_DS:
-		return (unsigned short)(regs->ds & 0xffff);
+		return regs->ds;
 	case INAT_SEG_REG_ES:
-		return (unsigned short)(regs->es & 0xffff);
+		return regs->es;
 	case INAT_SEG_REG_FS:
-		return (unsigned short)(regs->fs & 0xffff);
+		return regs->fs;
 	case INAT_SEG_REG_GS:
 		savesegment(gs, sel);
 		return sel;
@@ -688,7 +688,7 @@ static bool get_desc(struct desc_struct *out, unsigned short sel)
 unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
 {
 	struct desc_struct desc;
-	short sel;
+	int sel;
 
 	sel = get_segment_selector(regs, seg_reg_idx);
 	if (sel < 0)
@@ -756,7 +756,7 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx)
 {
 	struct desc_struct desc;
 	unsigned long limit;
-	short sel;
+	int sel;
 
 	sel = get_segment_selector(regs, seg_reg_idx);
 	if (sel < 0)
@@ -803,7 +803,7 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx)
 int insn_get_code_seg_params(struct pt_regs *regs)
 {
 	struct desc_struct desc;
-	short sel;
+	int sel;
 
 	if (v8086_mode(regs))
 		/* Address and operand size are both 16-bit. */
-- 
2.53.0

[PATCH RESEND -tip v2 3/7] x86/ptrace: Use savesegment() in get_segment_reg() instead of inline asm

[Uros Bizjak]

Replace direct 'movl' instructions for DS, ES, FS, and GS read in
get_segment_reg() with the savesegment() helper. This improves
readability, consistency, and ensures proper handling of
segment registers on x86_64.

No functional change intended.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/kernel/ptrace.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 3dcadc13f09a..4cb00aa0645f 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -251,32 +251,31 @@ static u16 get_segment_reg(struct task_struct *task, unsigned long offset)
 	/*
 	 * Returning the value truncates it to 16 bits.
 	 */
-	unsigned int seg;
+	unsigned int retval;
 
 	switch (offset) {
 	case offsetof(struct user_regs_struct, fs):
 		if (task == current) {
-			/* Older gas can't assemble movq %?s,%r?? */
-			asm("movl %%fs,%0" : "=r" (seg));
-			return seg;
+			savesegment(fs, retval);
+			return retval;
 		}
 		return task->thread.fsindex;
 	case offsetof(struct user_regs_struct, gs):
 		if (task == current) {
-			asm("movl %%gs,%0" : "=r" (seg));
-			return seg;
+			savesegment(gs, retval);
+			return retval;
 		}
 		return task->thread.gsindex;
 	case offsetof(struct user_regs_struct, ds):
 		if (task == current) {
-			asm("movl %%ds,%0" : "=r" (seg));
-			return seg;
+			savesegment(ds, retval);
+			return retval;
 		}
 		return task->thread.ds;
 	case offsetof(struct user_regs_struct, es):
 		if (task == current) {
-			asm("movl %%es,%0" : "=r" (seg));
-			return seg;
+			savesegment(es, retval);
+			return retval;
 		}
 		return task->thread.es;
 
-- 
2.53.0

[PATCH RESEND -tip v2 4/7] x86/ptrace: Use unsigned short for segment values in get_segment_reg()

[Uros Bizjak]

Change the temporary variable in get_segment_reg() from unsigned int
to unsigned short to match the function’s return type, making the
16-bit semantics explicit instead of relying on implicit truncation.

Also restructure the GS handling for x86_32 to make the special case
explicit and easier to follow.

This clarifies the control flow and makes the 16-bit semantics
explicit without changing behavior.

No functional change intended.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
v2: New patch.
---
 arch/x86/kernel/ptrace.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 4cb00aa0645f..58e09a764bcf 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -182,19 +182,16 @@ static unsigned long *pt_regs_access(struct pt_regs *regs, unsigned long regno)
 
 static u16 get_segment_reg(struct task_struct *task, unsigned long offset)
 {
-	/*
-	 * Returning the value truncates it to 16 bits.
-	 */
-	unsigned int retval;
-	if (offset != offsetof(struct user_regs_struct, gs))
-		retval = *pt_regs_access(task_pt_regs(task), offset);
-	else {
-		if (task == current)
+	unsigned short retval;
+
+	if (offset == offsetof(struct user_regs_struct, gs)) {
+		if (task == current) {
 			savesegment(gs, retval);
-		else
-			retval = task->thread.gs;
+			return retval;
+		}
+		return task->thread.gs;
 	}
-	return retval;
+	return *pt_regs_access(task_pt_regs(task), offset);
 }
 
 static int set_segment_reg(struct task_struct *task,
@@ -248,10 +245,7 @@ static unsigned long *pt_regs_access(struct pt_regs *regs, unsigned long offset)
 
 static u16 get_segment_reg(struct task_struct *task, unsigned long offset)
 {
-	/*
-	 * Returning the value truncates it to 16 bits.
-	 */
-	unsigned int retval;
+	unsigned short retval;
 
 	switch (offset) {
 	case offsetof(struct user_regs_struct, fs):
-- 
2.53.0

[PATCH RESEND -tip v2 5/7] x86/kexec: store segment registers directly to memory in crash_setup_regs()

[Uros Bizjak]

crash_setup_regs() currently moves segment registers (ss, cs, ds, es)
into pt_regs via a general purpose register (%eax). Update the code to
avoid the intermediate register and store segment registers directly
to their destination fields. This reduces the generated code from:

 131:	8c d0                	mov    %ss,%eax
 133:	66 89 84 24 a0 00 00 	mov    %ax,0xa0(%rsp)
 13a:	00
 13b:	8c c8                	mov    %cs,%eax
 13d:	66 89 84 24 88 00 00 	mov    %ax,0x88(%rsp)
 144:	00

to:

 131:	8c 94 24 a0 00 00 00 	mov    %ss,0xa0(%rsp)
 138:	8c 8c 24 88 00 00 00 	mov    %cs,0x88(%rsp)

No functional change intended.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/include/asm/kexec.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
index 5cfb27f26583..ad7ef567452b 100644
--- a/arch/x86/include/asm/kexec.h
+++ b/arch/x86/include/asm/kexec.h
@@ -106,11 +106,11 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
 		asm volatile("mov %%r14,%0" : "=m"(newregs->r14));
 		asm volatile("mov %%r15,%0" : "=m"(newregs->r15));
 #endif
-		asm volatile("mov %%ss,%k0" : "=a"(newregs->ss));
-		asm volatile("mov %%cs,%k0" : "=a"(newregs->cs));
+		asm volatile("mov %%ss,%0" : "=m"(newregs->ss));
+		asm volatile("mov %%cs,%0" : "=m"(newregs->cs));
 #ifdef CONFIG_X86_32
-		asm volatile("mov %%ds,%k0" : "=a"(newregs->ds));
-		asm volatile("mov %%es,%k0" : "=a"(newregs->es));
+		asm volatile("mov %%ds,%0" : "=m"(newregs->ds));
+		asm volatile("mov %%es,%0" : "=m"(newregs->es));
 #endif
 		asm volatile("pushf\n\t"
 			     "pop %0" : "=m"(newregs->flags));
-- 
2.53.0

[PATCH RESEND -tip v2 6/7] x86/segment: Introduce savesegment_mem16() helper to write segment selectors to memory

[Uros Bizjak]

Introduce a new helper, savesegment_mem16(), that stores a segment
selector directly into a u16 (or compatible) memory location without
using an intermediate general-purpose register.

To support this, split the existing SAVE_SEGMENT macro into two parts:

SAVE_SEGMENT_VAR(): retains the current behavior of reading a segment
register into an unsigned long via a register.
SAVE_SEGMENT_PTR(): adds a new variant that writes the 16-bit selector
directly to memory.

The combined SAVE_SEGMENT() macro now generates both helpers for each
segment register.

The new savesegment_mem16() interface is preferred over savesegment()
when the value only needs to be stored (e.g. into a struct field),
avoiding an unnecessary register move and making the intent clearer.

No functional change for existing users of savesegment().

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
---
v2: Rename storesegment() to savesegment_mem16() (Ingo).
---
 arch/x86/include/asm/segment.h | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
index 070ed87063e0..51a1fcdc7215 100644
--- a/arch/x86/include/asm/segment.h
+++ b/arch/x86/include/asm/segment.h
@@ -354,7 +354,7 @@ typedef unsigned long __seg_return_t;
 /*
  * Save a segment register away:
  */
-#define SAVE_SEGMENT(seg)				\
+#define SAVE_SEGMENT_VAR(seg)				\
 static inline unsigned long __savesegment_##seg(void)	\
 {							\
 	__seg_return_t v;				\
@@ -362,6 +362,16 @@ static inline unsigned long __savesegment_##seg(void)	\
 	return v;					\
 }
 
+#define SAVE_SEGMENT_PTR(seg)				\
+static inline void __savesegment_##seg##_ptr(u16 *p)	\
+{							\
+	asm volatile("movw %%" #seg ",%0" : "=m" (*p));	\
+}
+
+#define SAVE_SEGMENT(seg)				\
+	SAVE_SEGMENT_VAR(seg)				\
+	SAVE_SEGMENT_PTR(seg)
+
 SAVE_SEGMENT(cs)
 SAVE_SEGMENT(ss)
 SAVE_SEGMENT(ds)
@@ -369,10 +379,29 @@ SAVE_SEGMENT(es)
 SAVE_SEGMENT(fs)
 SAVE_SEGMENT(gs)
 
+#undef SAVE_SEGMENT_VAR
+#undef SAVE_SEGMENT_PTR
 #undef SAVE_SEGMENT
 
+/*
+ * savesegment(seg, var) - Read a segment register into an unsigned long.
+ *
+ * Reads the segment selector via a general-purpose register into an
+ * unsigned long. Preferred when the value is needed in a register for
+ * subsequent arithmetic or comparison.
+ */
 #define savesegment(seg, var) ((var) = __savesegment_##seg())
 
+/*
+ * savesegment_mem16(seg, loc) - Store a segment register directly
+ *                               to u16 (or compatible) location.
+ *
+ * Writes the 16-bit segment selector directly to memory, bypassing any
+ * intermediate general-purpose register. Preferred over savesegment()
+ * for simply saving a segment register to a u16 (or compatible) location.
+ */
+#define savesegment_mem16(seg, loc) __savesegment_##seg##_ptr(&(loc))
+
 #endif /* !__ASSEMBLER__ */
 #endif /* __KERNEL__ */
 
-- 
2.53.0

[PATCH RESEND -tip v2 7/7] x86/process: Use savesegment_mem16() when saving segment selectors

[Uros Bizjak]

Switch segment save sites in copy_thread() and save_fsgs() from
savesegment() to the newly introduced savesegment_mem16() helper.

These call sites only store the segment selector into a u16 field
and do not require the value in a general-purpose register. Using
savesegment_mem16() avoids the unnecessary register intermediate
and better matches the intended use.

The generated code is improved from, e.g.:

     34f:	8c c0                	mov    %es,%eax
     351:	66 89 83 08 0c 00 00 	mov    %ax,0xc08(%rbx)

to a single direct store:

     34f:	8c 83 08 0c 00 00    	mov    %es,0xc08(%rbx)

This is a mechanical follow-up to the introduction of
savesegment_mem16(), with no functional change.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/kernel/process.c    | 4 ++--
 arch/x86/kernel/process_64.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 4c718f8adc59..f7a38345980c 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -197,8 +197,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
 	p->thread.gsindex = current->thread.gsindex;
 	p->thread.gsbase = current->thread.gsbase;
 
-	savesegment(es, p->thread.es);
-	savesegment(ds, p->thread.ds);
+	savesegment_mem16(es, p->thread.es);
+	savesegment_mem16(ds, p->thread.ds);
 
 	if (p->mm && (clone_flags & (CLONE_VM | CLONE_VFORK)) == CLONE_VM)
 		set_bit(MM_CONTEXT_LOCK_LAM, &p->mm->context.flags);
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index b85e715ebb30..619b46f9bcba 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -275,8 +275,8 @@ static __always_inline void save_base_legacy(struct task_struct *prev_p,
 
 static __always_inline void save_fsgs(struct task_struct *task)
 {
-	savesegment(fs, task->thread.fsindex);
-	savesegment(gs, task->thread.gsindex);
+	savesegment_mem16(fs, task->thread.fsindex);
+	savesegment_mem16(gs, task->thread.gsindex);
 	if (static_cpu_has(X86_FEATURE_FSGSBASE)) {
 		/*
 		 * If FSGSBASE is enabled, we can't make any useful guesses
-- 
2.53.0