Fixing audit on ARM

Fixing audit on ARM

[Jon Masters]

Hi Folks,

I've been playing around with getting ARM kernels built in more standard
Fedora configurations (as have a number of other members of the Fedora
ARM community). This has turned up a bunch of problems so far, which
is both good and bad. The upside is that we can all work together to
make sure some of these non-embedded features get more testing :)

Anyway. I'm sending a trivial fix that prevents systemd going into a
forkbomb OOM tailspin on all recent Fedora ARM kernels (unless we
turn off audit). Note that the fact that this has been corrupting
userspace up to this point suggests strongly to me that we should
be also considering turning off audit on ARM systems until it is
known that there are not other issues. But I can run my test
system successfully, so I submit this fix for review anyway.

Thanks,

Jon.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Jon Masters]

The audit subsystem builds upon ptrace to record system calls. This is done
in a couple of places (on return from fork into a new task, on exit from
the SWI vector), using calls to syscall_trace. The latter function abuses
the userspace intra-procedure scratch register (regs->ARM_ip, aka r12),
and intends to restore it prior to return to userspace. Unfortunately,
there are cases where we will return to userspace without restoring.

If we are in fact not ptracing but are merely auditing calls, we will
happily trash the content of ip but will exit to userspace without
restoring the value. It just so happens that GLIBC uses ip as a
storage for the TLS thread pointer info, and bad things result.

The fix is simply to have an additional out when not ptracing.

Signed-off-by: Jon Masters <jcm@jonmasters.org>
---
 arch/arm/kernel/ptrace.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index ede6443..7b05b6e 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -928,10 +928,10 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 		audit_syscall_entry(AUDIT_ARCH_NR, scno, regs->ARM_r0,
 				    regs->ARM_r1, regs->ARM_r2, regs->ARM_r3);
 
-	if (!test_thread_flag(TIF_SYSCALL_TRACE))
-		return scno;
+	if (!test_thread_flag(TIF_SYSCALL_TRACE)) 
+		goto out_no_ptrace;
 	if (!(current->ptrace & PT_PTRACED))
-		return scno;
+		goto out_no_ptrace;
 
 	current_thread_info()->syscall = scno;
 
@@ -951,4 +951,8 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 	regs->ARM_ip = ip;
 
 	return current_thread_info()->syscall;
+
+out_no_ptrace:
+	regs->ARM_ip = ip;
+	return scno;
 }
-- 
1.7.7.4

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Will Deacon]

Hi Jon,

On Sun, Apr 29, 2012 at 07:38:24AM +0100, Jon Masters wrote:
> The audit subsystem builds upon ptrace to record system calls. This is done
> in a couple of places (on return from fork into a new task, on exit from
> the SWI vector), using calls to syscall_trace. The latter function abuses
> the userspace intra-procedure scratch register (regs->ARM_ip, aka r12),
> and intends to restore it prior to return to userspace. Unfortunately,
> there are cases where we will return to userspace without restoring.
> 
> If we are in fact not ptracing but are merely auditing calls, we will
> happily trash the content of ip but will exit to userspace without
> restoring the value. It just so happens that GLIBC uses ip as a
> storage for the TLS thread pointer info, and bad things result.

Ouch, that's pretty horrible. We should have spotted this when we were
fixing the build breakage introduced by the audit stuff. Looking more
closely, does this code work even with your patch? It looks like we use the
saved ip to infer syscall entry, rather than why. On top of that, I think
the polarity is back-to-front.

> The fix is simply to have an additional out when not ptracing.

Actually, I don't understand why we have to update pt_regs so early given
that I don't think the saved ip is used by audit_syscall_{entry,exit} at
all. Perhaps we could just move the ip manipulation until after the thread
flag checks [completely untested patch below]?


diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 80abafb..bfcadc0 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -916,14 +916,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 {
        unsigned long ip;
 
-       /*
-        * Save IP.  IP is used to denote syscall entry/exit:
-        *  IP = 0 -> entry, = 1 -> exit
-        */
-       ip = regs->ARM_ip;
-       regs->ARM_ip = why;
-
-       if (!ip)
+       if (why)
                audit_syscall_exit(regs);
        else
                audit_syscall_entry(AUDIT_ARCH_NR, scno, regs->ARM_r0,
@@ -936,6 +929,13 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
 
        current_thread_info()->syscall = scno;
 
+       /*
+        * IP is used to denote syscall entry/exit:
+        * IP = 0 -> entry, = 1 -> exit
+        */
+       ip = regs->ARM_ip;
+       regs->ARM_ip = why;
+
        /* the 0x80 provides a way for the tracing parent to distinguish
           between a syscall stop and SIGTRAP delivery */
        ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)


Also note that this will conflict with 7375/1 in the patch system, but this
should probably take priority and be CC'd for stable once we can convince
ourselves that it's working properly.

Can you take it for a spin please?

Will

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Jon Masters]

On 04/30/2012 06:07 AM, Will Deacon wrote:
> Hi Jon,
> 
> On Sun, Apr 29, 2012 at 07:38:24AM +0100, Jon Masters wrote:
>> The audit subsystem builds upon ptrace to record system calls. This is done
>> in a couple of places (on return from fork into a new task, on exit from
>> the SWI vector), using calls to syscall_trace. The latter function abuses
>> the userspace intra-procedure scratch register (regs->ARM_ip, aka r12),
>> and intends to restore it prior to return to userspace. Unfortunately,
>> there are cases where we will return to userspace without restoring.
>>
>> If we are in fact not ptracing but are merely auditing calls, we will
>> happily trash the content of ip but will exit to userspace without
>> restoring the value. It just so happens that GLIBC uses ip as a
>> storage for the TLS thread pointer info, and bad things result.
> 
> Ouch, that's pretty horrible. We should have spotted this when we were
> fixing the build breakage introduced by the audit stuff. Looking more
> closely, does this code work even with your patch? It looks like we use the
> saved ip to infer syscall entry, rather than why. On top of that, I think
> the polarity is back-to-front.

Yea, so like I said (I think?) elsewhere, I've not tested to see that
audit works in anger yet. It was a case of dealing with the register
corruption issue (which has - to use a British phase - taken me around
the houses finding it) first, then wanting to figure out what's left.

But I'll look over your patch and do some poking. Now that we know where
this problem is, I think the priority is for me to test this patch from
you (took the day off, but I'll give it a test tonight) to make sure
nothing blows up, then schedule some time for audit to make sure it's
actually doing anything useful. I'll email you later today. Still
leaning toward recommending nobody actually turn on audit on ARM systems
until we know that it doesn't do anything else that's terrible.

Jon.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Will Deacon]

On Mon, Apr 30, 2012 at 07:55:32PM +0100, Jon Masters wrote:
> But I'll look over your patch and do some poking. Now that we know where
> this problem is, I think the priority is for me to test this patch from
> you (took the day off, but I'll give it a test tonight) to make sure
> nothing blows up, then schedule some time for audit to make sure it's
> actually doing anything useful. I'll email you later today. Still
> leaning toward recommending nobody actually turn on audit on ARM systems
> until we know that it doesn't do anything else that's terrible.

Well this might make you smile.

The original audit code blew up the ARM kernel because it assumed a big-endian
target, which we since fixed. However, it looks like the userspace audit tools
only support ARMEB, so I've not been able to get them working on my board.
Linaro even had the heart to package them up nicely in their v7l filesystem!

I doubt it's much effort to fix the tools, but it implies nobody is using
them on armv7l today and turning it off is probably your safest bet for the time
being.

Will

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Russell King - ARM Linux]

