Re: [PATCH] i386/audit: stop scribbling on the stack frame

[H. Peter Anvin]

Yes, I will look at this tomorrow.

For the record, the calling convention is that eax, edx, ecx are clobbered, and used for the three first arguments in that order. eax, edx are used for the return value(s).

The exception is for __asmlinkage functions where all arguments are passed on the stack; something I would like to get rid of but would require changing a lot of assembly code.

The same registers are still clobbered, though.

On October 23, 2014 1:30:40 PM PDT, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
>On Thu, Oct 23, 2014 at 12:30 PM, Eric Paris <eparis@xxxxxxxxxx> wrote:
>> On Thu, 2014-10-23 at 12:20 -0700, Andy Lutomirski wrote:
>>> On Thu, Oct 23, 2014 at 12:15 PM, Eric Paris <eparis@xxxxxxxxxx>
>wrote:
>>> > On Thu, 2014-10-23 at 11:39 -0700, Andy Lutomirski wrote:
>>> >> On 10/22/2014 09:04 PM, Eric Paris wrote:
>>> >> > git commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a was very
>very dumb.
>>> >> > It was writing over %esp/pt_regs semi-randomly on i686  with
>the expected
>>> >> > "system can't boot" results.  As noted in:
>>> >> >
>>> >> > https://bugs.freedesktop.org/show_bug.cgi?id=85277
>>> >> >
>>> >> > This patch stops fscking with pt_regs.  Instead it sets up the
>registers
>>> >> > for the call to __audit_syscall_entry in the most obvious
>conceivable
>>> >> > way.  It then does just a tiny tiny touch of magic.  We need to
>get what
>>> >> > started in PT_EDX into 0(%esp) and PT_ESI into 4(%esp).  This
>is as easy
>>> >> > as a pair of pushes.
>>> >> >
>>> >> > After the call to __audit_syscall_entry all we need to do is
>get that
>>> >> > now useless junk off the stack (pair of pops) and reload %eax
>with the
>>> >> > original syscall so other stuff can keep going about it's
>business.
>>> >> >
>>> >> > Signed-off-by: Eric Paris <eparis@xxxxxxxxxx>
>>> >> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> >> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>>> >> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>>> >> > Cc: x86@xxxxxxxxxx
>>> >> > Cc: linux-kernel@xxxxxxxxxxxxxxx
>>> >> > Cc: linux-audit@xxxxxxxxxx
>>> >> > ---
>>> >> >  arch/x86/kernel/entry_32.S | 15 +++++++--------
>>> >> >  1 file changed, 7 insertions(+), 8 deletions(-)
>>> >> >
>>> >> > diff --git a/arch/x86/kernel/entry_32.S
>b/arch/x86/kernel/entry_32.S
>>> >> > index f9e3fab..fb01d22 100644
>>> >> > --- a/arch/x86/kernel/entry_32.S
>>> >> > +++ b/arch/x86/kernel/entry_32.S
>>> >> > @@ -447,15 +447,14 @@ sysenter_exit:
>>> >> >  sysenter_audit:
>>> >> >     testl $(_TIF_WORK_SYSCALL_ENTRY &
>~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
>>> >> >     jnz syscall_trace_entry
>>> >> > -   addl $4,%esp
>>> >> > -   CFI_ADJUST_CFA_OFFSET -4
>>> >> > -   movl %esi,4(%esp)               /* 5th arg: 4th syscall arg
>*/
>>> >> > -   movl %edx,(%esp)                /* 4th arg: 3rd syscall arg
>*/
>>> >> > -   /* %ecx already in %ecx            3rd arg: 2nd syscall arg
>*/
>>> >> > -   movl %ebx,%edx                  /* 2nd arg: 1st syscall arg
>*/
>>> >> > -   /* %eax already in %eax            1st arg: syscall number
>*/
>>> >> > +   /* movl PT_EAX(%esp), %eax      already set, syscall
>number: 1st arg to audit */
>>> >> > +   movl PT_EBX(%esp), %edx         /* ebx/a0: 2nd arg to audit
>*/
>>> >> > +   /* movl PT_ECX(%esp), %ecx      already set, a1: 3nd arg to
>audit */
>>> >> > +   pushl_cfi PT_ESI(%esp)          /* a3: 5th arg */
>>> >> > +   pushl_cfi PT_EDX+4(%esp)        /* a2: 4th arg */
>>> >> >     call __audit_syscall_entry
>>> >> > -   pushl_cfi %ebx
>>> >> > +   popl_cfi %ecx /* get that remapped edx off the stack */
>>> >> > +   popl_cfi %ecx /* get that remapped esi off the stack */
>>> >> >     movl PT_EAX(%esp),%eax          /* reload syscall number */
>>> >> >     jmp sysenter_do_call
>>> >> >
>>> >> >
>>> >>
>>> >> This looks reasonably likely to be correct, but this code is
>complicated
>>> >> and now ever slower.
>>> >
>>> > I guess I could just use push/pop and do the CFI_ADJUST_CFA_OFFSET
>by
>>> > hand.  But I figured this was reasonable enough...
>>> >
>>>
>>> I'm not complaining about your new assembly in particular.  There's
>>> just too much assembly in there in general.
>>>
>>> But I feel like I'm missing something in the new code.  Aren't you
>>> corrupting ecx with those popl_cfi insns?
>>
>> After the call __audit_syscall_entry aren't they already polluted?
>> Isn't that the reason we need to reload EAX?  You can verify this
>leaves
>> things in a similar state (although slightly differently polluted)
>than
>> before it got screwed up.  Here is diff between before the breakage
>and
>> what I propose we do now.
>>
>> (I admit I don't understand how the pushl_cfi %ebx wasn't messing up
>> PT_EBX)
>>
>> /me anxiously awaits x86 guy to tell me how dumb I am....
>>
>
>hpa, do you have time to figure this out?  I don't know the 32-bit ABI
>well enough, nor will I have time to disassemble things or find and
>read the spec to figure this out in the next few days.
>
>--Andy
>
>> $ git diff a17c8b54dc738c4fda31e8be0302cd131a04c19f --
>arch/x86/kernel/entry_32.S
>> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
>> index 0d0c9d4..fb01d22 100644
>> --- a/arch/x86/kernel/entry_32.S
>> +++ b/arch/x86/kernel/entry_32.S
>> @@ -447,16 +447,14 @@ sysenter_exit:
>>  sysenter_audit:
>>         testl $(_TIF_WORK_SYSCALL_ENTRY &
>~_TIF_SYSCALL_AUDIT),TI_flags(%ebp)
>>         jnz syscall_trace_entry
>> -       addl $4,%esp
>> -       CFI_ADJUST_CFA_OFFSET -4
>> -       /* %esi already in 8(%esp)         6th arg: 4th syscall arg
>*/
>> -       /* %edx already in 4(%esp)         5th arg: 3rd syscall arg
>*/
>> -       /* %ecx already in 0(%esp)         4th arg: 2nd syscall arg
>*/
>> -       movl %ebx,%ecx                  /* 3rd arg: 1st syscall arg
>*/
>> -       movl %eax,%edx                  /* 2nd arg: syscall number */
>> -       movl $AUDIT_ARCH_I386,%eax      /* 1st arg: audit arch */
>> +       /* movl PT_EAX(%esp), %eax      already set, syscall number:
>1st arg to audit */
>> +       movl PT_EBX(%esp), %edx         /* ebx/a0: 2nd arg to audit
>*/
>> +       /* movl PT_ECX(%esp), %ecx      already set, a1: 3nd arg to
>audit */
>> +       pushl_cfi PT_ESI(%esp)          /* a3: 5th arg */
>> +       pushl_cfi PT_EDX+4(%esp)        /* a2: 4th arg */
>>         call __audit_syscall_entry
>> -       pushl_cfi %ebx
>> +       popl_cfi %ecx /* get that remapped edx off the stack */
>> +       popl_cfi %ecx /* get that remapped esi off the stack */
>>         movl PT_EAX(%esp),%eax          /* reload syscall number */
>>         jmp sysenter_do_call
>>
>>

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/