Fixing audit on ARM
Subsystems (2)
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
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.
--- arch/arm/kernel/ptrace.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)
arch/arm/kernel/ptrace.c +7 -3
@@ -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; }
Signature · 1 line
1.7.7.4
PATCH 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Hi Jon,
Quoted 11 lines · 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.
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.
Quoted 1 line
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]?
arch/arm/kernel/ptrace.c +8 -8
@@ -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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 15 lines · Will Deacon
Hi Jon,
Quoted 11 lines · 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.
Quoted 5 lines
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 7 lines · Jon Masters
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 10 lines · Will Deacon
Quoted 7 lines · Jon Masters
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
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!
Quoted 11 lines · Russell King - ARM Linux
Quoted 10 lines · Will Deacon
Quoted 7 lines · Jon Masters
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>.
Quoted 7 lines
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.
Quoted 1 line
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
In reply to Will DeaconQuoted 10 lines · Will Deacon
Quoted 7 lines · Jon Masters
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 :)
Quoted 8 lines
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 15 lines · Jon Masters
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 }
Quoted 2 lines
#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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 19 lines · Will Deacon
Quoted 15 lines · Jon Masters
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 }Quoted 2 lines
#endif
};
Quoted 7 lines
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?
Quoted 4 lines
(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.
Quoted 5 lines
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 21 lines · Jon Masters
Quoted 19 lines · Will Deacon
Quoted 17 lines · Jon Masters
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
};
Quoted 3 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 4 lines
(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.
Quoted 7 lines
Quoted 4 lines
(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?
Quoted 5 lines
Quoted 5 lines
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 8 lines · Eric Paris
Quoted 4 lines · Jon Masters
Quoted 3 lines · Will Deacon
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.
Quoted 7 lines
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_machinefield.
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?
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
(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).
Quoted 5 lines
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.
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
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:
libaudit.c +4 -0
@@ -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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 8 lines · Will Deacon
Quoted 5 lines · Eric Paris
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?
Quoted 4 lines
My current hack in the kernel is to change the
Kconfigentries 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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
In reply to Will DeaconQuoted 7 lines · Will Deacon
arch/arm/kernel/ptrace.c +0
-0@@ -916,14 +916,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno) {unsigned long ip;
arch/arm/kernel/ptrace.c asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno) · line 919
Quoted 10 lines
arch/arm/kernel/ptrace.c +1
-8-/* -* 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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 9 lines · Russell King - ARM Linux
Quoted 8 lines · Will Deacon
arch/arm/kernel/ptrace.c +0
-0@@ -916,14 +916,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno) {unsigned long ip;
Quoted 12 lines
arch/arm/kernel/ptrace.c asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno) · line 919
Quoted 9 lines
arch/arm/kernel/ptrace.c +1
-8-/* -* 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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Quoted 23 lines · Jon Masters
Quoted 9 lines · Russell King - ARM Linux
Quoted 8 lines · Will Deacon
arch/arm/kernel/ptrace.c +0
-0@@ -916,14 +916,7 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno) {unsigned long ip;Quoted 12 lines
arch/arm/kernel/ptrace.c asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno) · line 919
Quoted 9 lines
arch/arm/kernel/ptrace.c +1
-8-/* -* 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.
Quoted 6 lines
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
Hi Al,
Quoted 3 lines · Al Viro
How about splitting the damn thing into
syscall_trace_enter()andsyscall_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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
In reply to Jon MastersQuoted 4 lines · Jon Masters
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 1/1 ARM: Fix restoration of IP scratch register when auditing syscalls
In reply to Will DeaconHi 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... ;)
Quoted 1 line · Will Deacon
Quoted 1 line
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 :)
Quoted 4 lines
Actually, I don't understand why we have to update
pt_regsso 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:
(or whatever else you want to shove in there for my signoff)
Jon.