On Tue, May 01, 2012 at 12:07:45PM +0100, Will Deacon wrote:
> On Mon, Apr 30, 2012 at 07:55:32PM +0100, Jon Masters wrote:
> > But I'll look over your patch and do some poking. Now that we know where
> > this problem is, I think the priority is for me to test this patch from
> > you (took the day off, but I'll give it a test tonight) to make sure
> > nothing blows up, then schedule some time for audit to make sure it's
> > actually doing anything useful. I'll email you later today. Still
> > leaning toward recommending nobody actually turn on audit on ARM systems
> > until we know that it doesn't do anything else that's terrible.
> 
> Well this might make you smile.

No it doesn't.  This is insane and absurd.

You know my views on this crappy audit stuff, I made myself pretty clear
after we discovered the back door merging of this code which had zero
review from ARM people, and my view on that has just been reinforced by
your comments which clearly indicate that there's no way this has ever
been properly tested.

It should have never gone into the mainline kernel.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Jon Masters]

Hi Russell,

Hope you're doing well. Look forward to possibly seeing you in Hong Kong
- if you make it, I know you are insanely busy these days!

On 05/01/2012 07:37 AM, Russell King - ARM Linux wrote:
> On Tue, May 01, 2012 at 12:07:45PM +0100, Will Deacon wrote:
>> On Mon, Apr 30, 2012 at 07:55:32PM +0100, Jon Masters wrote:
>>> But I'll look over your patch and do some poking. Now that we know where
>>> this problem is, I think the priority is for me to test this patch from
>>> you (took the day off, but I'll give it a test tonight) to make sure
>>> nothing blows up, then schedule some time for audit to make sure it's
>>> actually doing anything useful. I'll email you later today. Still
>>> leaning toward recommending nobody actually turn on audit on ARM systems
>>> until we know that it doesn't do anything else that's terrible.
>>
>> Well this might make you smile.

Thanks Will, it did. I'll get back to poking at the followup patch you
posted. In the end, I tried actually taking the day off yesterday and
didn't get chance to poke at it some more. btw, it was awesome. I did
nothing, and it was everything I thought it could be </Office Space>.

> No it doesn't.  This is insane and absurd.
> 
> You know my views on this crappy audit stuff, I made myself pretty clear
> after we discovered the back door merging of this code which had zero
> review from ARM people, and my view on that has just been reinforced by
> your comments which clearly indicate that there's no way this has ever
> been properly tested.

I choose to take a different angle here. I think Eric and others are
doing good work, and I think on the whole we can solve some of these
problems by making sure more folks have access to ARM hardware -
something I am trying to get moving for at least RH employees.

> It should have never gone into the mainline kernel.

This I do agree with. But that's history. Let's figure this out, fix it,
and move on. I realize not everyone likes audit, but it's something that
exists on x86. For various reasons I'm very motivated that we have
feature parity with x86 in the ARM space, especially as ARM moves into
(shall we say) "bigger devices". Therefore, audit needs to ultimately
work on ARM systems if we want them to be in that space.

I want us on the RH/Fedora end of things to get more involved with ARM
kernel stuff upstream. We'll try to help with less embedded stuff that
needs testing in the future, especially by using it in the default
Fedora configuration, which has lots of non-embedded options on.

Jon.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Jon Masters]

On 05/01/2012 07:07 AM, Will Deacon wrote:
> On Mon, Apr 30, 2012 at 07:55:32PM +0100, Jon Masters wrote:
>> But I'll look over your patch and do some poking. Now that we know where
>> this problem is, I think the priority is for me to test this patch from
>> you (took the day off, but I'll give it a test tonight) to make sure
>> nothing blows up, then schedule some time for audit to make sure it's
>> actually doing anything useful. I'll email you later today. Still
>> leaning toward recommending nobody actually turn on audit on ARM systems
>> until we know that it doesn't do anything else that's terrible.
> 
> Well this might make you smile.

It did :)

