* [PATCH] i386/audit: stop scribbling on the stack frame
@ 2014-10-23 4:04 Eric Paris
2014-10-23 18:39 ` Andy Lutomirski
[not found] ` <tip-26c2d2b39128adba276d140eefa2745591b88536@git.kernel.org>
0 siblings, 2 replies; 23+ messages in thread
From: Eric Paris @ 2014-10-23 4:04 UTC (permalink / raw)
To: rgb
Cc: Eric Paris, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
linux-kernel, linux-audit
git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb.
It was writing over %esp/pt_regs semi-randomly on i686 with the expected
"system can't boot" results. As noted in:
https://bugs.freedesktop.org/show_bug.cgi?id=85277
This patch stops fscking with pt_regs. Instead it sets up the registers
for the call to __audit_syscall_entry in the most obvious conceivable
way. It then does just a tiny tiny touch of magic. We need to get what
started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy
as a pair of pushes.
After the call to __audit_syscall_entry all we need to do is get that
now useless junk off the stack (pair of pops) and reload %eax with the
original syscall so other stuff can keep going about it's business.
Signed-off-by: Eric Paris <eparis@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-audit@redhat.com
---
arch/x86/kernel/entry_32.S | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index f9e3fab..fb01d22 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -447,15 +447,14 @@ sysenter_exit:
sysenter_audit:
testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
jnz syscall_trace_entry
- addl $4,%esp
- CFI_ADJUST_CFA_OFFSET -4
- movl %esi,4(%esp) /* 5th arg: 4th syscall arg */
- movl %edx,(%esp) /* 4th arg: 3rd syscall arg */
- /* %ecx already in %ecx 3rd arg: 2nd syscall arg */
- movl %ebx,%edx /* 2nd arg: 1st syscall arg */
- /* %eax already in %eax 1st arg: syscall number */
+ /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */
+ movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */
+ /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */
+ pushl_cfi PT_ESI(%esp) /* a3: 5th arg */
+ pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */
call __audit_syscall_entry
- pushl_cfi %ebx
+ popl_cfi %ecx /* get that remapped edx off the stack */
+ popl_cfi %ecx /* get that remapped esi off the stack */
movl PT_EAX(%esp),%eax /* reload syscall number */
jmp sysenter_do_call
--
1.9.3
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 4:04 [PATCH] i386/audit: stop scribbling on the stack frame Eric Paris @ 2014-10-23 18:39 ` Andy Lutomirski 2014-10-23 19:13 ` H. Peter Anvin 2014-10-23 19:15 ` Eric Paris [not found] ` <tip-26c2d2b39128adba276d140eefa2745591b88536@git.kernel.org> 1 sibling, 2 replies; 23+ messages in thread From: Andy Lutomirski @ 2014-10-23 18:39 UTC (permalink / raw) To: Eric Paris, rgb Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel, linux-audit On 10/22/2014 09:04 PM, Eric Paris wrote: > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. > It was writing over %esp/pt_regs semi-randomly on i686 with the expected > "system can't boot" results. As noted in: > > https://bugs.freedesktop.org/show_bug.cgi?id=85277 > > This patch stops fscking with pt_regs. Instead it sets up the registers > for the call to __audit_syscall_entry in the most obvious conceivable > way. It then does just a tiny tiny touch of magic. We need to get what > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy > as a pair of pushes. > > After the call to __audit_syscall_entry all we need to do is get that > now useless junk off the stack (pair of pops) and reload %eax with the > original syscall so other stuff can keep going about it's business. > > Signed-off-by: Eric Paris <eparis@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: x86@kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-audit@redhat.com > --- > arch/x86/kernel/entry_32.S | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > index f9e3fab..fb01d22 100644 > --- a/arch/x86/kernel/entry_32.S > +++ b/arch/x86/kernel/entry_32.S > @@ -447,15 +447,14 @@ sysenter_exit: > sysenter_audit: > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > jnz syscall_trace_entry > - addl $4,%esp > - CFI_ADJUST_CFA_OFFSET -4 > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > - /* %eax already in %eax 1st arg: syscall number */ > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ > call __audit_syscall_entry > - pushl_cfi %ebx > + popl_cfi %ecx /* get that remapped edx off the stack */ > + popl_cfi %ecx /* get that remapped esi off the stack */ > movl PT_EAX(%esp),%eax /* reload syscall number */ > jmp sysenter_do_call > > This looks reasonably likely to be correct, but this code is complicated and now ever slower. How hard would it be to just delete it and replace it with a straightforward two-phase trace invocation a la x86_64? --Andy ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 18:39 ` Andy Lutomirski @ 2014-10-23 19:13 ` H. Peter Anvin 2014-10-23 19:15 ` Eric Paris 1 sibling, 0 replies; 23+ messages in thread From: H. Peter Anvin @ 2014-10-23 19:13 UTC (permalink / raw) To: Andy Lutomirski, Eric Paris, rgb Cc: Thomas Gleixner, Ingo Molnar, x86, linux-kernel, linux-audit Well, probably not something for stable/urgent... On October 23, 2014 11:39:48 AM PDT, Andy Lutomirski <luto@amacapital.net> wrote: >On 10/22/2014 09:04 PM, Eric Paris wrote: >> git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very >dumb. >> It was writing over %esp/pt_regs semi-randomly on i686 with the >expected >> "system can't boot" results. As noted in: >> >> https://bugs.freedesktop.org/show_bug.cgi?id=85277 >> >> This patch stops fscking with pt_regs. Instead it sets up the >registers >> for the call to __audit_syscall_entry in the most obvious conceivable >> way. It then does just a tiny tiny touch of magic. We need to get >what >> started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as >easy >> as a pair of pushes. >> >> After the call to __audit_syscall_entry all we need to do is get that >> now useless junk off the stack (pair of pops) and reload %eax with >the >> original syscall so other stuff can keep going about it's business. >> >> Signed-off-by: Eric Paris <eparis@redhat.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: "H. Peter Anvin" <hpa@zytor.com> >> Cc: x86@kernel.org >> Cc: linux-kernel@vger.kernel.org >> Cc: linux-audit@redhat.com >> --- >> arch/x86/kernel/entry_32.S | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S >> index f9e3fab..fb01d22 100644 >> --- a/arch/x86/kernel/entry_32.S >> +++ b/arch/x86/kernel/entry_32.S >> @@ -447,15 +447,14 @@ sysenter_exit: >> sysenter_audit: >> testl $(_TIF_WORK_SYSCALL_ENTRY & >~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) >> jnz syscall_trace_entry >> - addl $4,%esp >> - CFI_ADJUST_CFA_OFFSET -4 >> - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ >> - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ >> - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ >> - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ >> - /* %eax already in %eax 1st arg: syscall number */ >> + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to >audit */ >> + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ >> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ >> + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ >> + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ >> call __audit_syscall_entry >> - pushl_cfi %ebx >> + popl_cfi %ecx /* get that remapped edx off the stack */ >> + popl_cfi %ecx /* get that remapped esi off the stack */ >> movl PT_EAX(%esp),%eax /* reload syscall number */ >> jmp sysenter_do_call >> >> > >This looks reasonably likely to be correct, but this code is >complicated >and now ever slower. > >How hard would it be to just delete it and replace it with a >straightforward two-phase trace invocation a la x86_64? > >--Andy -- Sent from my mobile phone. Please pardon brevity and lack of formatting. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 18:39 ` Andy Lutomirski 2014-10-23 19:13 ` H. Peter Anvin @ 2014-10-23 19:15 ` Eric Paris 2014-10-23 19:20 ` Andy Lutomirski 1 sibling, 1 reply; 23+ messages in thread From: Eric Paris @ 2014-10-23 19:15 UTC (permalink / raw) To: Andy Lutomirski Cc: rgb, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel, linux-audit On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote: > On 10/22/2014 09:04 PM, Eric Paris wrote: > > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. > > It was writing over %esp/pt_regs semi-randomly on i686 with the expected > > "system can't boot" results. As noted in: > > > > https://bugs.freedesktop.org/show_bug.cgi?id=85277 > > > > This patch stops fscking with pt_regs. Instead it sets up the registers > > for the call to __audit_syscall_entry in the most obvious conceivable > > way. It then does just a tiny tiny touch of magic. We need to get what > > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy > > as a pair of pushes. > > > > After the call to __audit_syscall_entry all we need to do is get that > > now useless junk off the stack (pair of pops) and reload %eax with the > > original syscall so other stuff can keep going about it's business. > > > > Signed-off-by: Eric Paris <eparis@redhat.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > Cc: x86@kernel.org > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-audit@redhat.com > > --- > > arch/x86/kernel/entry_32.S | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > > index f9e3fab..fb01d22 100644 > > --- a/arch/x86/kernel/entry_32.S > > +++ b/arch/x86/kernel/entry_32.S > > @@ -447,15 +447,14 @@ sysenter_exit: > > sysenter_audit: > > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > > jnz syscall_trace_entry > > - addl $4,%esp > > - CFI_ADJUST_CFA_OFFSET -4 > > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ > > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ > > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ > > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > > - /* %eax already in %eax 1st arg: syscall number */ > > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ > > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ > > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ > > call __audit_syscall_entry > > - pushl_cfi %ebx > > + popl_cfi %ecx /* get that remapped edx off the stack */ > > + popl_cfi %ecx /* get that remapped esi off the stack */ > > movl PT_EAX(%esp),%eax /* reload syscall number */ > > jmp sysenter_do_call > > > > > > This looks reasonably likely to be correct, but this code is complicated > and now ever slower. I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by hand. But I figured this was reasonable enough... > How hard would it be to just delete it and replace it with a > straightforward two-phase trace invocation a la x86_64? For me? Hard. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 19:15 ` Eric Paris @ 2014-10-23 19:20 ` Andy Lutomirski 2014-10-23 19:30 ` Eric Paris 0 siblings, 1 reply; 23+ messages in thread From: Andy Lutomirski @ 2014-10-23 19:20 UTC (permalink / raw) To: Eric Paris Cc: Richard Guy Briggs, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML, linux-kernel@vger.kernel.org, linux-audit On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <eparis@redhat.com> wrote: > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote: >> On 10/22/2014 09:04 PM, Eric Paris wrote: >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. >> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected >> > "system can't boot" results. As noted in: >> > >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277 >> > >> > This patch stops fscking with pt_regs. Instead it sets up the registers >> > for the call to __audit_syscall_entry in the most obvious conceivable >> > way. It then does just a tiny tiny touch of magic. We need to get what >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy >> > as a pair of pushes. >> > >> > After the call to __audit_syscall_entry all we need to do is get that >> > now useless junk off the stack (pair of pops) and reload %eax with the >> > original syscall so other stuff can keep going about it's business. >> > >> > Signed-off-by: Eric Paris <eparis@redhat.com> >> > Cc: Thomas Gleixner <tglx@linutronix.de> >> > Cc: Ingo Molnar <mingo@redhat.com> >> > Cc: "H. Peter Anvin" <hpa@zytor.com> >> > Cc: x86@kernel.org >> > Cc: linux-kernel@vger.kernel.org >> > Cc: linux-audit@redhat.com >> > --- >> > arch/x86/kernel/entry_32.S | 15 +++++++-------- >> > 1 file changed, 7 insertions(+), 8 deletions(-) >> > >> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S >> > index f9e3fab..fb01d22 100644 >> > --- a/arch/x86/kernel/entry_32.S >> > +++ b/arch/x86/kernel/entry_32.S >> > @@ -447,15 +447,14 @@ sysenter_exit: >> > sysenter_audit: >> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) >> > jnz syscall_trace_entry >> > - addl $4,%esp >> > - CFI_ADJUST_CFA_OFFSET -4 >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ >> > - /* %eax already in %eax 1st arg: syscall number */ >> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ >> > call __audit_syscall_entry >> > - pushl_cfi %ebx >> > + popl_cfi %ecx /* get that remapped edx off the stack */ >> > + popl_cfi %ecx /* get that remapped esi off the stack */ >> > movl PT_EAX(%esp),%eax /* reload syscall number */ >> > jmp sysenter_do_call >> > >> > >> >> This looks reasonably likely to be correct, but this code is complicated >> and now ever slower. > > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by > hand. But I figured this was reasonable enough... > I'm not complaining about your new assembly in particular. There's just too much assembly in there in general. But I feel like I'm missing something in the new code. Aren't you corrupting ecx with those popl_cfi insns? >> How hard would it be to just delete it and replace it with a >> straightforward two-phase trace invocation a la x86_64? > > For me? Hard. > I can try it eventually, but my track record with the 32-bit entry code isn't so good. I agree that this would be pretty nasty for urgent, but at least neither that or your fix would need to go to -stable. --Andy ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 19:20 ` Andy Lutomirski @ 2014-10-23 19:30 ` Eric Paris 2014-10-23 19:38 ` Eric Paris ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Eric Paris @ 2014-10-23 19:30 UTC (permalink / raw) To: Andy Lutomirski Cc: linux-audit, Richard Guy Briggs, X86 ML, linux-kernel@vger.kernel.org, Ingo Molnar, H. Peter Anvin, Thomas Gleixner On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote: > On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <eparis@redhat.com> wrote: > > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote: > >> On 10/22/2014 09:04 PM, Eric Paris wrote: > >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. > >> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected > >> > "system can't boot" results. As noted in: > >> > > >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277 > >> > > >> > This patch stops fscking with pt_regs. Instead it sets up the registers > >> > for the call to __audit_syscall_entry in the most obvious conceivable > >> > way. It then does just a tiny tiny touch of magic. We need to get what > >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy > >> > as a pair of pushes. > >> > > >> > After the call to __audit_syscall_entry all we need to do is get that > >> > now useless junk off the stack (pair of pops) and reload %eax with the > >> > original syscall so other stuff can keep going about it's business. > >> > > >> > Signed-off-by: Eric Paris <eparis@redhat.com> > >> > Cc: Thomas Gleixner <tglx@linutronix.de> > >> > Cc: Ingo Molnar <mingo@redhat.com> > >> > Cc: "H. Peter Anvin" <hpa@zytor.com> > >> > Cc: x86@kernel.org > >> > Cc: linux-kernel@vger.kernel.org > >> > Cc: linux-audit@redhat.com > >> > --- > >> > arch/x86/kernel/entry_32.S | 15 +++++++-------- > >> > 1 file changed, 7 insertions(+), 8 deletions(-) > >> > > >> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > >> > index f9e3fab..fb01d22 100644 > >> > --- a/arch/x86/kernel/entry_32.S > >> > +++ b/arch/x86/kernel/entry_32.S > >> > @@ -447,15 +447,14 @@ sysenter_exit: > >> > sysenter_audit: > >> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > >> > jnz syscall_trace_entry > >> > - addl $4,%esp > >> > - CFI_ADJUST_CFA_OFFSET -4 > >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ > >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ > >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ > >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > >> > - /* %eax already in %eax 1st arg: syscall number */ > >> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ > >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ > >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ > >> > call __audit_syscall_entry > >> > - pushl_cfi %ebx > >> > + popl_cfi %ecx /* get that remapped edx off the stack */ > >> > + popl_cfi %ecx /* get that remapped esi off the stack */ > >> > movl PT_EAX(%esp),%eax /* reload syscall number */ > >> > jmp sysenter_do_call > >> > > >> > > >> > >> This looks reasonably likely to be correct, but this code is complicated > >> and now ever slower. > > > > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by > > hand. But I figured this was reasonable enough... > > > > I'm not complaining about your new assembly in particular. There's > just too much assembly in there in general. > > But I feel like I'm missing something in the new code. Aren't you > corrupting ecx with those popl_cfi insns? After the call __audit_syscall_entry aren't they already polluted? Isn't that the reason we need to reload EAX? You can verify this leaves things in a similar state (although slightly differently polluted) than before it got screwed up. Here is diff between before the breakage and what I propose we do now. (I admit I don't understand how the pushl_cfi %ebx wasn't messing up PT_EBX) /me anxiously awaits x86 guy to tell me how dumb I am.... $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f -- arch/x86/kernel/entry_32.S diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S index 0d0c9d4..fb01d22 100644 --- a/arch/x86/kernel/entry_32.S +++ b/arch/x86/kernel/entry_32.S @@ -447,16 +447,14 @@ sysenter_exit: sysenter_audit: testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) jnz syscall_trace_entry - addl $4,%esp - CFI_ADJUST_CFA_OFFSET -4 - /* %esi already in 8(%esp) 6th arg: 4th syscall arg */ - /* %edx already in 4(%esp) 5th arg: 3rd syscall arg */ - /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg */ - movl %ebx,%ecx /* 3rd arg: 1st syscall arg */ - movl %eax,%edx /* 2nd arg: syscall number */ - movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */ + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ call __audit_syscall_entry - pushl_cfi %ebx + popl_cfi %ecx /* get that remapped edx off the stack */ + popl_cfi %ecx /* get that remapped esi off the stack */ movl PT_EAX(%esp),%eax /* reload syscall number */ jmp sysenter_do_call ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 19:30 ` Eric Paris @ 2014-10-23 19:38 ` Eric Paris 2014-10-24 20:19 ` H. Peter Anvin 2014-10-23 20:30 ` Andy Lutomirski 2014-10-27 2:01 ` Richard Guy Briggs 2 siblings, 1 reply; 23+ messages in thread From: Eric Paris @ 2014-10-23 19:38 UTC (permalink / raw) To: Andy Lutomirski Cc: Richard Guy Briggs, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML, linux-kernel@vger.kernel.org, linux-audit On Thu, 2014-10-23 at 15:30 -0400, Eric Paris wrote: > On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote: > > On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <eparis@redhat.com> wrote: > > > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote: > > >> On 10/22/2014 09:04 PM, Eric Paris wrote: > > >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. > > >> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected > > >> > "system can't boot" results. As noted in: > > >> > > > >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277 > > >> > > > >> > This patch stops fscking with pt_regs. Instead it sets up the registers > > >> > for the call to __audit_syscall_entry in the most obvious conceivable > > >> > way. It then does just a tiny tiny touch of magic. We need to get what > > >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy > > >> > as a pair of pushes. > > >> > > > >> > After the call to __audit_syscall_entry all we need to do is get that > > >> > now useless junk off the stack (pair of pops) and reload %eax with the > > >> > original syscall so other stuff can keep going about it's business. > > >> > > > >> > Signed-off-by: Eric Paris <eparis@redhat.com> > > >> > Cc: Thomas Gleixner <tglx@linutronix.de> > > >> > Cc: Ingo Molnar <mingo@redhat.com> > > >> > Cc: "H. Peter Anvin" <hpa@zytor.com> > > >> > Cc: x86@kernel.org > > >> > Cc: linux-kernel@vger.kernel.org > > >> > Cc: linux-audit@redhat.com > > >> > --- > > >> > arch/x86/kernel/entry_32.S | 15 +++++++-------- > > >> > 1 file changed, 7 insertions(+), 8 deletions(-) > > >> > > > >> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > > >> > index f9e3fab..fb01d22 100644 > > >> > --- a/arch/x86/kernel/entry_32.S > > >> > +++ b/arch/x86/kernel/entry_32.S > > >> > @@ -447,15 +447,14 @@ sysenter_exit: > > >> > sysenter_audit: > > >> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > > >> > jnz syscall_trace_entry > > >> > - addl $4,%esp > > >> > - CFI_ADJUST_CFA_OFFSET -4 > > >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ > > >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ > > >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ > > >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > > >> > - /* %eax already in %eax 1st arg: syscall number */ > > >> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > > >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ > > >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > > >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ > > >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ > > >> > call __audit_syscall_entry > > >> > - pushl_cfi %ebx > > >> > + popl_cfi %ecx /* get that remapped edx off the stack */ > > >> > + popl_cfi %ecx /* get that remapped esi off the stack */ > > >> > movl PT_EAX(%esp),%eax /* reload syscall number */ > > >> > jmp sysenter_do_call > > >> > > > >> > > > >> > > >> This looks reasonably likely to be correct, but this code is complicated > > >> and now ever slower. > > > > > > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by > > > hand. But I figured this was reasonable enough... > > > > > > > I'm not complaining about your new assembly in particular. There's > > just too much assembly in there in general. > > > > But I feel like I'm missing something in the new code. Aren't you > > corrupting ecx with those popl_cfi insns? > > After the call __audit_syscall_entry aren't they already polluted? > Isn't that the reason we need to reload EAX? Well, I guess EAX is special... > You can verify this leaves > things in a similar state (although slightly differently polluted) than > before it got screwed up. Here is diff between before the breakage and > what I propose we do now. > > (I admit I don't understand how the pushl_cfi %ebx wasn't messing up > PT_EBX) > > /me anxiously awaits x86 guy to tell me how dumb I am.... > > $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f -- arch/x86/kernel/entry_32.S > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > index 0d0c9d4..fb01d22 100644 > --- a/arch/x86/kernel/entry_32.S > +++ b/arch/x86/kernel/entry_32.S > @@ -447,16 +447,14 @@ sysenter_exit: > sysenter_audit: > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > jnz syscall_trace_entry > - addl $4,%esp > - CFI_ADJUST_CFA_OFFSET -4 > - /* %esi already in 8(%esp) 6th arg: 4th syscall arg */ > - /* %edx already in 4(%esp) 5th arg: 3rd syscall arg */ > - /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg */ > - movl %ebx,%ecx /* 3rd arg: 1st syscall arg */ > - movl %eax,%edx /* 2nd arg: syscall number */ > - movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */ > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ > call __audit_syscall_entry > - pushl_cfi %ebx > + popl_cfi %ecx /* get that remapped edx off the stack */ > + popl_cfi %ecx /* get that remapped esi off the stack */ > movl PT_EAX(%esp),%eax /* reload syscall number */ > jmp sysenter_do_call > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 19:38 ` Eric Paris @ 2014-10-24 20:19 ` H. Peter Anvin 2014-10-25 0:00 ` Andy Lutomirski 0 siblings, 1 reply; 23+ messages in thread From: H. Peter Anvin @ 2014-10-24 20:19 UTC (permalink / raw) To: Eric Paris, Andy Lutomirski Cc: Richard Guy Briggs, Thomas Gleixner, Ingo Molnar, X86 ML, linux-kernel@vger.kernel.org, linux-audit On 10/23/2014 12:38 PM, Eric Paris wrote: >> >> After the call __audit_syscall_entry aren't they already polluted? >> Isn't that the reason we need to reload EAX? > > Well, I guess EAX is special... > Because system calls are "asmlinkage", all the parameters are on the stack, but %eax is used as the index into the system call table. This should thus be fine until we get rid of regparm(0) entirely, if that ever happens. -hpa ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-24 20:19 ` H. Peter Anvin @ 2014-10-25 0:00 ` Andy Lutomirski 2014-10-27 2:06 ` Richard Guy Briggs 0 siblings, 1 reply; 23+ messages in thread From: Andy Lutomirski @ 2014-10-25 0:00 UTC (permalink / raw) To: H. Peter Anvin Cc: Eric Paris, Richard Guy Briggs, Thomas Gleixner, Ingo Molnar, X86 ML, linux-kernel@vger.kernel.org, linux-audit On Fri, Oct 24, 2014 at 1:19 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 10/23/2014 12:38 PM, Eric Paris wrote: >>> >>> After the call __audit_syscall_entry aren't they already polluted? >>> Isn't that the reason we need to reload EAX? >> >> Well, I guess EAX is special... >> > > Because system calls are "asmlinkage", all the parameters are on the > stack, but %eax is used as the index into the system call table. This > should thus be fine until we get rid of regparm(0) entirely, if that > ever happens. > ...and because __audit_syscall_entry *isn't* asmlinkage, it uses the other convention, which is where the confusion comes from. And, by the time you get to sysenter_do_call, nothing cares about ecx, so you can freely clobber it while popping from the stack. I get it now. --Andy > -hpa > > -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-25 0:00 ` Andy Lutomirski @ 2014-10-27 2:06 ` Richard Guy Briggs 0 siblings, 0 replies; 23+ messages in thread From: Richard Guy Briggs @ 2014-10-27 2:06 UTC (permalink / raw) To: Andy Lutomirski Cc: H. Peter Anvin, Eric Paris, Thomas Gleixner, Ingo Molnar, X86 ML, linux-kernel@vger.kernel.org, linux-audit On 14/10/24, Andy Lutomirski wrote: > On Fri, Oct 24, 2014 at 1:19 PM, H. Peter Anvin <hpa@zytor.com> wrote: > > On 10/23/2014 12:38 PM, Eric Paris wrote: > >>> After the call __audit_syscall_entry aren't they already polluted? > >>> Isn't that the reason we need to reload EAX? > >> > >> Well, I guess EAX is special... > > > > Because system calls are "asmlinkage", all the parameters are on the > > stack, but %eax is used as the index into the system call table. This > > should thus be fine until we get rid of regparm(0) entirely, if that > > ever happens. > > ...and because __audit_syscall_entry *isn't* asmlinkage, it uses the > other convention, which is where the confusion comes from. And, by > the time you get to sysenter_do_call, nothing cares about ecx, so you > can freely clobber it while popping from the stack. I get it now. So you could just as easily clobber %eax since that'll be restored from PT_EAX(%esp) anyways in the following step... Or instead of popping these two values, could ajust the stack with addl $8,%esp CFI_ADJUST_CFA_OFFSET -8 since we don't need either value popped off the stack? > --Andy > > > -hpa - RGB -- Richard Guy Briggs <rbriggs@redhat.com> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 19:30 ` Eric Paris 2014-10-23 19:38 ` Eric Paris @ 2014-10-23 20:30 ` Andy Lutomirski 2014-10-24 2:55 ` H. Peter Anvin 2014-10-27 2:01 ` Richard Guy Briggs 2 siblings, 1 reply; 23+ messages in thread From: Andy Lutomirski @ 2014-10-23 20:30 UTC (permalink / raw) To: Eric Paris Cc: Richard Guy Briggs, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML, linux-kernel@vger.kernel.org, linux-audit On Thu, Oct 23, 2014 at 12:30 PM, Eric Paris <eparis@redhat.com> wrote: > On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote: >> On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <eparis@redhat.com> wrote: >> > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote: >> >> On 10/22/2014 09:04 PM, Eric Paris wrote: >> >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. >> >> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected >> >> > "system can't boot" results. As noted in: >> >> > >> >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277 >> >> > >> >> > This patch stops fscking with pt_regs. Instead it sets up the registers >> >> > for the call to __audit_syscall_entry in the most obvious conceivable >> >> > way. It then does just a tiny tiny touch of magic. We need to get what >> >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy >> >> > as a pair of pushes. >> >> > >> >> > After the call to __audit_syscall_entry all we need to do is get that >> >> > now useless junk off the stack (pair of pops) and reload %eax with the >> >> > original syscall so other stuff can keep going about it's business. >> >> > >> >> > Signed-off-by: Eric Paris <eparis@redhat.com> >> >> > Cc: Thomas Gleixner <tglx@linutronix.de> >> >> > Cc: Ingo Molnar <mingo@redhat.com> >> >> > Cc: "H. Peter Anvin" <hpa@zytor.com> >> >> > Cc: x86@kernel.org >> >> > Cc: linux-kernel@vger.kernel.org >> >> > Cc: linux-audit@redhat.com >> >> > --- >> >> > arch/x86/kernel/entry_32.S | 15 +++++++-------- >> >> > 1 file changed, 7 insertions(+), 8 deletions(-) >> >> > >> >> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S >> >> > index f9e3fab..fb01d22 100644 >> >> > --- a/arch/x86/kernel/entry_32.S >> >> > +++ b/arch/x86/kernel/entry_32.S >> >> > @@ -447,15 +447,14 @@ sysenter_exit: >> >> > sysenter_audit: >> >> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) >> >> > jnz syscall_trace_entry >> >> > - addl $4,%esp >> >> > - CFI_ADJUST_CFA_OFFSET -4 >> >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ >> >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ >> >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ >> >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ >> >> > - /* %eax already in %eax 1st arg: syscall number */ >> >> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ >> >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ >> >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ >> >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ >> >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ >> >> > call __audit_syscall_entry >> >> > - pushl_cfi %ebx >> >> > + popl_cfi %ecx /* get that remapped edx off the stack */ >> >> > + popl_cfi %ecx /* get that remapped esi off the stack */ >> >> > movl PT_EAX(%esp),%eax /* reload syscall number */ >> >> > jmp sysenter_do_call >> >> > >> >> > >> >> >> >> This looks reasonably likely to be correct, but this code is complicated >> >> and now ever slower. >> > >> > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by >> > hand. But I figured this was reasonable enough... >> > >> >> I'm not complaining about your new assembly in particular. There's >> just too much assembly in there in general. >> >> But I feel like I'm missing something in the new code. Aren't you >> corrupting ecx with those popl_cfi insns? > > After the call __audit_syscall_entry aren't they already polluted? > Isn't that the reason we need to reload EAX? You can verify this leaves > things in a similar state (although slightly differently polluted) than > before it got screwed up. Here is diff between before the breakage and > what I propose we do now. > > (I admit I don't understand how the pushl_cfi %ebx wasn't messing up > PT_EBX) > > /me anxiously awaits x86 guy to tell me how dumb I am.... > hpa, do you have time to figure this out? I don't know the 32-bit ABI well enough, nor will I have time to disassemble things or find and read the spec to figure this out in the next few days. --Andy > $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f -- arch/x86/kernel/entry_32.S > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > index 0d0c9d4..fb01d22 100644 > --- a/arch/x86/kernel/entry_32.S > +++ b/arch/x86/kernel/entry_32.S > @@ -447,16 +447,14 @@ sysenter_exit: > sysenter_audit: > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > jnz syscall_trace_entry > - addl $4,%esp > - CFI_ADJUST_CFA_OFFSET -4 > - /* %esi already in 8(%esp) 6th arg: 4th syscall arg */ > - /* %edx already in 4(%esp) 5th arg: 3rd syscall arg */ > - /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg */ > - movl %ebx,%ecx /* 3rd arg: 1st syscall arg */ > - movl %eax,%edx /* 2nd arg: syscall number */ > - movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */ > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ > call __audit_syscall_entry > - pushl_cfi %ebx > + popl_cfi %ecx /* get that remapped edx off the stack */ > + popl_cfi %ecx /* get that remapped esi off the stack */ > movl PT_EAX(%esp),%eax /* reload syscall number */ > jmp sysenter_do_call > > -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 20:30 ` Andy Lutomirski @ 2014-10-24 2:55 ` H. Peter Anvin 0 siblings, 0 replies; 23+ messages in thread From: H. Peter Anvin @ 2014-10-24 2:55 UTC (permalink / raw) To: Andy Lutomirski, Eric Paris Cc: Richard Guy Briggs, Thomas Gleixner, Ingo Molnar, X86 ML, linux-kernel@vger.kernel.org, linux-audit Yes, I will look at this tomorrow. For the record, the calling convention is that eax, edx, ecx are clobbered, and used for the three first arguments in that order. eax, edx are used for the return value(s). The exception is for __asmlinkage functions where all arguments are passed on the stack; something I would like to get rid of but would require changing a lot of assembly code. The same registers are still clobbered, though. On October 23, 2014 1:30:40 PM PDT, Andy Lutomirski <luto@amacapital.net> wrote: >On Thu, Oct 23, 2014 at 12:30 PM, Eric Paris <eparis@redhat.com> wrote: >> On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote: >>> On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <eparis@redhat.com> >wrote: >>> > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote: >>> >> On 10/22/2014 09:04 PM, Eric Paris wrote: >>> >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very >very dumb. >>> >> > It was writing over %esp/pt_regs semi-randomly on i686 with >the expected >>> >> > "system can't boot" results. As noted in: >>> >> > >>> >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277 >>> >> > >>> >> > This patch stops fscking with pt_regs. Instead it sets up the >registers >>> >> > for the call to __audit_syscall_entry in the most obvious >conceivable >>> >> > way. It then does just a tiny tiny touch of magic. We need to >get what >>> >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This >is as easy >>> >> > as a pair of pushes. >>> >> > >>> >> > After the call to __audit_syscall_entry all we need to do is >get that >>> >> > now useless junk off the stack (pair of pops) and reload %eax >with the >>> >> > original syscall so other stuff can keep going about it's >business. >>> >> > >>> >> > Signed-off-by: Eric Paris <eparis@redhat.com> >>> >> > Cc: Thomas Gleixner <tglx@linutronix.de> >>> >> > Cc: Ingo Molnar <mingo@redhat.com> >>> >> > Cc: "H. Peter Anvin" <hpa@zytor.com> >>> >> > Cc: x86@kernel.org >>> >> > Cc: linux-kernel@vger.kernel.org >>> >> > Cc: linux-audit@redhat.com >>> >> > --- >>> >> > arch/x86/kernel/entry_32.S | 15 +++++++-------- >>> >> > 1 file changed, 7 insertions(+), 8 deletions(-) >>> >> > >>> >> > diff --git a/arch/x86/kernel/entry_32.S >b/arch/x86/kernel/entry_32.S >>> >> > index f9e3fab..fb01d22 100644 >>> >> > --- a/arch/x86/kernel/entry_32.S >>> >> > +++ b/arch/x86/kernel/entry_32.S >>> >> > @@ -447,15 +447,14 @@ sysenter_exit: >>> >> > sysenter_audit: >>> >> > testl $(_TIF_WORK_SYSCALL_ENTRY & >~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) >>> >> > jnz syscall_trace_entry >>> >> > - addl $4,%esp >>> >> > - CFI_ADJUST_CFA_OFFSET -4 >>> >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg >*/ >>> >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg >*/ >>> >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg >*/ >>> >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg >*/ >>> >> > - /* %eax already in %eax 1st arg: syscall number >*/ >>> >> > + /* movl PT_EAX(%esp), %eax already set, syscall >number: 1st arg to audit */ >>> >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit >*/ >>> >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to >audit */ >>> >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ >>> >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ >>> >> > call __audit_syscall_entry >>> >> > - pushl_cfi %ebx >>> >> > + popl_cfi %ecx /* get that remapped edx off the stack */ >>> >> > + popl_cfi %ecx /* get that remapped esi off the stack */ >>> >> > movl PT_EAX(%esp),%eax /* reload syscall number */ >>> >> > jmp sysenter_do_call >>> >> > >>> >> > >>> >> >>> >> This looks reasonably likely to be correct, but this code is >complicated >>> >> and now ever slower. >>> > >>> > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET >by >>> > hand. But I figured this was reasonable enough... >>> > >>> >>> I'm not complaining about your new assembly in particular. There's >>> just too much assembly in there in general. >>> >>> But I feel like I'm missing something in the new code. Aren't you >>> corrupting ecx with those popl_cfi insns? >> >> After the call __audit_syscall_entry aren't they already polluted? >> Isn't that the reason we need to reload EAX? You can verify this >leaves >> things in a similar state (although slightly differently polluted) >than >> before it got screwed up. Here is diff between before the breakage >and >> what I propose we do now. >> >> (I admit I don't understand how the pushl_cfi %ebx wasn't messing up >> PT_EBX) >> >> /me anxiously awaits x86 guy to tell me how dumb I am.... >> > >hpa, do you have time to figure this out? I don't know the 32-bit ABI >well enough, nor will I have time to disassemble things or find and >read the spec to figure this out in the next few days. > >--Andy > >> $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f -- >arch/x86/kernel/entry_32.S >> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S >> index 0d0c9d4..fb01d22 100644 >> --- a/arch/x86/kernel/entry_32.S >> +++ b/arch/x86/kernel/entry_32.S >> @@ -447,16 +447,14 @@ sysenter_exit: >> sysenter_audit: >> testl $(_TIF_WORK_SYSCALL_ENTRY & >~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) >> jnz syscall_trace_entry >> - addl $4,%esp >> - CFI_ADJUST_CFA_OFFSET -4 >> - /* %esi already in 8(%esp) 6th arg: 4th syscall arg >*/ >> - /* %edx already in 4(%esp) 5th arg: 3rd syscall arg >*/ >> - /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg >*/ >> - movl %ebx,%ecx /* 3rd arg: 1st syscall arg >*/ >> - movl %eax,%edx /* 2nd arg: syscall number */ >> - movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */ >> + /* movl PT_EAX(%esp), %eax already set, syscall number: >1st arg to audit */ >> + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit >*/ >> + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to >audit */ >> + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ >> + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ >> call __audit_syscall_entry >> - pushl_cfi %ebx >> + popl_cfi %ecx /* get that remapped edx off the stack */ >> + popl_cfi %ecx /* get that remapped esi off the stack */ >> movl PT_EAX(%esp),%eax /* reload syscall number */ >> jmp sysenter_do_call >> >> -- Sent from my mobile phone. Please pardon brevity and lack of formatting. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-23 19:30 ` Eric Paris 2014-10-23 19:38 ` Eric Paris 2014-10-23 20:30 ` Andy Lutomirski @ 2014-10-27 2:01 ` Richard Guy Briggs 2 siblings, 0 replies; 23+ messages in thread From: Richard Guy Briggs @ 2014-10-27 2:01 UTC (permalink / raw) To: Eric Paris Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML, linux-kernel@vger.kernel.org, linux-audit On 14/10/23, Eric Paris wrote: > On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote: > > On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <eparis@redhat.com> wrote: > > > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote: > > >> On 10/22/2014 09:04 PM, Eric Paris wrote: > > >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. > > >> > It was writing over %esp/pt_regs semi-randomly on i686 with the expected > > >> > "system can't boot" results. As noted in: > > >> > > > >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277 > > >> > > > >> > This patch stops fscking with pt_regs. Instead it sets up the registers > > >> > for the call to __audit_syscall_entry in the most obvious conceivable > > >> > way. It then does just a tiny tiny touch of magic. We need to get what > > >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy > > >> > as a pair of pushes. > > >> > > > >> > After the call to __audit_syscall_entry all we need to do is get that > > >> > now useless junk off the stack (pair of pops) and reload %eax with the > > >> > original syscall so other stuff can keep going about it's business. > > >> > > > >> > Signed-off-by: Eric Paris <eparis@redhat.com> > > >> > Cc: Thomas Gleixner <tglx@linutronix.de> > > >> > Cc: Ingo Molnar <mingo@redhat.com> > > >> > Cc: "H. Peter Anvin" <hpa@zytor.com> > > >> > Cc: x86@kernel.org > > >> > Cc: linux-kernel@vger.kernel.org > > >> > Cc: linux-audit@redhat.com > > >> > --- > > >> > arch/x86/kernel/entry_32.S | 15 +++++++-------- > > >> > 1 file changed, 7 insertions(+), 8 deletions(-) > > >> > > > >> > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > > >> > index f9e3fab..fb01d22 100644 > > >> > --- a/arch/x86/kernel/entry_32.S > > >> > +++ b/arch/x86/kernel/entry_32.S > > >> > @@ -447,15 +447,14 @@ sysenter_exit: > > >> > sysenter_audit: > > >> > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > > >> > jnz syscall_trace_entry > > >> > - addl $4,%esp > > >> > - CFI_ADJUST_CFA_OFFSET -4 > > >> > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ > > >> > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ > > >> > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ > > >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > > >> > - /* %eax already in %eax 1st arg: syscall number */ > > >> > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > > >> > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ > > >> > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > > >> > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ > > >> > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ > > >> > call __audit_syscall_entry > > >> > - pushl_cfi %ebx > > >> > + popl_cfi %ecx /* get that remapped edx off the stack */ > > >> > + popl_cfi %ecx /* get that remapped esi off the stack */ > > >> > movl PT_EAX(%esp),%eax /* reload syscall number */ > > >> > jmp sysenter_do_call > > >> > > > >> > > > >> > > >> This looks reasonably likely to be correct, but this code is complicated > > >> and now ever slower. > > > > > > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET by > > > hand. But I figured this was reasonable enough... > > > > > > > I'm not complaining about your new assembly in particular. There's > > just too much assembly in there in general. > > > > But I feel like I'm missing something in the new code. Aren't you > > corrupting ecx with those popl_cfi insns? > > After the call __audit_syscall_entry aren't they already polluted? > Isn't that the reason we need to reload EAX? You can verify this leaves > things in a similar state (although slightly differently polluted) than > before it got screwed up. Here is diff between before the breakage and > what I propose we do now. > > (I admit I don't understand how the pushl_cfi %ebx wasn't messing up > PT_EBX) (Credit to HPA for walking me through some of this... I had to stare at it for a while...) The bottom of the stack was dropped to reuse syscall args a1-a3 on the stack for the call to __audit_syscall_entry while %ebx wasn't changed in the call. a0 was in the unchanged %ebx and the pushl_cfi directly restores the value in PT_EBX when it puts a0 back on the stack and restores the original value of %esp. > /me anxiously awaits x86 guy to tell me how dumb I am.... ("Dumber" tries to explain...) > $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f -- arch/x86/kernel/entry_32.S > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > index 0d0c9d4..fb01d22 100644 > --- a/arch/x86/kernel/entry_32.S > +++ b/arch/x86/kernel/entry_32.S > @@ -447,16 +447,14 @@ sysenter_exit: > sysenter_audit: > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > jnz syscall_trace_entry > - addl $4,%esp > - CFI_ADJUST_CFA_OFFSET -4 > - /* %esi already in 8(%esp) 6th arg: 4th syscall arg */ > - /* %edx already in 4(%esp) 5th arg: 3rd syscall arg */ > - /* %ecx already in 0(%esp) 4th arg: 2nd syscall arg */ > - movl %ebx,%ecx /* 3rd arg: 1st syscall arg */ > - movl %eax,%edx /* 2nd arg: syscall number */ > - movl $AUDIT_ARCH_I386,%eax /* 1st arg: audit arch */ > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > + movl PT_EBX(%esp), %edx /* ebx/a0: 2nd arg to audit */ > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > + pushl_cfi PT_ESI(%esp) /* a3: 5th arg */ > + pushl_cfi PT_EDX+4(%esp) /* a2: 4th arg */ > call __audit_syscall_entry > - pushl_cfi %ebx > + popl_cfi %ecx /* get that remapped edx off the stack */ > + popl_cfi %ecx /* get that remapped esi off the stack */ > movl PT_EAX(%esp),%eax /* reload syscall number */ > jmp sysenter_do_call > > - RGB -- Richard Guy Briggs <rbriggs@redhat.com> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <tip-26c2d2b39128adba276d140eefa2745591b88536@git.kernel.org>]
[parent not found: <alpine.DEB.2.11.1410251034320.5308@nanos>]
* [PATCH] i386/audit: stop scribbling on the stack frame [not found] ` <alpine.DEB.2.11.1410251034320.5308@nanos> @ 2014-10-27 2:34 ` Richard Guy Briggs 2014-10-27 13:55 ` Eric Paris 2014-10-27 20:52 ` Thomas Gleixner 0 siblings, 2 replies; 23+ messages in thread From: Richard Guy Briggs @ 2014-10-27 2:34 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel, eparis, przanoni, mingo, hpa, hpa, linux-tip-commits, linux-audit git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. It was writing over %esp/pt_regs semi-randomly on i686 with the expected "system can't boot" results. As noted in: https://bugs.freedesktop.org/show_bug.cgi?id=85277 This patch stops fscking with pt_regs. Instead it sets up the registers for the call to __audit_syscall_entry in the most obvious conceivable way. It then does just a tiny tiny touch of magic. We need to get what started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy as a pair of pushes using the values still in those registers. After the call to __audit_syscall_entry all we need to do is get that now useless junk off the stack (pair of pops) and reload %eax with the original syscall so other stuff can keep going about it's business. Reported-by: Paulo Zanoni <przanoni@gmail.com> Signed-off-by: Eric Paris <eparis@redhat.com> Signed-off-by: Richard Guy Briggs <rgb@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: x86@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-audit@redhat.com --- On 14/10/25, Thomas Gleixner wrote: > Why are we grabbing that from the stack? AFAICT all arguments are in > the registers still. Right, re-arranging the instructions slightly to avoid overwriting %edx with %ebx before needing it to push onto the stack, how does this look? arch/x86/kernel/entry_32.S | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S index b553ed8..344b63f 100644 --- a/arch/x86/kernel/entry_32.S +++ b/arch/x86/kernel/entry_32.S @@ -447,15 +447,14 @@ sysenter_exit: sysenter_audit: testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) jnz syscall_trace_entry - addl $4,%esp - CFI_ADJUST_CFA_OFFSET -4 - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax 1st arg: syscall number */ + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ + pushl_cfi %esi /* a3: 5th arg */ + pushl_cfi %edx /* a2: 4th arg */ + movl %ebx, %edx /* ebx/a0: 2nd arg to audit */ call __audit_syscall_entry - pushl_cfi %ebx + popl_cfi %ecx /* get that remapped edx off the stack */ + popl_cfi %ecx /* get that remapped esi off the stack */ movl PT_EAX(%esp),%eax /* reload syscall number */ jmp sysenter_do_call - RGB -- Richard Guy Briggs <rbriggs@redhat.com> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-27 2:34 ` Richard Guy Briggs @ 2014-10-27 13:55 ` Eric Paris 2014-10-27 17:02 ` H. Peter Anvin ` (2 more replies) 2014-10-27 20:52 ` Thomas Gleixner 1 sibling, 3 replies; 23+ messages in thread From: Eric Paris @ 2014-10-27 13:55 UTC (permalink / raw) To: Richard Guy Briggs Cc: Thomas Gleixner, linux-kernel, przanoni, mingo, hpa, hpa, linux-tip-commits, linux-audit My patch was already committed to the -tip urgent branch. I believe any optimization should be based on that branch, Richard. If you are trying to wrangle every bit of speed out of this, should you push %esi; push %edi; CFI_ADJUST_CFA_OFFSET 8 call __audit_syscall_entry pop; pop; CFI_ADJUST_CFA_OFFSET -8 Instead of using the pushl_cfi and popl_cfi macros? I wrote my patch to be obviously correct, but agree there are certainly some speedups possible. -Eric On Sun, 2014-10-26 at 22:34 -0400, Richard Guy Briggs wrote: > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. > It was writing over %esp/pt_regs semi-randomly on i686 with the expected > "system can't boot" results. As noted in: > > https://bugs.freedesktop.org/show_bug.cgi?id=85277 > > This patch stops fscking with pt_regs. Instead it sets up the registers > for the call to __audit_syscall_entry in the most obvious conceivable > way. It then does just a tiny tiny touch of magic. We need to get what > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy > as a pair of pushes using the values still in those registers. > > After the call to __audit_syscall_entry all we need to do is get that > now useless junk off the stack (pair of pops) and reload %eax with the > original syscall so other stuff can keep going about it's business. > > Reported-by: Paulo Zanoni <przanoni@gmail.com> > Signed-off-by: Eric Paris <eparis@redhat.com> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: x86@kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-audit@redhat.com > > --- > On 14/10/25, Thomas Gleixner wrote: > > Why are we grabbing that from the stack? AFAICT all arguments are in > > the registers still. > > Right, re-arranging the instructions slightly to avoid overwriting %edx > with %ebx before needing it to push onto the stack, how does this look? > > arch/x86/kernel/entry_32.S | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > index b553ed8..344b63f 100644 > --- a/arch/x86/kernel/entry_32.S > +++ b/arch/x86/kernel/entry_32.S > @@ -447,15 +447,14 @@ sysenter_exit: > sysenter_audit: > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > jnz syscall_trace_entry > - addl $4,%esp > - CFI_ADJUST_CFA_OFFSET -4 > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > - /* %eax already in %eax 1st arg: syscall number */ > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > + pushl_cfi %esi /* a3: 5th arg */ > + pushl_cfi %edx /* a2: 4th arg */ > + movl %ebx, %edx /* ebx/a0: 2nd arg to audit */ > call __audit_syscall_entry > - pushl_cfi %ebx > + popl_cfi %ecx /* get that remapped edx off the stack */ > + popl_cfi %ecx /* get that remapped esi off the stack */ > movl PT_EAX(%esp),%eax /* reload syscall number */ > jmp sysenter_do_call > > > - RGB > > -- > Richard Guy Briggs <rbriggs@redhat.com> > Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat > Remote, Ottawa, Canada > Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-27 13:55 ` Eric Paris @ 2014-10-27 17:02 ` H. Peter Anvin 2014-10-27 17:29 ` Eric Paris 2014-10-27 17:38 ` Richard Guy Briggs 2014-10-27 21:18 ` Thomas Gleixner 2 siblings, 1 reply; 23+ messages in thread From: H. Peter Anvin @ 2014-10-27 17:02 UTC (permalink / raw) To: Eric Paris, Richard Guy Briggs Cc: Thomas Gleixner, linux-kernel, przanoni, mingo, hpa, linux-tip-commits, linux-audit On 10/27/2014 06:55 AM, Eric Paris wrote: > My patch was already committed to the -tip urgent branch. I believe any > optimization should be based on that branch, Richard. If you are trying > to wrangle every bit of speed out of this, should you > > push %esi; > push %edi; > CFI_ADJUST_CFA_OFFSET 8 > call __audit_syscall_entry > pop; > pop; > CFI_ADJUST_CFA_OFFSET -8 > > Instead of using the pushl_cfi and popl_cfi macros? > > I wrote my patch to be obviously correct, but agree there are certainly > some speedups possible. > Uh... not only is that plain wrong (the CFI should be adjusted after each instruction that changes the stack pointer), but what the heck is wrong with using the macros? -hpa ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-27 17:02 ` H. Peter Anvin @ 2014-10-27 17:29 ` Eric Paris 2014-10-28 6:30 ` Ingo Molnar 0 siblings, 1 reply; 23+ messages in thread From: Eric Paris @ 2014-10-27 17:29 UTC (permalink / raw) To: H. Peter Anvin Cc: Richard Guy Briggs, Thomas Gleixner, linux-kernel, przanoni, mingo, hpa, linux-tip-commits, linux-audit On Mon, 2014-10-27 at 10:02 -0700, H. Peter Anvin wrote: > On 10/27/2014 06:55 AM, Eric Paris wrote: > > My patch was already committed to the -tip urgent branch. I believe any > > optimization should be based on that branch, Richard. If you are trying > > to wrangle every bit of speed out of this, should you > > > > push %esi; > > push %edi; > > CFI_ADJUST_CFA_OFFSET 8 > > call __audit_syscall_entry > > pop; > > pop; > > CFI_ADJUST_CFA_OFFSET -8 > > > > Instead of using the pushl_cfi and popl_cfi macros? > > > > I wrote my patch to be obviously correct, but agree there are certainly > > some speedups possible. > > > > Uh... not only is that plain wrong (the CFI should be adjusted after > each instruction that changes the stack pointer), Sure, things would be screwed up between the two push's > but what the heck is > wrong with using the macros? I was asking if that would save an instruction or two by consolidating the CFI update and if so would that tradeoff be worth it, given the regularity of this code being run. > > -hpa > > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-27 17:29 ` Eric Paris @ 2014-10-28 6:30 ` Ingo Molnar 0 siblings, 0 replies; 23+ messages in thread From: Ingo Molnar @ 2014-10-28 6:30 UTC (permalink / raw) To: Eric Paris Cc: H. Peter Anvin, Richard Guy Briggs, Thomas Gleixner, linux-kernel, przanoni, hpa, linux-tip-commits, linux-audit * Eric Paris <eparis@redhat.com> wrote: > On Mon, 2014-10-27 at 10:02 -0700, H. Peter Anvin wrote: > > On 10/27/2014 06:55 AM, Eric Paris wrote: > > > My patch was already committed to the -tip urgent branch. I believe any > > > optimization should be based on that branch, Richard. If you are trying > > > to wrangle every bit of speed out of this, should you > > > > > > push %esi; > > > push %edi; > > > CFI_ADJUST_CFA_OFFSET 8 > > > call __audit_syscall_entry > > > pop; > > > pop; > > > CFI_ADJUST_CFA_OFFSET -8 > > > > > > Instead of using the pushl_cfi and popl_cfi macros? > > > > > > I wrote my patch to be obviously correct, but agree there are certainly > > > some speedups possible. > > > > > > > Uh... not only is that plain wrong (the CFI should be adjusted after > > each instruction that changes the stack pointer), > > Sure, things would be screwed up between the two push's > > > but what the heck is > > wrong with using the macros? > > I was asking if that would save an instruction or two by > consolidating the CFI update and if so would that tradeoff be > worth it, given the regularity of this code being run. CFI updates have no effect on runtime behavior whatsoever (they don't emit any instructions), they only affect the debug info being constructed. Thanks, Ingo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-27 13:55 ` Eric Paris 2014-10-27 17:02 ` H. Peter Anvin @ 2014-10-27 17:38 ` Richard Guy Briggs 2014-10-27 21:18 ` Thomas Gleixner 2 siblings, 0 replies; 23+ messages in thread From: Richard Guy Briggs @ 2014-10-27 17:38 UTC (permalink / raw) To: Eric Paris Cc: Thomas Gleixner, linux-kernel, przanoni, mingo, hpa, hpa, linux-tip-commits, linux-audit On 14/10/27, Eric Paris wrote: > My patch was already committed to the -tip urgent branch. I believe any > optimization should be based on that branch, Richard. If you are trying > to wrangle every bit of speed out of this, should you > > push %esi; > push %edi; > CFI_ADJUST_CFA_OFFSET 8 > call __audit_syscall_entry > pop; > pop; > CFI_ADJUST_CFA_OFFSET -8 > > Instead of using the pushl_cfi and popl_cfi macros? > > I wrote my patch to be obviously correct, but agree there are certainly > some speedups possible. I was just more surprised that you didn't use the assumptions and approach of the original code before I botched it, which simply re-used values in registers that were already there, rather than fetching them from the pt_regs struct on the stack and adjusting the reference with an offset on the fly as you're pushing items onto the stack. > -Eric > > On Sun, 2014-10-26 at 22:34 -0400, Richard Guy Briggs wrote: > > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very very dumb. > > It was writing over %esp/pt_regs semi-randomly on i686 with the expected > > "system can't boot" results. As noted in: > > > > https://bugs.freedesktop.org/show_bug.cgi?id=85277 > > > > This patch stops fscking with pt_regs. Instead it sets up the registers > > for the call to __audit_syscall_entry in the most obvious conceivable > > way. It then does just a tiny tiny touch of magic. We need to get what > > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp). This is as easy > > as a pair of pushes using the values still in those registers. > > > > After the call to __audit_syscall_entry all we need to do is get that > > now useless junk off the stack (pair of pops) and reload %eax with the > > original syscall so other stuff can keep going about it's business. > > > > Reported-by: Paulo Zanoni <przanoni@gmail.com> > > Signed-off-by: Eric Paris <eparis@redhat.com> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > Cc: x86@kernel.org > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-audit@redhat.com > > > > --- > > On 14/10/25, Thomas Gleixner wrote: > > > Why are we grabbing that from the stack? AFAICT all arguments are in > > > the registers still. > > > > Right, re-arranging the instructions slightly to avoid overwriting %edx > > with %ebx before needing it to push onto the stack, how does this look? > > > > arch/x86/kernel/entry_32.S | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > > index b553ed8..344b63f 100644 > > --- a/arch/x86/kernel/entry_32.S > > +++ b/arch/x86/kernel/entry_32.S > > @@ -447,15 +447,14 @@ sysenter_exit: > > sysenter_audit: > > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > > jnz syscall_trace_entry > > - addl $4,%esp > > - CFI_ADJUST_CFA_OFFSET -4 > > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ > > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ > > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ > > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > > - /* %eax already in %eax 1st arg: syscall number */ > > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > > + pushl_cfi %esi /* a3: 5th arg */ > > + pushl_cfi %edx /* a2: 4th arg */ > > + movl %ebx, %edx /* ebx/a0: 2nd arg to audit */ > > call __audit_syscall_entry > > - pushl_cfi %ebx > > + popl_cfi %ecx /* get that remapped edx off the stack */ > > + popl_cfi %ecx /* get that remapped esi off the stack */ > > movl PT_EAX(%esp),%eax /* reload syscall number */ > > jmp sysenter_do_call > > > > > > - RGB > > > > -- > > Richard Guy Briggs <rbriggs@redhat.com> > > Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat > > Remote, Ottawa, Canada > > Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 > > - RGB -- Richard Guy Briggs <rbriggs@redhat.com> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-27 13:55 ` Eric Paris 2014-10-27 17:02 ` H. Peter Anvin 2014-10-27 17:38 ` Richard Guy Briggs @ 2014-10-27 21:18 ` Thomas Gleixner 2014-10-27 21:22 ` Richard Guy Briggs 2 siblings, 1 reply; 23+ messages in thread From: Thomas Gleixner @ 2014-10-27 21:18 UTC (permalink / raw) To: Eric Paris Cc: Richard Guy Briggs, linux-kernel, przanoni, mingo, hpa, hpa, linux-tip-commits, linux-audit On Mon, 27 Oct 2014, Eric Paris wrote: > My patch was already committed to the -tip urgent branch. I believe any > optimization should be based on that branch, Richard. If you are trying > to wrangle every bit of speed out of this, should you > > push %esi; > push %edi; > CFI_ADJUST_CFA_OFFSET 8 Wrong. You want to use pushl_cfi as you need the CFI adjustment after each modification of esp. > call __audit_syscall_entry > pop; > pop; > CFI_ADJUST_CFA_OFFSET -8 > > Instead of using the pushl_cfi and popl_cfi macros? Wrong again. See above. Aside of that, why do you want to use pop at all? All we care about is adjusting esp, right? Thanks, tglx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-27 21:18 ` Thomas Gleixner @ 2014-10-27 21:22 ` Richard Guy Briggs 0 siblings, 0 replies; 23+ messages in thread From: Richard Guy Briggs @ 2014-10-27 21:22 UTC (permalink / raw) To: Thomas Gleixner Cc: Eric Paris, linux-kernel, przanoni, mingo, hpa, hpa, linux-tip-commits, linux-audit On 14/10/27, Thomas Gleixner wrote: > On Mon, 27 Oct 2014, Eric Paris wrote: > > My patch was already committed to the -tip urgent branch. I believe any > > optimization should be based on that branch, Richard. If you are trying > > to wrangle every bit of speed out of this, should you > > > > push %esi; > > push %edi; > > CFI_ADJUST_CFA_OFFSET 8 > > Wrong. You want to use pushl_cfi as you need the CFI adjustment after > each modification of esp. > > > call __audit_syscall_entry > > pop; > > pop; > > CFI_ADJUST_CFA_OFFSET -8 > > > > Instead of using the pushl_cfi and popl_cfi macros? > > Wrong again. See above. Aside of that, why do you want to use pop at > all? All we care about is adjusting esp, right? Right. We don't care about those two values on the bottom of the stack. Just move the extended stack pointer and adjust CFI. > Thanks, > > tglx - RGB -- Richard Guy Briggs <rbriggs@redhat.com> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-27 2:34 ` Richard Guy Briggs 2014-10-27 13:55 ` Eric Paris @ 2014-10-27 20:52 ` Thomas Gleixner 2014-10-27 21:13 ` Eric Paris 1 sibling, 1 reply; 23+ messages in thread From: Thomas Gleixner @ 2014-10-27 20:52 UTC (permalink / raw) To: Richard Guy Briggs Cc: linux-kernel, eparis, przanoni, mingo, hpa, hpa, linux-tip-commits, linux-audit On Sun, 26 Oct 2014, Richard Guy Briggs wrote: > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > index b553ed8..344b63f 100644 > --- a/arch/x86/kernel/entry_32.S > +++ b/arch/x86/kernel/entry_32.S > @@ -447,15 +447,14 @@ sysenter_exit: > sysenter_audit: > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > jnz syscall_trace_entry > - addl $4,%esp > - CFI_ADJUST_CFA_OFFSET -4 > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > - /* %eax already in %eax 1st arg: syscall number */ > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > + pushl_cfi %esi /* a3: 5th arg */ > + pushl_cfi %edx /* a2: 4th arg */ > + movl %ebx, %edx /* ebx/a0: 2nd arg to audit */ > call __audit_syscall_entry > - pushl_cfi %ebx > + popl_cfi %ecx /* get that remapped edx off the stack */ > + popl_cfi %ecx /* get that remapped esi off the stack */ Why use pop instead of simply adjusting esp and CFI by 8? Thanks, tglx ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] i386/audit: stop scribbling on the stack frame 2014-10-27 20:52 ` Thomas Gleixner @ 2014-10-27 21:13 ` Eric Paris 0 siblings, 0 replies; 23+ messages in thread From: Eric Paris @ 2014-10-27 21:13 UTC (permalink / raw) To: Thomas Gleixner Cc: Richard Guy Briggs, linux-kernel, przanoni, mingo, hpa, hpa, linux-tip-commits, linux-audit On Mon, 2014-10-27 at 21:52 +0100, Thomas Gleixner wrote: > On Sun, 26 Oct 2014, Richard Guy Briggs wrote: > > diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S > > index b553ed8..344b63f 100644 > > --- a/arch/x86/kernel/entry_32.S > > +++ b/arch/x86/kernel/entry_32.S > > @@ -447,15 +447,14 @@ sysenter_exit: > > sysenter_audit: > > testl $(_TIF_WORK_SYSCALL_ENTRY & ~_TIF_SYSCALL_AUDIT),TI_flags(%ebp) > > jnz syscall_trace_entry > > - addl $4,%esp > > - CFI_ADJUST_CFA_OFFSET -4 > > - movl %esi,4(%esp) /* 5th arg: 4th syscall arg */ > > - movl %edx,(%esp) /* 4th arg: 3rd syscall arg */ > > - /* %ecx already in %ecx 3rd arg: 2nd syscall arg */ > > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > > - /* %eax already in %eax 1st arg: syscall number */ > > + /* movl PT_ECX(%esp), %ecx already set, a1: 3nd arg to audit */ > > + /* movl PT_EAX(%esp), %eax already set, syscall number: 1st arg to audit */ > > + pushl_cfi %esi /* a3: 5th arg */ > > + pushl_cfi %edx /* a2: 4th arg */ > > + movl %ebx, %edx /* ebx/a0: 2nd arg to audit */ > > call __audit_syscall_entry > > - pushl_cfi %ebx > > + popl_cfi %ecx /* get that remapped edx off the stack */ > > + popl_cfi %ecx /* get that remapped esi off the stack */ > > Why use pop instead of simply adjusting esp and CFI by 8? Certainly seems like a good idea for RGB's perf improvement patch to go on top of -tip urgent. -Eric ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2014-10-28 6:30 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-23 4:04 [PATCH] i386/audit: stop scribbling on the stack frame Eric Paris
2014-10-23 18:39 ` Andy Lutomirski
2014-10-23 19:13 ` H. Peter Anvin
2014-10-23 19:15 ` Eric Paris
2014-10-23 19:20 ` Andy Lutomirski
2014-10-23 19:30 ` Eric Paris
2014-10-23 19:38 ` Eric Paris
2014-10-24 20:19 ` H. Peter Anvin
2014-10-25 0:00 ` Andy Lutomirski
2014-10-27 2:06 ` Richard Guy Briggs
2014-10-23 20:30 ` Andy Lutomirski
2014-10-24 2:55 ` H. Peter Anvin
2014-10-27 2:01 ` Richard Guy Briggs
[not found] ` <tip-26c2d2b39128adba276d140eefa2745591b88536@git.kernel.org>
[not found] ` <alpine.DEB.2.11.1410251034320.5308@nanos>
2014-10-27 2:34 ` Richard Guy Briggs
2014-10-27 13:55 ` Eric Paris
2014-10-27 17:02 ` H. Peter Anvin
2014-10-27 17:29 ` Eric Paris
2014-10-28 6:30 ` Ingo Molnar
2014-10-27 17:38 ` Richard Guy Briggs
2014-10-27 21:18 ` Thomas Gleixner
2014-10-27 21:22 ` Richard Guy Briggs
2014-10-27 20:52 ` Thomas Gleixner
2014-10-27 21:13 ` Eric Paris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox