i386 Kprobes semaphore fix
PATCH 19/21 i386 Kprobes semaphore fix
IA-32 linear address translation is loads of fun.
While cleaning up the LDT code, I noticed that kprobes code was very bogus with respect to segment handling. Many, many bugs are fixed here. I chose to combine the three separate functions that try to do linear address conversion into one, nice and working functions. All of the versions had bugs.
1) Taking an int3 from v8086 mode could cause the kprobes code to read a non-existent LDT.
2) The CS value was not truncated to 16 bit, which could cause an access beyond the bounds of the LDT.
3) The LDT was being read without taking the mm->context semaphore, which means bogus and or non-existent vmalloc()ed pages could be read.
4) 16-bit code segments do not truncate EIP to 16-bit, it is perfectly valid to issue an instruction at 0xffff, and there is no wraparound of EIP in protected mode.
5) V8086 mode does truncate EIP to 16-bit.
6) Taking the mm->context semaphore requires interrupts to be enabled.
7) Do not assume the GDT TLS descriptors are flat.
8) Raceful testing of segment access rights without LDT semaphore
9) Segment limit for V8086 code is USER limit.
Kprobes was still broken; it would try to read userspace directly; since I'm already here, might as well fix that too.
arch/i386/kernel/kprobes.c +25 -16
@@ -156,23 +156,25 @@ static int __kprobes kprobe_handler(strustruct kprobe *p;int ret =0 ;kprobe_opcode_t *addr =NULL ; -unsigned long *lp; +unsigned long limit; -/* We're in an interrupt, but this is clear and BUG()-safe. */ -preempt_disable (); -/* Check if the application is using LDT entry for its code segment and -* calculate the address by reading the base address from the LDT entry. +/* +* Getting the address may require getting the LDT semaphore, so +* wait to disable preempt since we may take interrupts here. */ -if ((regs->xcs &4 ) && (current->mm)) { - lp = (unsigned long *) ((unsigned long )((regs->xcs >>3 ) *8 ) - + (char *) current->mm->context.ldt); - addr = (kprobe_opcode_t *) (get_desc_base (lp) + regs->eip - -sizeof (kprobe_opcode_t)); - }else { - addr = (kprobe_opcode_t *)(regs->eip -sizeof (kprobe_opcode_t)); - } -/* Check we're not actually recursing */ -if (kprobe_running ()) { + addr = (kprobe_opcode_t *)convert_eip_to_linear (regs, + regs->eip -sizeof (kprobe_opcode_t), + ¤t->mm->context, &limit); + +/* Don't let userspace races re-address into kernel space */ +if ((unsigned long )addr > limit) +return 0 ; + +/* We're in an interrupt, but this is clear and BUG()-safe. */ +preempt_disable (); + +/* Check we're not actually recursing */ +if (kprobe_running ()) {/* We *are* holding lock here, so this is safe. Disarm the probe we just hit, and ignore it. */ p =get_kprobe (addr);
@@ -209,13 +211,20 @@ static int __kprobes kprobe_handler(strulock_kprobes (); p =get_kprobe (addr);if (!p) { +unsigned char instr;unlock_kprobes ();if (regs->eflags &VM_MASK ) {/* We are in virtual-8086 mode. Return 0 */ goto no_kprobe; } -if (*addr !=BREAKPOINT_INSTRUCTION ) { + instr =BREAKPOINT_INSTRUCTION ; +if (user_mode (regs)) +__get_user (instr, (unsigned char __user *) addr); +else + instr = *addr; + +if (instr !=BREAKPOINT_INSTRUCTION ) {/* * The breakpoint instruction was removed right * after we hit it. Another cpu has removed
arch/i386/kernel/ptrace.c +59 -29
105 lines not on this page · raw message
arch/i386/mm/fault.c +2 -72
86 lines not on this page · raw message
include/asm-i386/ptrace.h +43 -2
58 lines not on this page · raw message
Quoted 1 line · Zachary Amsden
IA-32 linear address translation is loads of fun.
Thanks for doing that audit work. Can you please double check x86-64 code is ok?
Actually giving all that complexity maybe it would be better to just stop handling the case and remove all that. I'm not sure what kprobes needs it for - it doesn't even handle user space yet and even if it ever does it is unlikely that handling 16bit code makes much sense. And the prefetch workaround does it, but 16bit DOS code is unlikely to contain prefetches anyways. And for ptrace - well, who cares? I suppose dosemu has an own debugger anyways and it could be handled in user space (i suppose they still have that code from 2.4 anyways)
Quoted 11 lines
While cleaning up the LDT code, I noticed that kprobes code was very bogus with respect to segment handling. Many, many bugs are fixed here. I chose to combine the three separate functions that try to do linear address conversion into one, nice and working functions. All of the versions had bugs.
1) Taking an int3 from v8086 mode could cause the kprobes code to read a non-existent LDT.
2) The CS value was not truncated to 16 bit, which could cause an access beyond the bounds of the LDT.
That's a (small) security hole, isn't it?
-Andi
Quoted 7 lines · Andi Kleen
Quoted 3 lines · Zachary Amsden
IA-32 linear address translation is loads of fun.
Quoted 9 lines
Thanks for doing that audit work. Can you please double check x86-64 code is ok?
Actually giving all that complexity maybe it would be better to just
stop handling the case and remove all that. I'm not sure what kprobes needs it for - it doesn't even handle user space yet and even if it ever does it is unlikely that handling 16bit code makes much sense. And the prefetch workaround does it, but 16bit DOS code is unlikely to contain prefetches anyways. And for ptrace - well, who cares? I suppose dosemu has an own debugger anyways and it could be handled in user space (i suppose
they still have that code from 2.4 anyways)
I got the idea from the x86-64 code; prompted by you, I looked at it, and it appears correct, but I would like to give it a full audit as well (especially regarding 32-bit compatibility segments).
About the three cases here:
The prefetch workaround should be harmless, again because of limit checking, the kernel is safe even in the raceful cases.
One can imagine clever uses for ptrace to do, say user space virtualization (since I'm on the topic), or other neat things. So there is nothing really wrong about having the fully correct EIP conversion (and here we shouldn't need to worry about races causing some issues with strict correctness, since there can be one external control thread).
But were kprobes even inteneded for userspace? There are races here that are difficult to close without some heavy machinery, and I would rather not put the machinery in place if simplifying the code is the right answer.
Zach
Quoted 5 lines · Zachary Amsden
One can imagine clever uses for ptrace to do, say user space virtualization (since I'm on the topic), or other neat things. So there is nothing really wrong about having the fully correct EIP conversion (and here we shouldn't need to worry about races causing some issues with strict correctness, since there can be one external control thread).
Well, the code still scaries me a bit, but ok. x86-64 left at least one case intentionally out.
Quoted 4 lines
But were kprobes even inteneded for userspace? There are races here that are difficult to close without some heavy machinery, and I would rather not put the machinery in place if simplifying the code is the right answer.
I believe user space kprobes are being worked on by some IBM India folks yes.
-Andi
Quoted 16 lines · Andi Kleen
Quoted 7 lines · Zachary Amsden
One can imagine clever uses for ptrace to do, say user space virtualization (since I'm on the topic), or other neat things. So there is nothing really wrong about having the fully correct EIP conversion (and here we shouldn't need to worry about races causing some issues with strict correctness, since there can be one external control thread).
Well, the code still scaries me a bit, but ok. x86-64 left at least one case intentionally out.
Quoted 10 lines
Quoted 6 lines
But were kprobes even inteneded for userspace? There are races here that are difficult to close without some heavy machinery, and I would rather not put the machinery in place if simplifying the code is the right answer.
I believe user space kprobes are being worked on by some IBM India folks yes.
I'm convinced this is pointless. What does it buy you over a ptrace based debugger? Why would you want extra code running in the kernel that can be done perfectly well in userspace?
Let me stress that if you are running on modified segment state, you have no way to safely determine the virtual address on which you took an instruction trap (int3, general protection, etc..). If you can't determine the virtual address safely, you can't back out your code patch to remove the breakpoint. At this point, you can't execute the next instruction; you must wait for a _policy_ decision to be made. Adding policy decisions like this to the kernel surely seems like a bad idea. If the fallback is to have a debugger running in userspace that has a user or script attached that can make the interactive decision, then why not solve the entire problem in userspace from the start? It's a lot safer, it simplifies the kernel kprobes code a lot, and it leaves the debugger implementation free to do anything it chooses, as well as not locking the solution to a particular kernel.
Zach
Quoted 6 lines · Zachary Amsden
Quoted 2 lines
I believe user space kprobes are being worked on by some IBM India folks yes.
I'm convinced this is pointless. What does it buy you over a ptrace based debugger? Why would you want extra code running in the kernel that can be done perfectly well in userspace?
kprobes are not just for 'debuggers', they are also used for tracing and other dynamic instrumentation in projects like systemtap. Ptrace is way too slow and limited for things like that.
Ingo
Quoted 4 lines · Ingo Molnar
* Zachary Amsden <[email protected]> wrote:
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
I believe user space kprobes are being worked on by some IBM India folks yes.
I'm convinced this is pointless. What does it buy you over a ptrace based debugger? Why would you want extra code running in the kernel that can be done perfectly well in userspace?
kprobes are not just for 'debuggers', they are also used for tracing and other dynamic instrumentation in projects like systemtap. Ptrace is way too slow and limited for things like that.
Well, if there is a justification for it, that means we really should handle all the nasty EIP conversion cases due to segmentation and v8086 mode in the kprobes code. I was hoping that might not be the case.
Zach
Quoted 22 lines · Zachary Amsden
Quoted 14 lines · Ingo Molnar
Quoted 9 lines · Zachary Amsden
Quoted 4 lines
I believe user space kprobes are being worked on by some IBM India folks yes.
I'm convinced this is pointless. What does it buy you over a ptrace based debugger? Why would you want extra code running in the kernel that can be done perfectly well in userspace?
Quoted 5 lines
kprobes are not just for 'debuggers', they are also used for tracing and other dynamic instrumentation in projects like systemtap. Ptrace is way too slow and limited for things like that.
Quoted 4 lines
Well, if there is a justification for it, that means we really should handle all the nasty EIP conversion cases due to segmentation and v8086 mode in the kprobes code. I was hoping that might not be the case.
As Ingo mentioned above, Systemtap uses kprobes infrastructure to provide dynamic kernel instrumentation. Using which user can add lots of probes easily, so we need to take care of this fast path.
Instead of calling convert_eip_to_linear() for all cases, you can just check if it is in kernel mode and calculate the address directly
if (kernel mode)
addr = regs->eip - sizeof(kprobe_opcode_t);
else
addr = convert_eip_to_linear(..);there by avoiding call to convert_eip_to_linear () for every kernel probes.
As Andi mentioned user space probes support is in progress and this address conversion will help in case of user space probes as well.
Thanks
Prasanna
Signature · 5 lines
Prasanna S Panchamukhi Linux Technology Center India Software Labs, IBM Bangalore Ph: 91-80-25044636 <[email protected]>
Quoted 39 lines · Prasanna S Panchamukhi
Quoted 20 lines · Zachary Amsden
Quoted 16 lines · Ingo Molnar
Quoted 10 lines · Zachary Amsden
Quoted 5 lines
I believe user space kprobes are being worked on by some IBM India folks yes.
I'm convinced this is pointless. What does it buy you over a ptrace based debugger? Why would you want extra code running in the kernel that can be done perfectly well in userspace?
Quoted 9 lines
Quoted 5 lines
kprobes are not just for 'debuggers', they are also used for tracing and other dynamic instrumentation in projects like systemtap. Ptrace is way too slow and limited for things like that.
Well, if there is a justification for it, that means we really should handle all the nasty EIP conversion cases due to segmentation and v8086 mode in the kprobes code. I was hoping that might not be the case.
As Ingo mentioned above, Systemtap uses kprobes infrastructure to provide
dynamic kernel instrumentation. Using which user can add lots of probes easily, so we need to take care of this fast path.Instead of calling
convert_eip_to_linear()for all cases, you can just check if it is in kernel mode and calculate the address directly
Quoted 9 lines
if (kernel mode) addr = regs->eip - sizeof(kprobe_opcode_t); else addr = convert_eip_to_linear(..);there by avoiding call to
convert_eip_to_linear() for every kernel probes.As Andi mentioned user space probes support is in progress and this address conversion will help in case of user space probes as well.
I like this better. I have to rework that patch anyways, since it no longer applies cleanly.
Zach
Quoted 7 lines · Prasanna S Panchamukhi
As Ingo mentioned above, Systemtap uses kprobes infrastructure to provide
dynamic kernel instrumentation. Using which user can add lots of probes easily, so we foreed to take care of this fast path.Instead of calling
convert_eip_to_linear()for all cases, you can just check if it is in kernel mode and calculate the address directly
Quoted 9 lines
if (kernel mode) addr = regs->eip - sizeof(kprobe_opcode_t); else addr = convert_eip_to_linear(..);there by avoiding call to
convert_eip_to_linear() for every kernel probes.As Andi mentioned user space probes support is in progress and this address conversion will help in case of user space probes as well.
Would it make sense for this to be part of convert_eip_to_linear?
-hpa
Quoted 3 lines · Zachary Amsden
Well, if there is a justification for it, that means we really should handle all the nasty EIP conversion cases due to segmentation and v8086 mode in the kprobes code. I was hoping that might not be the case.
Or just forbid kprobes for 16bit processes (or anything running in a non GDT code segment). Would be perfectly reasonable IMHO.
-Andi
Quoted 16 lines · Zachary Amsden
Quoted 15 lines · Andi Kleen
Quoted 5 lines · Zachary Amsden
One can imagine clever uses for ptrace to do, say user space virtualization (since I'm on the topic), or other neat things. So there is nothing really wrong about having the fully correct EIP conversion (and here we shouldn't need to worry about races causing some issues with strict correctness, since there can be one external control thread).
Well, the code still scaries me a bit, but ok. x86-64 left at least one case intentionally out.
Quoted 4 lines
But were kprobes even inteneded for userspace? There are races here that are difficult to close without some heavy machinery, and I would rather not put the machinery in place if simplifying the code is the right answer.
Quoted 6 lines
Quoted 2 lines
I believe user space kprobes are being worked on by some IBM India folks yes.
I'm convinced this is pointless. What does it buy you over a ptrace based debugger? Why would you want extra code running in the kernel that can be done perfectly well in userspace?
People might want to do something like "attach trace point to glibc function X" for all processes on the system. Attaching ptrace to everything would cause large overhead. systemtap really handles a different need - of just low overhead tracing, not heavy weight debugging.
Quoted 5 lines
Let me stress that if you are running on modified segment state, you have no way to safely determine the virtual address on which you took an instruction trap (int3, general protection, etc..). If you can't determine the virtual address safely, you can't back out your code patch to remove the breakpoint. At this point, you can't execute the next
Kernel kprobes solves this by executing the code out of line. I don't know how they want to do that in user space though (need a safe address for that), but somehow that can be likely done.
Quoted 5 lines
instruction; you must wait for a _policy_ decision to be made. Adding policy decisions like this to the kernel surely seems like a bad idea. If the fallback is to have a debugger running in userspace that has a user or script attached that can make the interactive decision, then why not solve the entire problem in userspace from the start? It's a lot
Doing it in user space would make it hard to do global tracing, and it also likely would have much higher overhead.
-Andi
Quoted 9 lines
Quoted 5 lines
Let me stress that if you are running on modified segment state, you have no way to safely determine the virtual address on which you took an instruction trap (int3, general protection, etc..). If you can't determine the virtual address safely, you can't back out your code patch to remove the breakpoint. At this point, you can't execute the next
Kernel kprobes solves this by executing the code out of line. I don't know how they want to do that in user space though (need a safe address for that), but somehow that can be likely done.
In case of user space probes we adopt a similar method for executing the code out-of-line. In user space probes we find free space in the current
process address space and copy the original instruction to that location and execute that instruction from that location. User processes use stack space to store local variables, agruments and return values. Normally the stack space either below or above the stack pointer indicates the free stack space.
Also in case of no stack free space, we can expand the process stack, copy the instruction and execute the instruction from that location.
Detials about this method is discussed on systemtap mailing lists. URL is below.
http://sourceware.org/ml/systemtap/2005-q3/msg00542.html
Please let me know if you have any other solution to the above problem.
Thanks
Prasanna
Signature · 5 lines
Prasanna S Panchamukhi Linux Technology Center India Software Labs, IBM Bangalore Email: [email protected] Ph: 91-80-25044636
A message that is not in the archive (sent off-list or not yet received): [email protected]
resending this mail, since my earlier email did not reach lkml.
Quoted 70 lines · Prasanna S Panchamukhi
Zach,
Thanks for doing this.
Quoted 9 lines · Zachary Amsden
Quoted 7 lines · Andi Kleen
Quoted 3 lines · Zachary Amsden
IA-32 linear address translation is loads of fun.
Quoted 8 lines
Quoted 8 lines
Thanks for doing that audit work. Can you please double check x86-64 code is
ok?Actually giving all that complexity maybe it would be better to just stop handling the case and remove all that. I'm not sure what kprobes needs it for - it doesn't even handle user space yet and even if it ever does it is unlikely that handling 16bit code makes much sense. And the
The code was added to address the problem related to stealing of interrupts from VM86. Please see the discussion thread for more details from the URL below
http://lkml.org/lkml/2004/11/9/214Quoted 4 lines
But were kprobes even inteneded for userspace? There are races here that are difficult to close without some heavy machinery, and I would rather not put the machinery in place if simplifying the code is the right answer.
Presently kprobes supports only kernel space probes. Work is in progress for user space probes support.
Review
Quoted 5 lines
(quoted diff) +2
-0+ addr = (kprobe_opcode_t *)convert_eip_to_linear(regs, + regs->eip -
sizeof(kprobe_opcode_t),(quoted diff) +2
-0+ ¤t->mm->context, &limit); +Instead you can check if it is in kernel mode and calculate the address directly first, since it is in the fast path.
addr = regs->eip - sizeof(kprobe_opcode_t); else addr = convert_eip_to_linear(..);there by avoiding calling
convert_eip_to_linear() in case of every kernel probes.arch/i386/kernel/kprobes.c static int __kprobes kprobe_handler(stru · line 171
Quoted 3 lines
arch/i386/kernel/kprobes.c +3
-0+/* Don't let userspace races re-address into kernel space */ +if ((unsigned long )addr > limit) +return 0 ;there is no need for this check here in the fast path, because kprobes handles this case by checking if the address is on the kprobes hash list and later returning from that point.
Please make sure it pass the test case discussed in the thread, URL is below.
http://lkml.org/lkml/2004/11/9/214Thanks
-Prasanna
--
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<[email protected]>
Signature · 8 lines
Have a Nice Day! Thanks & Regards Prasanna S Panchamukhi Linux Technology Center India Software Labs, IBM Bangalore Ph: 91-80-25044636 <[email protected]>