> The original audit code blew up the ARM kernel because it assumed a big-endian
> target, which we since fixed. However, it looks like the userspace audit tools
> only support ARMEB, so I've not been able to get them working on my board.
> Linaro even had the heart to package them up nicely in their v7l filesystem!
> 
> I doubt it's much effort to fix the tools, but it implies nobody is using
> them on armv7l today and turning it off is probably your safest bet for the time
> being.

Right. So audit userspace has this:

static const struct int_transtab elftab[] = {
    { MACH_X86,     AUDIT_ARCH_I386   },
    { MACH_86_64,   AUDIT_ARCH_X86_64 },
    { MACH_IA64,    AUDIT_ARCH_IA64   },
    { MACH_PPC64,   AUDIT_ARCH_PPC64  },
    { MACH_PPC,     AUDIT_ARCH_PPC    },
    { MACH_S390X,   AUDIT_ARCH_S390X  },
    { MACH_S390,    AUDIT_ARCH_S390   },
#ifdef WITH_ALPHA
    { MACH_ALPHA,   AUDIT_ARCH_ALPHA  }
#endif
#ifdef WITH_ARMEB
    { MACH_ARMEB,   AUDIT_ARCH_ARMEB  }
#endif
};

However. I went through all of the kernel code and could see no arch
specificness other than the mach type (it already supports little arm)
so I think it's just userspace, and not much that needs changing. It
seems that it "works" for me because the default audit rules in Fedora
are "-D" (delete everything basically), unless I'm missing something.

Anyway. I'd like to get this fixed. I'll make some hardware available to
Eric (initially a shared test box, but we'll buy him an ARM board) and
I'm happy to test patches. I may get time this week to poke at it
myself, but I'm not counting on it. Meanwhile, I think it's harmless
actually to have audit enabled, just that userspace won't use it. I
prefer that we get in the habit of leaving non-embedded stuff turned on
where we can - and where we know it won't explode (I don't think this
will now that I've looked at it some more) for test coverage.

Finally, as an aside, and not meant as a jab, the the thing with Linaro
shipping this alludes to a bigger problem. We need to band together to
ensure that features common to "bigger" x86 systems get more coverage.
I'm trying to push us to do this on our end, and anything we can do
collaboratively to spot things like this is win for us all.

Thanks,

Jon.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Will Deacon]

On Wed, May 02, 2012 at 07:27:22AM +0100, Jon Masters wrote:
> Right. So audit userspace has this:
> 
> static const struct int_transtab elftab[] = {
>     { MACH_X86,     AUDIT_ARCH_I386   },
>     { MACH_86_64,   AUDIT_ARCH_X86_64 },
>     { MACH_IA64,    AUDIT_ARCH_IA64   },
>     { MACH_PPC64,   AUDIT_ARCH_PPC64  },
>     { MACH_PPC,     AUDIT_ARCH_PPC    },
>     { MACH_S390X,   AUDIT_ARCH_S390X  },
>     { MACH_S390,    AUDIT_ARCH_S390   },
> #ifdef WITH_ALPHA
>     { MACH_ALPHA,   AUDIT_ARCH_ALPHA  }
> #endif
> #ifdef WITH_ARMEB
>     { MACH_ARMEB,   AUDIT_ARCH_ARMEB  }
> #endif
> };

Yes -- it seems that ARMEB was confused with ARM EABI, so it's probably
worth dropping the -EB suffix in the tools and then updating the RHS of the
above table to use the little-endian identifier. This raises some questions:

(1) Why does endianness come into this? Is there some structure parsing code
    somewhere that I can't see or is there an assumption about syscall ABIs
    being necessarily different if the endianness changes?

(2) What do we do about OABI? I think the two choices are either (a) add
    some new AUDIT_ARCH_ARM* entries (although then you have the messy
    problem of determining the ABI of the current task during tracing) or
    (b) support EABI only for the time being.

