Regression: audit: x86: drop arch from __audit_syscall_entry() interface

Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[Paulo Zanoni]

Hi

(Cc'ing everybody mentioned in the original patch)

I work for Intel, on our Linux Graphics driver - aka i915.ko - and our
QA team recently reported a regression on:

commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a
Author: Richard Guy Briggs
Date:   Tue Mar 4 10:38:06 2014 -0500
    audit: x86: drop arch from __audit_syscall_entry() interface

According to our QA, their i386 machine doesn't boot anymore. I tried
to write my own revert for the patch, asked QA to test, and they
confirmed it "solves" the problem.

Here are the details of QA' s bug report:
https://bugs.freedesktop.org/show_bug.cgi?id=85277 .

The trees our QA tests are the development trees from i915.ko:
http://cgit.freedesktop.org/drm-intel?h=drm-intel-fixes .

I tried searching for other bug reports on the same patch, but
couldn't find any. Forgive me if this bug was already reported.

Feel free to continue this discussion on the bugzilla report if you want.

Thanks,
Paulo

-- 
Paulo Zanoni

Re: Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[Eric Paris]

That's really serious.  Looking now.

On Wed, 2014-10-22 at 16:08 -0200, Paulo Zanoni wrote:
> Hi
> 
> (Cc'ing everybody mentioned in the original patch)
> 
> I work for Intel, on our Linux Graphics driver - aka i915.ko - and our
> QA team recently reported a regression on:
> 
> commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a
> Author: Richard Guy Briggs
> Date:   Tue Mar 4 10:38:06 2014 -0500
>     audit: x86: drop arch from __audit_syscall_entry() interface
> 
> According to our QA, their i386 machine doesn't boot anymore. I tried
> to write my own revert for the patch, asked QA to test, and they
> confirmed it "solves" the problem.
> 
> Here are the details of QA' s bug report:
> https://bugs.freedesktop.org/show_bug.cgi?id=85277 .
> 
> The trees our QA tests are the development trees from i915.ko:
> http://cgit.freedesktop.org/drm-intel?h=drm-intel-fixes .
> 
> I tried searching for other bug reports on the same patch, but
> couldn't find any. Forgive me if this bug was already reported.
> 
> Feel free to continue this discussion on the bugzilla report if you want.
> 
> Thanks,
> Paulo
> 

Re: Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[Andy Lutomirski]

On 10/22/2014 11:23 AM, Eric Paris wrote:
> That's really serious.  Looking now.
> 
> On Wed, 2014-10-22 at 16:08 -0200, Paulo Zanoni wrote:
>> Hi
>>
>> (Cc'ing everybody mentioned in the original patch)
>>
>> I work for Intel, on our Linux Graphics driver - aka i915.ko - and our
>> QA team recently reported a regression on:
>>
>> commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a
>> Author: Richard Guy Briggs
>> Date:   Tue Mar 4 10:38:06 2014 -0500
>>     audit: x86: drop arch from __audit_syscall_entry() interface
>>
>> According to our QA, their i386 machine doesn't boot anymore. I tried
>> to write my own revert for the patch, asked QA to test, and they
>> confirmed it "solves" the problem.
>>
>> Here are the details of QA' s bug report:
>> https://bugs.freedesktop.org/show_bug.cgi?id=85277 .
>>
>> The trees our QA tests are the development trees from i915.ko:
>> http://cgit.freedesktop.org/drm-intel?h=drm-intel-fixes .
>>
>> I tried searching for other bug reports on the same patch, but
>> couldn't find any. Forgive me if this bug was already reported.
>>
>> Feel free to continue this discussion on the bugzilla report if you want.

This piece:

	movl %esi,4(%esp)		/* 5th arg: 4th syscall arg */
	movl %edx,(%esp)		/* 4th arg: 3rd syscall arg */

looks like it's overwriting syscall arguments.

This is clearly fixable, but an even better fix would be to drop the asm
entirely and switch to two-phase tracing.  Want to do it?  I can test
the seccomp bits if you switch over the asm :)

--Andy

Re: Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[Richard Guy Briggs]

On 14/10/22, Andy Lutomirski wrote:
> On 10/22/2014 11:23 AM, Eric Paris wrote:
> > That's really serious.  Looking now.
> > 
> > On Wed, 2014-10-22 at 16:08 -0200, Paulo Zanoni wrote:
> >> Hi
> >>
> >> (Cc'ing everybody mentioned in the original patch)
> >>
> >> I work for Intel, on our Linux Graphics driver - aka i915.ko - and our
> >> QA team recently reported a regression on:
> >>
> >> commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a
> >> Author: Richard Guy Briggs
> >> Date:   Tue Mar 4 10:38:06 2014 -0500
> >>     audit: x86: drop arch from __audit_syscall_entry() interface
> >>
> >> According to our QA, their i386 machine doesn't boot anymore. I tried
> >> to write my own revert for the patch, asked QA to test, and they
> >> confirmed it "solves" the problem.
> >>
> >> Here are the details of QA' s bug report:
> >> https://bugs.freedesktop.org/show_bug.cgi?id=85277 .
> >>
> >> The trees our QA tests are the development trees from i915.ko:
> >> http://cgit.freedesktop.org/drm-intel?h=drm-intel-fixes .
> >>
> >> I tried searching for other bug reports on the same patch, but
> >> couldn't find any. Forgive me if this bug was already reported.
> >>
> >> Feel free to continue this discussion on the bugzilla report if you want.
> 
> This piece:
> 
> 	movl %esi,4(%esp)		/* 5th arg: 4th syscall arg */
> 	movl %edx,(%esp)		/* 4th arg: 3rd syscall arg */
> 
> looks like it's overwriting syscall arguments.
> 
> This is clearly fixable, but an even better fix would be to drop the asm
> entirely and switch to two-phase tracing.  Want to do it?  I can test
> the seccomp bits if you switch over the asm :)

Like what you did for x86_64.  That sounds worth investigating.

I'll have a look at the asm, but I'm being distracted by a gunman loose
2km from me and my wife and kids under lockdown in two different
locations on the other side of the shooting site.  Had to cancel lunch
today with two work colleagues 1/2km away from that site.  ...not been a
productive day.

> --Andy

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[Andy Lutomirski]

On Wed, Oct 22, 2014 at 12:16 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 14/10/22, Andy Lutomirski wrote:
>> On 10/22/2014 11:23 AM, Eric Paris wrote:
>> > That's really serious.  Looking now.
>> >
>> > On Wed, 2014-10-22 at 16:08 -0200, Paulo Zanoni wrote:
>> >> Hi
>> >>
>> >> (Cc'ing everybody mentioned in the original patch)
>> >>
>> >> I work for Intel, on our Linux Graphics driver - aka i915.ko - and our
>> >> QA team recently reported a regression on:
>> >>
>> >> commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a
>> >> Author: Richard Guy Briggs
>> >> Date:   Tue Mar 4 10:38:06 2014 -0500
>> >>     audit: x86: drop arch from __audit_syscall_entry() interface
>> >>
>> >> According to our QA, their i386 machine doesn't boot anymore. I tried
>> >> to write my own revert for the patch, asked QA to test, and they
>> >> confirmed it "solves" the problem.
>> >>
>> >> Here are the details of QA' s bug report:
>> >> https://bugs.freedesktop.org/show_bug.cgi?id=85277 .
>> >>
>> >> The trees our QA tests are the development trees from i915.ko:
>> >> http://cgit.freedesktop.org/drm-intel?h=drm-intel-fixes .
>> >>
>> >> I tried searching for other bug reports on the same patch, but
>> >> couldn't find any. Forgive me if this bug was already reported.
>> >>
>> >> Feel free to continue this discussion on the bugzilla report if you want.
>>
>> This piece:
>>
>>       movl %esi,4(%esp)               /* 5th arg: 4th syscall arg */
>>       movl %edx,(%esp)                /* 4th arg: 3rd syscall arg */
>>
>> looks like it's overwriting syscall arguments.
>>
>> This is clearly fixable, but an even better fix would be to drop the asm
>> entirely and switch to two-phase tracing.  Want to do it?  I can test
>> the seccomp bits if you switch over the asm :)
>
> Like what you did for x86_64.  That sounds worth investigating.
>
> I'll have a look at the asm, but I'm being distracted by a gunman loose
> 2km from me and my wife and kids under lockdown in two different
> locations on the other side of the shooting site.  Had to cancel lunch
> today with two work colleagues 1/2km away from that site.  ...not been a
> productive day.
>

That's putting it mildly.  Stay safe.

--Andy

Re: Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[Thomas Gleixner]

On Wed, 22 Oct 2014, Eric Paris wrote:

> That's really serious.  Looking now.

Indeed its serious. And it's even more serious as this masterpiece of
assembly wreckage was pulled in via your tree w/o having an acked-by
one of the x86 maintainers.

> On Wed, 2014-10-22 at 16:08 -0200, Paulo Zanoni wrote:
> > commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a
> > Author: Richard Guy Briggs
> > Date:   Tue Mar 4 10:38:06 2014 -0500
> >     audit: x86: drop arch from __audit_syscall_entry() interface
> > 
> > According to our QA, their i386 machine doesn't boot anymore. I tried
> > to write my own revert for the patch, asked QA to test, and they
> > confirmed it "solves" the problem.

diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 0d0c9d4ab6d5..f9e3fabc8716 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -449,12 +449,11 @@ sysenter_audit:
 	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 %esi,4(%esp)		/* 5th arg: 4th syscall arg */
+	movl %edx,(%esp)		/* 4th arg: 3rd syscall arg */

Bilndly overwriting the stack which holds the syscall arguments is
really a brilliant way to ensure security.

Thanks,

	tglx

Re: Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[Eric Paris]

On Wed, 2014-10-22 at 23:36 +0200, Thomas Gleixner wrote:
> On Wed, 22 Oct 2014, Eric Paris wrote:
> 
> > That's really serious.  Looking now.
> 
> Indeed its serious. And it's even more serious as this masterpiece of
> assembly wreckage was pulled in via your tree w/o having an acked-by
> one of the x86 maintainers.
> 
> > On Wed, 2014-10-22 at 16:08 -0200, Paulo Zanoni wrote:
> > > commit b4f0d3755c5e9cc86292d5fd78261903b4f23d4a
> > > Author: Richard Guy Briggs
> > > Date:   Tue Mar 4 10:38:06 2014 -0500
> > >     audit: x86: drop arch from __audit_syscall_entry() interface
> > > 
> > > According to our QA, their i386 machine doesn't boot anymore. I tried
> > > to write my own revert for the patch, asked QA to test, and they
> > > confirmed it "solves" the problem.
> 
> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
> index 0d0c9d4ab6d5..f9e3fabc8716 100644
> --- a/arch/x86/kernel/entry_32.S
> +++ b/arch/x86/kernel/entry_32.S
> @@ -449,12 +449,11 @@ sysenter_audit:
>  	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 %esi,4(%esp)		/* 5th arg: 4th syscall arg */
> +	movl %edx,(%esp)		/* 4th arg: 3rd syscall arg */
> 
> Bilndly overwriting the stack which holds the syscall arguments is
> really a brilliant way to ensure security.

It was sent, numerous times, to the x86 list for reviews, and lived in
-next for 2 complete devel cycles without a complaint.  I'm trying to
get an i386 system to test a fix.  But yes, it's total crap.

-Eric

Re: Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[H. Peter Anvin]

On 10/22/2014 02:38 PM, Eric Paris wrote:
> 
> It was sent, numerous times, to the x86 list for reviews, and lived in
> -next for 2 complete devel cycles without a complaint.  I'm trying to
> get an i386 system to test a fix.  But yes, it's total crap.
> 

You don't need an i386 system -- you can install an i386 distro on an
x86-64 system, or in KVM.

	-hpa

Re: Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[Eric Paris]

On Wed, 2014-10-22 at 14:43 -0700, H. Peter Anvin wrote:
> On 10/22/2014 02:38 PM, Eric Paris wrote:
> > 
> > It was sent, numerous times, to the x86 list for reviews, and lived in
> > -next for 2 complete devel cycles without a complaint.  I'm trying to
> > get an i386 system to test a fix.  But yes, it's total crap.
> > 
> 
> You don't need an i386 system -- you can install an i386 distro on an
> x86-64 system, or in KVM.

I'm currently building on i386 on KVM.  That's what I meant by "get"

Re: Regression: audit: x86: drop arch from __audit_syscall_entry() interface

[Eric Paris]

On Wed, 2014-10-22 at 14:43 -0700, H. Peter Anvin wrote:
> On 10/22/2014 02:38 PM, Eric Paris wrote:
> > 
> > It was sent, numerous times, to the x86 list for reviews, and lived in
> > -next for 2 complete devel cycles without a complaint.  I'm trying to
> > get an i386 system to test a fix.  But yes, it's total crap.
> > 
> 
> You don't need an i386 system -- you can install an i386 distro on an
> x86-64 system, or in KVM.

So I might still be an idiot, because I still haven't gotten a working
kernel.  But I can't get Linus' latest not panic even without
CONFIG_AUDITSYSCALL.  I kept blaming myself for not fixing this problem,
but reverting the patch like the reporter didn't give me bootable
kernels either.

I just jumped back in time and am looking to get anything I build to
boot...