Re: [PATCH] i386/audit: stop scribbling on the stack frame
* Eric Paris 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
* Eric Paris epa...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 %ecx3rd arg: 2nd syscall arg */ > > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > > - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 %ecx3rd arg: 2nd syscall arg */ > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 > > Signed-off-by: Eric Paris > > Signed-off-by: Richard Guy Briggs > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: "H. Peter Anvin" > > Cc: x...@kernel.org > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ > > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > > - /* %eax already in %eax1st 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 > > 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 > Signed-off-by: Eric Paris > Signed-off-by: Richard Guy Briggs > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: x...@kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > - /* %eax already in %eax1st 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 > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 przan...@gmail.com Signed-off-by: Eric Paris epa...@redhat.com Signed-off-by: Richard Guy Briggs r...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 rbri...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 przan...@gmail.com Signed-off-by: Eric Paris epa...@redhat.com Signed-off-by: Richard Guy Briggs r...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 rbri...@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 rbri...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 rbri...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] i386/audit: stop scribbling on the stack frame
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 Signed-off-by: Eric Paris Signed-off-by: Richard Guy Briggs Cc: Thomas Gleixner Cc: Ingo Molnar Cc: "H. Peter Anvin" Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On 14/10/24, Andy Lutomirski wrote: > On Fri, Oct 24, 2014 at 1:19 PM, H. Peter Anvin 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 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
> > >> > Cc: Thomas Gleixner
> > >> > Cc: Ingo Molnar
> > >> > Cc: "H. Peter Anvin"
> > >> > Cc: x...@kernel.org
> > >> > Cc: linux-kernel@vger.kernel.org
> > >> > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */
> > >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */
> > >> > - /* %eax already in %eax1st 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
>
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 epa...@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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On 14/10/24, Andy Lutomirski wrote: On Fri, Oct 24, 2014 at 1:19 PM, H. Peter Anvin h...@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 rbri...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] i386/audit: stop scribbling on the stack frame
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 przan...@gmail.com Signed-off-by: Eric Paris epa...@redhat.com Signed-off-by: Richard Guy Briggs r...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 rbri...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On Fri, Oct 24, 2014 at 1:19 PM, H. Peter Anvin 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On Fri, Oct 24, 2014 at 1:19 PM, H. Peter Anvin h...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 wrote: >On Thu, Oct 23, 2014 at 12:30 PM, 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 >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 >>> >> > Cc: Thomas Gleixner >>> >> > Cc: Ingo Molnar >>> >> > Cc: "H. Peter Anvin" >>> >> > Cc: x...@kernel.org >>> >> > Cc: linux-kernel@vger.kernel.org >>> >> > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg >*/ >>> >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg >*/ >>> >> > - /* %eax already in %eax1st 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
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On Thu, Oct 23, 2014 at 12:30 PM, 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 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 >> >> > Cc: Thomas Gleixner >> >> > Cc: Ingo Molnar >> >> > Cc: "H. Peter Anvin" >> >> > Cc: x...@kernel.org >> >> > Cc: linux-kernel@vger.kernel.org >> >> > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ >> >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ >> >> > - /* %eax already in %eax1st 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:
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote: > On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris 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 > >> > Cc: Thomas Gleixner > >> > Cc: Ingo Molnar > >> > Cc: "H. Peter Anvin" > >> > Cc: x...@kernel.org > >> > Cc: linux-kernel@vger.kernel.org > >> > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ > >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > >> > - /* %eax already in %eax1st 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 */ +
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 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 > > >> > Cc: Thomas Gleixner > > >> > Cc: Ingo Molnar > > >> > Cc: "H. Peter Anvin" > > >> > Cc: x...@kernel.org > > >> > Cc: linux-kernel@vger.kernel.org > > >> > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ > > >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > > >> > - /* %eax already in %eax1st 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
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris 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 >> > Cc: Thomas Gleixner >> > Cc: Ingo Molnar >> > Cc: "H. Peter Anvin" >> > Cc: x...@kernel.org >> > Cc: linux-kernel@vger.kernel.org >> > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ >> > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ >> > - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: "H. Peter Anvin" > > Cc: x...@kernel.org > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ > > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > > - /* %eax already in %eax1st 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
Well, probably not something for stable/urgent... On October 23, 2014 11:39:48 AM PDT, 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 >> Cc: Thomas Gleixner >> Cc: Ingo Molnar >> Cc: "H. Peter Anvin" >> Cc: x...@kernel.org >> Cc: linux-kernel@vger.kernel.org >> Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ >> -movl %ebx,%edx /* 2nd arg: 1st syscall arg */ >> -/* %eax already in %eax1st 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: x...@kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ > - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ > - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
Well, probably not something for stable/urgent... On October 23, 2014 11:39:48 AM PDT, Andy Lutomirski l...@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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ -movl %ebx,%edx /* 2nd arg: 1st syscall arg */ -/* %eax already in %eax1st 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris epa...@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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 epa...@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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote: On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris epa...@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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 +
Re: [PATCH] i386/audit: stop scribbling on the stack frame
On Thu, Oct 23, 2014 at 12:30 PM, Eric Paris epa...@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 epa...@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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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
Re: [PATCH] i386/audit: stop scribbling on the stack frame
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 l...@amacapital.net wrote: On Thu, Oct 23, 2014 at 12:30 PM, Eric Paris epa...@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 epa...@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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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)
[PATCH] i386/audit: stop scribbling on the stack frame
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 Cc: Thomas Gleixner Cc: Ingo Molnar Cc: "H. Peter Anvin" Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] i386/audit: stop scribbling on the stack frame
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 epa...@redhat.com Cc: Thomas Gleixner t...@linutronix.de Cc: Ingo Molnar mi...@redhat.com Cc: H. Peter Anvin h...@zytor.com Cc: x...@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-au...@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 %ecx3rd arg: 2nd syscall arg */ - movl %ebx,%edx /* 2nd arg: 1st syscall arg */ - /* %eax already in %eax1st 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