I had to hack a random switch statement in the tools too, otherwise I got a
cryptic message about `requested bit level not supported by machine'.

Anyway, I threw the kernel changes into my audit branch and will re-post later
on.

Will

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Jon Masters]

On May 2, 2012, at 4:58 AM, Will Deacon wrote:

> On Wed, May 02, 2012 at 07:27:22AM +0100, Jon Masters wrote:
>> Right. So audit userspace has this:
>> 
>> static const struct int_transtab elftab[] = {
>>    { MACH_X86,     AUDIT_ARCH_I386   },
>>    { MACH_86_64,   AUDIT_ARCH_X86_64 },
>>    { MACH_IA64,    AUDIT_ARCH_IA64   },
>>    { MACH_PPC64,   AUDIT_ARCH_PPC64  },
>>    { MACH_PPC,     AUDIT_ARCH_PPC    },
>>    { MACH_S390X,   AUDIT_ARCH_S390X  },
>>    { MACH_S390,    AUDIT_ARCH_S390   },
>> #ifdef WITH_ALPHA
>>    { MACH_ALPHA,   AUDIT_ARCH_ALPHA  }
>> #endif
>> #ifdef WITH_ARMEB
>>    { MACH_ARMEB,   AUDIT_ARCH_ARMEB  }
>> #endif
>> };
> 
> Yes -- it seems that ARMEB was confused with ARM EABI, so it's probably
> worth dropping the -EB suffix in the tools and then updating the RHS of the
> above table to use the little-endian identifier. This raises some questions:
> 
> (1) Why does endianness come into this? Is there some structure parsing code
>    somewhere that I can't see or is there an assumption about syscall ABIs
>    being necessarily different if the endianness changes?

Can Eric or someone else provide context here?

> (2) What do we do about OABI? I think the two choices are either (a) add
>    some new AUDIT_ARCH_ARM* entries (although then you have the messy
>    problem of determining the ABI of the current task during tracing) or
>    (b) support EABI only for the time being.

I think the answer is?nobody cares about OABI :) Seriously though, for
"new" stuff, let's just look to the future I say. But that's my opinion.

> I had to hack a random switch statement in the tools too, otherwise I got a
> cryptic message about `requested bit level not supported by machine'.
> 
> Anyway, I threw the kernel changes into my audit branch and will re-post later
> on.

Cool. And you'll make sure stable is CC'd on this specific patch too?

Jon.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Eric Paris]

On Wed, 2012-05-02 at 10:10 -0400, Jon Masters wrote:
> On May 2, 2012, at 4:58 AM, Will Deacon wrote:
> 
> > On Wed, May 02, 2012 at 07:27:22AM +0100, Jon Masters wrote:
> >> Right. So audit userspace has this:
> >> 
> >> static const struct int_transtab elftab[] = {
> >>    { MACH_X86,     AUDIT_ARCH_I386   },
> >>    { MACH_86_64,   AUDIT_ARCH_X86_64 },
> >>    { MACH_IA64,    AUDIT_ARCH_IA64   },
> >>    { MACH_PPC64,   AUDIT_ARCH_PPC64  },
> >>    { MACH_PPC,     AUDIT_ARCH_PPC    },
> >>    { MACH_S390X,   AUDIT_ARCH_S390X  },
> >>    { MACH_S390,    AUDIT_ARCH_S390   },
> >> #ifdef WITH_ALPHA
> >>    { MACH_ALPHA,   AUDIT_ARCH_ALPHA  }
> >> #endif
> >> #ifdef WITH_ARMEB
> >>    { MACH_ARMEB,   AUDIT_ARCH_ARMEB  }
> >> #endif
> >> };
> > 
> > Yes -- it seems that ARMEB was confused with ARM EABI, so it's probably
> > worth dropping the -EB suffix in the tools and then updating the RHS of the
> > above table to use the little-endian identifier. This raises some questions:

Absolutely I will get all references s/ARMEB/ARM/g in the userspace
tools.

