This macro is small, has only four callsites, and one of them is
slightly different using a conditional parameter.
A few saved lines aren't worth the resulting obfuscation.
Generated machine code is identical.
Signed-off-by: Denys Vlasenko <[email protected]>
CC: Linus Torvalds <[email protected]>
CC: Steven Rostedt <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Borislav Petkov <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: Andy Lutomirski <[email protected]>
CC: Oleg Nesterov <[email protected]>
CC: Frederic Weisbecker <[email protected]>
CC: Alexei Starovoitov <[email protected]>
CC: Will Drewry <[email protected]>
CC: Kees Cook <[email protected]>
CC: [email protected]
CC: [email protected]
---
These two patches are on top of "Simplify zeroing of pt_regs->r8..r11" patch.
arch/x86/ia32/ia32entry.S | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index 2801cbe..86cbfe6 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -29,15 +29,6 @@
.section .entry.text, "ax"
- /* clobbers %rax */
- .macro CLEAR_RREGS _r9=rax
- xorl %eax,%eax
- movq %rax,R11(%rsp)
- movq %rax,R10(%rsp)
- movq %\_r9,R9(%rsp)
- movq %rax,R8(%rsp)
- .endm
-
/*
* Reload arg registers from stack in case ptrace changed them.
* We don't reload %eax because syscall_trace_enter() returned
@@ -243,7 +234,11 @@ sysexit_from_sys_call:
TRACE_IRQS_OFF
testl %edi, ASM_THREAD_INFO(TI_flags, %rsp, SIZEOF_PTREGS)
jz \exit
- CLEAR_RREGS
+ xorl %eax, %eax
+ movq %rax, R11(%rsp)
+ movq %rax, R10(%rsp)
+ movq %rax, R9(%rsp)
+ movq %rax, R8(%rsp)
jmp int_with_check
.endm
@@ -267,7 +262,11 @@ sysenter_tracesys:
jz sysenter_auditsys
#endif
SAVE_EXTRA_REGS
- CLEAR_RREGS
+ xorl %eax, %eax
+ movq %rax, R11(%rsp)
+ movq %rax, R10(%rsp)
+ movq %rax, R9(%rsp)
+ movq %rax, R8(%rsp)
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
LOAD_ARGS32 /* reload args from stack in case ptrace changed it */
@@ -407,7 +406,11 @@ cstar_tracesys:
#endif
xchgl %r9d,%ebp
SAVE_EXTRA_REGS
- CLEAR_RREGS r9
+ xorl %eax, %eax
+ movq %rax, R11(%rsp)
+ movq %rax, R10(%rsp)
+ movq %r9, R9(%rsp)
+ movq %rax, R8(%rsp)
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
LOAD_ARGS32 1 /* reload args from stack in case ptrace changed it */
@@ -422,7 +425,11 @@ ia32_badarg:
jmp ia32_sysret
ia32_ret_from_sys_call:
- CLEAR_RREGS
+ xorl %eax, %eax
+ movq %rax, R11(%rsp)
+ movq %rax, R10(%rsp)
+ movq %rax, R9(%rsp)
+ movq %rax, R8(%rsp)
jmp int_ret_from_sys_call
/*
--
1.8.1.4
This macro is small, has only three callsites, and one of them is
slightly different using a conditional parameter.
A few saved lines aren't worth the resulting obfuscation.
Generated machine code is identical.
Signed-off-by: Denys Vlasenko <[email protected]>
CC: Linus Torvalds <[email protected]>
CC: Steven Rostedt <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Borislav Petkov <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: Andy Lutomirski <[email protected]>
CC: Oleg Nesterov <[email protected]>
CC: Frederic Weisbecker <[email protected]>
CC: Alexei Starovoitov <[email protected]>
CC: Will Drewry <[email protected]>
CC: Kees Cook <[email protected]>
CC: [email protected]
CC: [email protected]
---
arch/x86/ia32/ia32entry.S | 62 ++++++++++++++++++++++++++++-------------------
1 file changed, 37 insertions(+), 25 deletions(-)
diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index 86cbfe6..0ff676a 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -29,28 +29,6 @@
.section .entry.text, "ax"
- /*
- * Reload arg registers from stack in case ptrace changed them.
- * We don't reload %eax because syscall_trace_enter() returned
- * the %rax value we should see. Instead, we just truncate that
- * value to 32 bits again as we did on entry from user mode.
- * If it's a new value set by user_regset during entry tracing,
- * this matches the normal truncation of the user-mode value.
- * If it's -1 to make us punt the syscall, then (u32)-1 is still
- * an appropriately invalid value.
- */
- .macro LOAD_ARGS32 _r9=0
- .if \_r9
- movl R9(%rsp),%r9d
- .endif
- movl RCX(%rsp),%ecx
- movl RDX(%rsp),%edx
- movl RSI(%rsp),%esi
- movl RDI(%rsp),%edi
- movl %eax,%eax /* zero extension */
- .endm
-
-
#ifdef CONFIG_PARAVIRT
ENTRY(native_usergs_sysret32)
swapgs
@@ -269,7 +247,18 @@ sysenter_tracesys:
movq %rax, R8(%rsp)
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
- LOAD_ARGS32 /* reload args from stack in case ptrace changed it */
+ /*
+ * Reload arg registers from stack in case ptrace changed them.
+ * Don't reload %eax because syscall_trace_enter() returned
+ * the %rax value we should see. But do truncate it to 32 bits.
+ * If it's -1 to make us punt the syscall, then (u32)-1 is still
+ * an appropriately invalid value.
+ */
+ movl RCX(%rsp), %ecx
+ movl RDX(%rsp), %edx
+ movl RSI(%rsp), %esi
+ movl RDI(%rsp), %edi
+ movl %eax, %eax /* zero extension */
RESTORE_EXTRA_REGS
jmp sysenter_do_call
ENDPROC(ia32_sysenter_target)
@@ -413,7 +402,19 @@ cstar_tracesys:
movq %rax, R8(%rsp)
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
- LOAD_ARGS32 1 /* reload args from stack in case ptrace changed it */
+ movl R9(%rsp),%r9d
+ /*
+ * Reload arg registers from stack in case ptrace changed them.
+ * Don't reload %eax because syscall_trace_enter() returned
+ * the %rax value we should see. But do truncate it to 32 bits.
+ * If it's -1 to make us punt the syscall, then (u32)-1 is still
+ * an appropriately invalid value.
+ */
+ movl RCX(%rsp), %ecx
+ movl RDX(%rsp), %edx
+ movl RSI(%rsp), %esi
+ movl RDI(%rsp), %edi
+ movl %eax, %eax /* zero extension */
RESTORE_EXTRA_REGS
xchgl %ebp,%r9d
jmp cstar_do_call
@@ -502,7 +503,18 @@ ia32_tracesys:
SAVE_EXTRA_REGS
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
- LOAD_ARGS32 /* reload args from stack in case ptrace changed it */
+ /*
+ * Reload arg registers from stack in case ptrace changed them.
+ * Don't reload %eax because syscall_trace_enter() returned
+ * the %rax value we should see. But do truncate it to 32 bits.
+ * If it's -1 to make us punt the syscall, then (u32)-1 is still
+ * an appropriately invalid value.
+ */
+ movl RCX(%rsp), %ecx
+ movl RDX(%rsp), %edx
+ movl RSI(%rsp), %esi
+ movl RDI(%rsp), %edi
+ movl %eax, %eax /* zero extension */
RESTORE_EXTRA_REGS
jmp ia32_do_call
END(ia32_syscall)
--
1.8.1.4
On Tue, Jun 02, 2015 at 09:04:02PM +0200, Denys Vlasenko wrote:
> This macro is small, has only three callsites, and one of them is
> slightly different using a conditional parameter.
>
> A few saved lines aren't worth the resulting obfuscation.
>
> Generated machine code is identical.
>
> Signed-off-by: Denys Vlasenko <[email protected]>
> CC: Linus Torvalds <[email protected]>
> CC: Steven Rostedt <[email protected]>
> CC: Ingo Molnar <[email protected]>
> CC: Borislav Petkov <[email protected]>
> CC: "H. Peter Anvin" <[email protected]>
> CC: Andy Lutomirski <[email protected]>
> CC: Oleg Nesterov <[email protected]>
> CC: Frederic Weisbecker <[email protected]>
> CC: Alexei Starovoitov <[email protected]>
> CC: Will Drewry <[email protected]>
> CC: Kees Cook <[email protected]>
> CC: [email protected]
> CC: [email protected]
> ---
> arch/x86/ia32/ia32entry.S | 62 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
> index 86cbfe6..0ff676a 100644
> --- a/arch/x86/ia32/ia32entry.S
> +++ b/arch/x86/ia32/ia32entry.S
> @@ -29,28 +29,6 @@
>
> .section .entry.text, "ax"
>
> - /*
> - * Reload arg registers from stack in case ptrace changed them.
> - * We don't reload %eax because syscall_trace_enter() returned
> - * the %rax value we should see. Instead, we just truncate that
> - * value to 32 bits again as we did on entry from user mode.
> - * If it's a new value set by user_regset during entry tracing,
> - * this matches the normal truncation of the user-mode value.
> - * If it's -1 to make us punt the syscall, then (u32)-1 is still
> - * an appropriately invalid value.
Can't say that I'm crazy about the replication of (a shorter version of)
that comment three times though.
The macro is in the same file so looking it up is trivial. So what's the
point?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Tue, 2 Jun 2015 21:04:01 +0200
Denys Vlasenko <[email protected]> wrote:
> This macro is small, has only four callsites, and one of them is
> slightly different using a conditional parameter.
>
> A few saved lines aren't worth the resulting obfuscation.
I'm curious, why? Did someone recommend this change? I don't see it as
obfuscation at all.
-- Steve
>
> Generated machine code is identical.
On 06/02/2015 09:34 PM, Steven Rostedt wrote:
> On Tue, 2 Jun 2015 21:04:01 +0200
> Denys Vlasenko <[email protected]> wrote:
>
>> This macro is small, has only four callsites, and one of them is
>> slightly different using a conditional parameter.
>>
>> A few saved lines aren't worth the resulting obfuscation.
>
> I'm curious, why? Did someone recommend this change?
I'm proposing to do this. Of course, I don't expect that
any my patch must be accepted.
> I don't see it as obfuscation at all.
Riddle me this, looking at the current code. What's up with that
strange manipulations of %r9 register? Why the code in
SYSCALL and SYSENTER entry points is not the same,
why "r9 dance" is done only in one of these entry points?
* Steven Rostedt <[email protected]> wrote:
> On Tue, 2 Jun 2015 21:04:01 +0200
> Denys Vlasenko <[email protected]> wrote:
>
> > This macro is small, has only four callsites, and one of them is slightly
> > different using a conditional parameter.
> >
> > A few saved lines aren't worth the resulting obfuscation.
>
> I'm curious, why? Did someone recommend this change? I don't see it as
> obfuscation at all.
So here are a few easy questions, I'm wondering how many minutes it takes for you
to answer them correctly:
- What does the CLEAR_RREGS name stand for?
- What is this macro's purpose?
- In a single case CLEAR_RREGS takes a 'r9' argument:
arch/x86/ia32/ia32entry.S: CLEAR_RREGS
arch/x86/ia32/ia32entry.S: CLEAR_RREGS
arch/x86/ia32/ia32entry.S: CLEAR_RREGS r9
arch/x86/ia32/ia32entry.S: CLEAR_RREGS
arch/x86/ia32/ia32entry.S: CLEAR_RREGS
What is the 'r9' argument's purpose and why is activated in the place where
it's activated?
The CLEAR_RREGS macro has zero comments. If it takes more than a quick glance to
determine all these three first-order questions from the source code, then it's an
obvious code cleanliness fail which needs to be improved.
Thanks,
Ingo
Commit-ID: ef0cd5dc25404594f832dad9133abae52e3b2fa3
Gitweb: http://git.kernel.org/tip/ef0cd5dc25404594f832dad9133abae52e3b2fa3
Author: Denys Vlasenko <[email protected]>
AuthorDate: Tue, 2 Jun 2015 21:04:01 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 5 Jun 2015 13:22:22 +0200
x86/asm/entry/32: Open-code CLEAR_RREGS
This macro is small, has only four callsites, and one of them is
slightly different using a conditional parameter.
A few saved lines aren't worth the resulting obfuscation.
Generated machine code is identical.
Signed-off-by: Denys Vlasenko <[email protected]>
[ Added comments. ]
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Drewry <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/entry/ia32entry.S | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/arch/x86/entry/ia32entry.S b/arch/x86/entry/ia32entry.S
index f00a409..8a45d2c 100644
--- a/arch/x86/entry/ia32entry.S
+++ b/arch/x86/entry/ia32entry.S
@@ -29,15 +29,6 @@
.section .entry.text, "ax"
- /* clobbers %rax */
- .macro CLEAR_RREGS _r9=rax
- xorl %eax,%eax
- movq %rax,R11(%rsp)
- movq %rax,R10(%rsp)
- movq %\_r9,R9(%rsp)
- movq %rax,R8(%rsp)
- .endm
-
/*
* Reload arg registers from stack in case ptrace changed them.
* We don't reload %eax because syscall_trace_enter() returned
@@ -243,7 +234,11 @@ sysexit_from_sys_call:
TRACE_IRQS_OFF
testl %edi, ASM_THREAD_INFO(TI_flags, %rsp, SIZEOF_PTREGS)
jz \exit
- CLEAR_RREGS
+ xorl %eax, %eax /* do not leak kernel information */
+ movq %rax, R11(%rsp)
+ movq %rax, R10(%rsp)
+ movq %rax, R9(%rsp)
+ movq %rax, R8(%rsp)
jmp int_with_check
.endm
@@ -267,7 +262,11 @@ sysenter_tracesys:
jz sysenter_auditsys
#endif
SAVE_EXTRA_REGS
- CLEAR_RREGS
+ xorl %eax, %eax /* do not leak kernel information */
+ movq %rax, R11(%rsp)
+ movq %rax, R10(%rsp)
+ movq %rax, R9(%rsp)
+ movq %rax, R8(%rsp)
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
LOAD_ARGS32 /* reload args from stack in case ptrace changed it */
@@ -407,7 +406,11 @@ cstar_tracesys:
#endif
xchgl %r9d,%ebp
SAVE_EXTRA_REGS
- CLEAR_RREGS r9
+ xorl %eax, %eax /* do not leak kernel information */
+ movq %rax, R11(%rsp)
+ movq %rax, R10(%rsp)
+ movq %r9, R9(%rsp)
+ movq %rax, R8(%rsp)
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
LOAD_ARGS32 1 /* reload args from stack in case ptrace changed it */
@@ -422,7 +425,11 @@ ia32_badarg:
jmp ia32_sysret
ia32_ret_from_sys_call:
- CLEAR_RREGS
+ xorl %eax, %eax /* do not leak kernel information */
+ movq %rax, R11(%rsp)
+ movq %rax, R10(%rsp)
+ movq %rax, R9(%rsp)
+ movq %rax, R8(%rsp)
jmp int_ret_from_sys_call
/*
Commit-ID: 73cbf687914fd5f4ef88a42a55784fd28b7450cf
Gitweb: http://git.kernel.org/tip/73cbf687914fd5f4ef88a42a55784fd28b7450cf
Author: Denys Vlasenko <[email protected]>
AuthorDate: Tue, 2 Jun 2015 21:04:02 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 5 Jun 2015 13:22:22 +0200
x86/asm/entry/32: Open-code LOAD_ARGS32
This macro is small, has only three callsites, and one of them
is slightly different using a conditional parameter.
A few saved lines aren't worth the resulting obfuscation.
Generated machine code is identical.
Signed-off-by: Denys Vlasenko <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Drewry <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/entry/ia32entry.S | 54 +++++++++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/arch/x86/entry/ia32entry.S b/arch/x86/entry/ia32entry.S
index 8a45d2c..56f819e 100644
--- a/arch/x86/entry/ia32entry.S
+++ b/arch/x86/entry/ia32entry.S
@@ -29,28 +29,6 @@
.section .entry.text, "ax"
- /*
- * Reload arg registers from stack in case ptrace changed them.
- * We don't reload %eax because syscall_trace_enter() returned
- * the %rax value we should see. Instead, we just truncate that
- * value to 32 bits again as we did on entry from user mode.
- * If it's a new value set by user_regset during entry tracing,
- * this matches the normal truncation of the user-mode value.
- * If it's -1 to make us punt the syscall, then (u32)-1 is still
- * an appropriately invalid value.
- */
- .macro LOAD_ARGS32 _r9=0
- .if \_r9
- movl R9(%rsp),%r9d
- .endif
- movl RCX(%rsp),%ecx
- movl RDX(%rsp),%edx
- movl RSI(%rsp),%esi
- movl RDI(%rsp),%edi
- movl %eax,%eax /* zero extension */
- .endm
-
-
#ifdef CONFIG_PARAVIRT
ENTRY(native_usergs_sysret32)
swapgs
@@ -269,7 +247,14 @@ sysenter_tracesys:
movq %rax, R8(%rsp)
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
- LOAD_ARGS32 /* reload args from stack in case ptrace changed it */
+
+ /* Reload arg registers from stack. (see sysenter_tracesys) */
+ movl RCX(%rsp), %ecx
+ movl RDX(%rsp), %edx
+ movl RSI(%rsp), %esi
+ movl RDI(%rsp), %edi
+ movl %eax, %eax /* zero extension */
+
RESTORE_EXTRA_REGS
jmp sysenter_do_call
ENDPROC(ia32_sysenter_target)
@@ -413,7 +398,15 @@ cstar_tracesys:
movq %rax, R8(%rsp)
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
- LOAD_ARGS32 1 /* reload args from stack in case ptrace changed it */
+ movl R9(%rsp),%r9d
+
+ /* Reload arg registers from stack. (see sysenter_tracesys) */
+ movl RCX(%rsp), %ecx
+ movl RDX(%rsp), %edx
+ movl RSI(%rsp), %esi
+ movl RDI(%rsp), %edi
+ movl %eax, %eax /* zero extension */
+
RESTORE_EXTRA_REGS
xchgl %ebp,%r9d
jmp cstar_do_call
@@ -502,7 +495,18 @@ ia32_tracesys:
SAVE_EXTRA_REGS
movq %rsp,%rdi /* &pt_regs -> arg1 */
call syscall_trace_enter
- LOAD_ARGS32 /* reload args from stack in case ptrace changed it */
+ /*
+ * Reload arg registers from stack in case ptrace changed them.
+ * Don't reload %eax because syscall_trace_enter() returned
+ * the %rax value we should see. But do truncate it to 32 bits.
+ * If it's -1 to make us punt the syscall, then (u32)-1 is still
+ * an appropriately invalid value.
+ */
+ movl RCX(%rsp), %ecx
+ movl RDX(%rsp), %edx
+ movl RSI(%rsp), %esi
+ movl RDI(%rsp), %edi
+ movl %eax, %eax /* zero extension */
RESTORE_EXTRA_REGS
jmp ia32_do_call
END(ia32_syscall)