> > 
> > (1) Why does endianness come into this? Is there some structure parsing code
> >    somewhere that I can't see or is there an assumption about syscall ABIs
> >    being necessarily different if the endianness changes?
> 
> Can Eric or someone else provide context here?

Endianness doesn't really matter from audit's PoV.  It really cares
about ABI.  Most of the communication from kernel to userspace is
strings.  The only portion of that communiction which doesn't seem to be
a string is the netlink message type.  So as long as userspace is the
same endianness as the kernel creating the netlink message the type is
going to come out just fine.  Historically ABI has been indicated by a
mixture of 32/64bit, endianness, and the elf header e_machine field.

> > (2) What do we do about OABI? I think the two choices are either (a) add
> >    some new AUDIT_ARCH_ARM* entries (although then you have the messy
> >    problem of determining the ABI of the current task during tracing) or
> >    (b) support EABI only for the time being.
> 
> I think the answer is?nobody cares about OABI :) Seriously though, for
> "new" stuff, let's just look to the future I say. But that's my opinion.

I'm fine with not supporting things.  But I'm pretty stupid here.  Is
this just not supporting some old chip?  Or is this some ABI that a new
chip could have both and can switch at run time?  If the latter, we need
to support it.  If the former, and hints on how to make sure you can't
build audit with OABI?

> > I had to hack a random switch statement in the tools too, otherwise I got a
> > cryptic message about `requested bit level not supported by machine'.
> > 
> > Anyway, I threw the kernel changes into my audit branch and will re-post later
> > on.

Got a patch?

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Will Deacon]

On Wed, May 02, 2012 at 03:48:10PM +0100, Eric Paris wrote:
> On Wed, 2012-05-02 at 10:10 -0400, Jon Masters wrote:
> > On May 2, 2012, at 4:58 AM, Will Deacon wrote:
> > > Yes -- it seems that ARMEB was confused with ARM EABI, so it's probably
> > > worth dropping the -EB suffix in the tools and then updating the RHS of the
> > > above table to use the little-endian identifier. This raises some questions:
> 
> Absolutely I will get all references s/ARMEB/ARM/g in the userspace
> tools.

Cracking, thanks.

> Endianness doesn't really matter from audit's PoV.  It really cares
> about ABI.  Most of the communication from kernel to userspace is
> strings.  The only portion of that communiction which doesn't seem to be
> a string is the netlink message type.  So as long as userspace is the
> same endianness as the kernel creating the netlink message the type is
> going to come out just fine.  Historically ABI has been indicated by a
> mixture of 32/64bit, endianness, and the elf header e_machine field.

For ARM, we can't really use the endianness to tell us anything about the
ABI. You'll need to rely on the ELF header instead (see EF_ARM_EABI_MASK).
With that, can we get rid of the endianness checks in ptrace.c and simply
advertise AUDIT_ARCH_ARM unconditionally?

> > > (2) What do we do about OABI? I think the two choices are either (a) add
> > >    some new AUDIT_ARCH_ARM* entries (although then you have the messy
> > >    problem of determining the ABI of the current task during tracing) or
> > >    (b) support EABI only for the time being.
> > 
> > I think the answer is?nobody cares about OABI :) Seriously though, for
> > "new" stuff, let's just look to the future I say. But that's my opinion.

Well, we care enough about it not to cause regressions since some people are
still using it. I would say that audit can be EABI only though (we can add
support later for OABI if somebody shouts).

> I'm fine with not supporting things.  But I'm pretty stupid here.  Is
> this just not supporting some old chip?  Or is this some ABI that a new
> chip could have both and can switch at run time?  If the latter, we need
> to support it.  If the former, and hints on how to make sure you can't
> build audit with OABI?

My current hack in the kernel is to change the Kconfig entries for audit. As
for userspace, I guess you have to check the toolchain triplet somehow. v6
onwards makes use only of EABI and it's becoming increasingly more difficult
to find distributions supporting OABI (required for CPUs prior to v4t).

Short answer: audit can concern itself only with EABI.

> > > I had to hack a random switch statement in the tools too, otherwise I got a
> > > cryptic message about `requested bit level not supported by machine'.
> > > 
> 
> Got a patch?

Something like:

Index: lib/libaudit.c
===================================================================
--- lib/libaudit.c	(revision 693)
+++ lib/libaudit.c	(working copy)
@@ -1327,6 +1327,10 @@
 						if (bits == __AUDIT_ARCH_64BIT)
 							return -6;
 						break;
+					case MACH_ARMEB:
+						if (bits == __AUDIT_ARCH_64BIT)
+							return -6;
+						break;
 					case MACH_86_64: /* fallthrough */
 					case MACH_PPC64: /* fallthrough */
 					case MACH_S390X: /* fallthrough */

Will

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Jon Masters]

On 05/02/2012 11:39 AM, Will Deacon wrote:
> On Wed, May 02, 2012 at 03:48:10PM +0100, Eric Paris wrote:

>> I'm fine with not supporting things.  But I'm pretty stupid here.  Is
>> this just not supporting some old chip?  Or is this some ABI that a new
>> chip could have both and can switch at run time?  If the latter, we need
>> to support it.  If the former, and hints on how to make sure you can't
>> build audit with OABI?
> 
> My current hack in the kernel is to change the Kconfig entries for audit. As
> for userspace, I guess you have to check the toolchain triplet somehow. v6
> onwards makes use only of EABI and it's becoming increasingly more difficult
> to find distributions supporting OABI (required for CPUs prior to v4t).

I'll followup with an email explaining ARM ABIs to help out - might take
me a bit of time to get to it.

Jon.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Russell King - ARM Linux]

On Mon, Apr 30, 2012 at 11:07:46AM +0100, Will Deacon wrote:
> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index 80abafb..bfcadc0 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -916,14 +916,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
>  {
>         unsigned long ip;
>  
> -       /*
> -        * Save IP.  IP is used to denote syscall entry/exit:
> -        *  IP = 0 -> entry, = 1 -> exit
> -        */
> -       ip = regs->ARM_ip;
> -       regs->ARM_ip = why;
> -
> -       if (!ip)
> +       if (why)

Umm yes, that original code is complete crap, because the old IP value
has no meaning what so ever.  The replacement looks much better here.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Jon Masters]

On 04/30/2012 03:00 PM, Russell King - ARM Linux wrote:
> On Mon, Apr 30, 2012 at 11:07:46AM +0100, Will Deacon wrote:
>> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
>> index 80abafb..bfcadc0 100644
>> --- a/arch/arm/kernel/ptrace.c
>> +++ b/arch/arm/kernel/ptrace.c
>> @@ -916,14 +916,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
>>  {
>>         unsigned long ip;
>>  
>> -       /*
>> -        * Save IP.  IP is used to denote syscall entry/exit:
>> -        *  IP = 0 -> entry, = 1 -> exit
>> -        */
>> -       ip = regs->ARM_ip;
>> -       regs->ARM_ip = why;
>> -
>> -       if (!ip)
>> +       if (why)
> 
> Umm yes, that original code is complete crap, because the old IP value
> has no meaning what so ever.  The replacement looks much better here.

Hey Russell,

So given that Will's replacement works in my investigation, etc. Can you
pull that please with my reported/tested-by ACK? I think it's a stable
candidate too. I mean, ok, it won't crash your system unless you have
audit capability, but it's still a good idea to fix I think.

Jon.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Al Viro]

On Wed, May 02, 2012 at 10:59:37PM -0400, Jon Masters wrote:
> On 04/30/2012 03:00 PM, Russell King - ARM Linux wrote:
> > On Mon, Apr 30, 2012 at 11:07:46AM +0100, Will Deacon wrote:
> >> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> >> index 80abafb..bfcadc0 100644
> >> --- a/arch/arm/kernel/ptrace.c
> >> +++ b/arch/arm/kernel/ptrace.c
> >> @@ -916,14 +916,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno)
> >>  {
> >>         unsigned long ip;
> >>  
> >> -       /*
> >> -        * Save IP.  IP is used to denote syscall entry/exit:
> >> -        *  IP = 0 -> entry, = 1 -> exit
> >> -        */
> >> -       ip = regs->ARM_ip;
> >> -       regs->ARM_ip = why;
> >> -
> >> -       if (!ip)
> >> +       if (why)
> > 
> > Umm yes, that original code is complete crap, because the old IP value
> > has no meaning what so ever.  The replacement looks much better here.
> 
> Hey Russell,
> 
> So given that Will's replacement works in my investigation, etc. Can you
> pull that please with my reported/tested-by ACK? I think it's a stable
> candidate too. I mean, ok, it won't crash your system unless you have
> audit capability, but it's still a good idea to fix I think.

How about splitting the damn thing into syscall_trace_enter() and
syscall_trace_exit(), losing the "why" argument along with all possible
confusion as to which audit hook to call?

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Will Deacon]

Hi Al,

On Thu, May 03, 2012 at 04:03:12AM +0100, Al Viro wrote:
> How about splitting the damn thing into syscall_trace_enter() and
> syscall_trace_exit(), losing the "why" argument along with all possible
> confusion as to which audit hook to call?

That could certainly pan out to be slightly cleaner but I'd rather approach
it separately given that this stuff should go into the -stable trees and the
current fix is nicely self-contained.

I'll post some follow up patches.

Cheers,

Will

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Russell King - ARM Linux]

On Wed, May 02, 2012 at 10:59:37PM -0400, Jon Masters wrote:
> So given that Will's replacement works in my investigation, etc. Can you
> pull that please with my reported/tested-by ACK? I think it's a stable
> candidate too. I mean, ok, it won't crash your system unless you have
> audit capability, but it's still a good idea to fix I think.

Not only what Viro said, but... that's not how things work in any Linux
community.

1. Changes get pulled upstream only when the git tree owner is ready to
   have the changes pulled, which is indicated by them sending a pull
   request.

2. The person pulling someone elses git tree _never_ modifies commits
   which have been pulled from someone elses git tree, and that includes
   modifying them to add acks and such like.  If Will is handling the
   patch, then you need to ask Will to add your attributations.

The final point is that I've no idea until Will sends a pull request what
the git tree URL is to pull this change.

Please, wait for the proper process to happen.

[PATCH] ARM: Fix restoration of IP scratch register when auditing syscalls

[Jon Masters]

Hi will,

First, for the record, I want to note that I had an actual nightmare
about this last night. Woke up at 5am in a cold sweat with a fear that
"register 12" was out to get me (WTF?). I am *not* joking... ;)

On 04/30/2012 06:07 AM, Will Deacon wrote:

>> The fix is simply to have an additional out when not ptracing.

I retract that. The way to avoid trashing userspace is to do that. But
now that this is identified, I've spent some quality time reading the
audit code and I now even understand what it's trying to do :)

> Actually, I don't understand why we have to update pt_regs so early given
> that I don't think the saved ip is used by audit_syscall_{entry,exit} at
> all. Perhaps we could just move the ip manipulation until after the thread
> flag checks [completely untested patch below]?

Yea. There's no reason I can see to include the IP there, even for the
mach-specific macros we'll use later to pull stuff out of regs (e.g.
regs_return_value, or r0 to its friends). Your patch boots on my test
system running auditd, and more to the point - paraphrasing what Russell
said - the existing code wasn't exactly the best there, since it wants
to use "why" (set in common) and not ip as a conditional.

Ship it. Or er, I dunno, perhaps:

Reported-by: Jon Masters <jcm@jonmasters.org>
Tested-by: Jon Masters <jcm@jonmasters.org>

(or whatever else you want to shove in there for my signoff)

Jon.