From: Bodo S. <bst...@fu...> - 2004-10-21 15:20:46
|
If a process in UML does a systemcall with the systemcall number being less than 0, in TT-mode and SKAS-mode using SYSEMU, the process is killed by an SIGTRAP instead of simply returning -ENOSYS. In SKAS-mode without SYSEMU, UML even crashes, no matter if SYSEMU is unsupported by the host or switched off in UML: Kernel panic - not syncing: handle_trap - failed to wait at end of syscall, errno = 4, status = 2943 The reason is, that UML can't distinguish between a debugger trap and an systemcall interception. Currently, it checks the systemcall number. If it is less than 0, it assumes the event to be a debugger trap. It would be better to assume a debugger trap only, if the syscall number is -1 (which it is guaranteed to be in case of a debugger event), but even then UML wouldn't be safe. Syscalls with syscall number -1 still would be a problem! AFAICS, the only solution for this is using the PTRACE_O_TRACESYSGOOD option. This option seems to be specific for linux, but the problem maybe is specific for linux, too. So, here attached are three patches. The first adds a check for availability and function of ptrace(PTRACE_SETOPTIONS,,,PTRACE_O_TRACESYSGOOD) to the normal ptrace checks. The second implements the usage of the option in SKAS-mode. The third does the same for TT-mode. For the third patch I'm quite anxious, that there could go something wrong when using the debugger. I don't understand much about this. Maybe someone else could look into this? Regards Bodo |
From: BlaisorBlade <bla...@ya...> - 2004-10-21 17:57:56
|
On Thursday 21 October 2004 17:25, Bodo Stroesser wrote: > If a process in UML does a systemcall with the systemcall number > being less than 0, in TT-mode and SKAS-mode using SYSEMU, the > process is killed by an SIGTRAP instead of simply returning -ENOSYS. > In SKAS-mode without SYSEMU, UML even crashes, no matter if SYSEMU > is unsupported by the host or switched off in UML: > Kernel panic - not syncing: handle_trap - failed to wait at end of > syscall, errno = 4, status = 2943 > The reason is, that UML can't distinguish between a debugger trap > and an systemcall interception. Currently, it checks the systemcall > number. If it is less than 0, it assumes the event to be a debugger > trap. It would be better to assume a debugger trap only, if the > syscall number is -1 (which it is guaranteed to be in case of a > debugger event), but even then UML wouldn't be safe. Syscalls with > syscall number -1 still would be a problem! > AFAICS, the only solution for this is using the PTRACE_O_TRACESYSGOOD > option. This option seems to be specific for linux, but the problem > maybe is specific for linux, too. I have been thinking to using SYSGOOD, but without finding a good reason for that... so thanks for this. The first remark (the only I have for now) is that you should replace "SIGTRAP + 0x80" with "SIGTRAP | 0x80". It should not make a difference in this particular case, but it's a style issue, which exists because the second way is more robust for setting bits: think about (SIGTRAP & 0x80) == 0x80 and using the + 0x80: it clears the bit it should set and set another one. Remark no. 2: could you, please, in next patches, try to add -p to diff flags to improve readability? That makes clear which function is being changed, so the patch becomes more readable. > So, here attached are three patches. The first adds a check for > availability and function of > ptrace(PTRACE_SETOPTIONS,,,PTRACE_O_TRACESYSGOOD) to the normal > ptrace checks. > > The second implements the usage of the option in SKAS-mode. > The third does the same for TT-mode. > For the third patch I'm quite anxious, that there could go something > wrong when using the debugger. I don't understand much about this. > Maybe someone else could look into this? I'll give a look when I have the needed time. However, could you explain what makes you worry in detail? > Regards > Bodo -- Paolo Giarrusso, aka Blaisorblade Linux registered user n. 292729 |
From: Bodo S. <bst...@fu...> - 2004-10-22 07:27:58
|
BlaisorBlade wrote: > > The first remark (the only I have for now) is that you should replace "SIGTRAP > + 0x80" with "SIGTRAP | 0x80". It should not make a difference in this > particular case, but it's a style issue, which exists because the second way > is more robust for setting bits: think about (SIGTRAP & 0x80) == 0x80 and > using the + 0x80: it clears the bit it should set and set another one. Yes. I agree. > > Remark no. 2: could you, please, in next patches, try to add -p to diff flags > to improve readability? That makes clear which function is being changed, so > the patch becomes more readable. I will do so. > >>For the third patch I'm quite anxious, that there could go something >>wrong when using the debugger. I don't understand much about this. >>Maybe someone else could look into this? > > I'll give a look when I have the needed time. However, could you explain what > makes you worry in detail? I do not understand, how the debugger works. I didn't even use it, yet. Maybe it would be a good idea to learn while testing. I tried to mask the additional 0x80 when the debugger is called (i.e. "(status&0x7fff)"). But I don't know, whether this is the only place, where a special handling has to be inserted to let the debugger see no changes. Bodo |
From: Jeff D. <jd...@ad...> - 2004-10-21 20:59:42
|
bst...@fu... said: > AFAICS, the only solution for this is using the PTRACE_O_TRACESYSGOOD > option. This option seems to be specific for linux, but the problem > maybe is specific for linux, too. Another solution is to read the instruction. However TRACESYSGOOD is cleaner. My only concern is whether that limits the hosts that UML will run on. If TRACESYSGOOD has been around for the 2.4 series, then that's OK. I would be tempted to #define SIGSYSCALL (SIGTRAP + 0x80) or something just to make the +0x80 bit less magic-looking. > For the third patch I'm quite anxious, that there could go something > wrong when using the debugger. I don't understand much about this. > Maybe someone else could look into this? if(!tracing && (debugger_pid != -1)){ child_signal(pid, status); continue; } tracing = 0; if(do_syscall(task, pid)) sig = SIGUSR2; OK, what this says is if we are running in the kernel, not userspace (!tracing) and we are debugging UML (debugger_pid != -1) then we fake the syscall-traced gdb into thinking the SIGTRAP was sent to the process Otherwise we call do_syscall, which will check the syscall number and return true if it thinks it's handling a syscall. In that case, we hit the process with SUGUSR2 in order to force it into the UML syscall handler. So, if we get SIGTRAP + 0x80, we know we have a syscall, and the debugging stuff can just go away. You can look at this sort of mathematically, and say SIGTRAP + 0x80 => tracing, so that if(!tracing...) can just be deleted. Similarly, do_syscall will always return true, so you don't need to check its return, which you have removed. In the SIGTRAP case, you might as well inline do_sigtrap. One line functions are pretty much a waste. In any case, I don't understand why you're doing that. orig_eax should already be -1. Jeff |
From: BlaisorBlade <bla...@ya...> - 2004-10-21 22:32:20
|
On Friday 22 October 2004 00:09, Jeff Dike wrote: > bst...@fu... said: > > AFAICS, the only solution for this is using the PTRACE_O_TRACESYSGOOD > > option. This option seems to be specific for linux, but the problem > > maybe is specific for linux, too. > > Another solution is to read the instruction. > > However TRACESYSGOOD is cleaner. My only concern is whether that limits > the hosts that UML will run on. If TRACESYSGOOD has been around for the > 2.4 series, then that's OK. > > I would be tempted to #define SIGSYSCALL (SIGTRAP + 0x80) or something just > to make the +0x80 bit less magic-looking. It should be | 0x80, not + 0x80 (style issue). See the code in do_syscall_trace. -- Paolo Giarrusso, aka Blaisorblade Linux registered user n. 292729 |
From: Jeff D. <jd...@ad...> - 2004-10-22 03:04:53
|
bla...@ya... said: > It should be | 0x80, not + 0x80 (style issue). See the code in > do_syscall_trace. Right, I was using his style just to not confuse things. Jeff |
From: Bodo S. <bst...@fu...> - 2004-10-22 08:09:10
|
BlaisorBlade wrote: > On Friday 22 October 2004 00:09, Jeff Dike wrote: > >>bst...@fu... said: >> >>>AFAICS, the only solution for this is using the PTRACE_O_TRACESYSGOOD >>>option. This option seems to be specific for linux, but the problem >>>maybe is specific for linux, too. >> >>Another solution is to read the instruction. >> >>However TRACESYSGOOD is cleaner. My only concern is whether that limits >>the hosts that UML will run on. If TRACESYSGOOD has been around for the >>2.4 series, then that's OK. >> >>I would be tempted to #define SIGSYSCALL (SIGTRAP + 0x80) or something just >>to make the +0x80 bit less magic-looking. > > > It should be | 0x80, not + 0x80 (style issue). See the code in > do_syscall_trace. > Yes. Should I submit new patches, or will you do the changes yourself? Bodo |
From: Bodo S. <bst...@fu...> - 2004-10-22 08:07:04
|
Jeff Dike wrote: > bst...@fu... said: > >>AFAICS, the only solution for this is using the PTRACE_O_TRACESYSGOOD >>option. This option seems to be specific for linux, but the problem >>maybe is specific for linux, too. > > > Another solution is to read the instruction. It's a bit tricky to do this, if the vsyscall-page is in use. Then you also have to know the address of the return-point for "sysenter", which the kernel uses. Also, the tracer thread at the moment has no access to the code of the process! I tapped into this trap while trying to simplify the singlestepping even more by moving the opcode-check from kernel_do_signal() to is_syscall(). So I reverted the change ... > > However TRACESYSGOOD is cleaner. My only concern is whether that limits the > hosts that UML will run on. If TRACESYSGOOD has been around for the 2.4 > series, then that's OK. 2.4 supports it, at least in the newer versions. I looked for it before patching. > > I would be tempted to #define SIGSYSCALL (SIGTRAP + 0x80) or something just > to make the +0x80 bit less magic-looking. Yes. I used the 0x80, because I didn't find a definition for this in the header files from /usr/include. So, if the host linux doesn't use a #define THIS_IS_A_SYSCALL_INTERCEPTION 0x80 I decided not to do it also. But feel free to change this. > > >>For the third patch I'm quite anxious, that there could go something >>wrong when using the debugger. I don't understand much about this. >>Maybe someone else could look into this? > > > if(!tracing && (debugger_pid != -1)){ > child_signal(pid, status); > continue; > } > tracing = 0; > if(do_syscall(task, pid)) > sig = SIGUSR2; > > OK, what this says is > if we are running in the kernel, not userspace (!tracing) > and we are debugging UML (debugger_pid != -1) > then we fake the syscall-traced gdb into thinking the SIGTRAP was sent > to the process > > Otherwise we call do_syscall, which will check the syscall number > and return true if it thinks it's handling a syscall. In that case, > we hit the process with SUGUSR2 in order to force it into the UML > syscall handler. > > So, if we get SIGTRAP + 0x80, we know we have a syscall, and the debugging > stuff can just go away. You can look at this sort of mathematically, and > say SIGTRAP + 0x80 => tracing, so that if(!tracing...) can just be deleted. No. The processes in UML-TT, no matter if they run in userspace or execute the kernel, always are ptraced processes and PTRACE_O_TRACESSYSGOOD now will be set permanently. So, if the debugger does an syscall-trace, it will expect an SIGTRAP, but what happens is an (SIGTRAP|0x80). Since I didn't want to change the debugger, I masked the 0x80. For breakpoints or singlestep-traps, the signal still will be SIGTRAP only. Maybe it would make sense, to change the debugger-code. It could accept (SIGTRAP|0x80) and give it to the real debugger as SIGTRAP normally. Also, it could accept a ptrace(PTRACE_SETOTIONS,,,PTRACE_O_TRACESYSGOOD) and after this it could relay the (SIGTRAP|0x80) without change. > > Similarly, do_syscall will always return true, so you don't need to check its > return, which you have removed. > > In the SIGTRAP case, you might as well inline do_sigtrap. One line functions > are pretty much a waste. In any case, I don't understand why you're doing > that. orig_eax should already be -1. Yes. Inlining is a good idea. You are right, orig_eax on the processes stack is -1. But this *has* to be saved in the tt-regs structure for later use. What I'm doing is the same that UPT_SYSCALL_NR(TASK_REGS(task)) = PT_SYSCALL_NR(proc_regs); does in the case of a systemcall. I used "-1" instead of "PT_SYSCALL_NR(proc_regs)" because it *is* the same in this situation. > > Jeff > |
From: Jeff D. <jd...@ad...> - 2004-10-22 19:52:41
|
bst...@fu... said: > Maybe it would make sense, to change the debugger-code. It could > accept (SIGTRAP|0x80) and give it to the real debugger as SIGTRAP > normally. Also, it could accept a ptrace(PTRACE_SETOTIONS,,,PTRACE_O_TR > ACESYSGOOD) and after this it could relay the (SIGTRAP|0x80) without > change. Yeah, it would, but what you have now will work for the time being. > You are right, orig_eax on the processes stack is -1. But this *has* > to be saved in the tt-regs structure for later use. What I'm doing is > the same that > UPT_SYSCALL_NR(TASK_REGS(task)) = PT_SYSCALL_NR(proc_regs); does > in the case of a systemcall. I used "-1" instead of > "PT_SYSCALL_NR(proc_regs)" because it *is* the same in this situation. Right, good point. Jeff |
From: Bodo S. <bst...@fu...> - 2004-10-25 14:41:46
|
Jeff Dike wrote: > > I would be tempted to #define SIGSYSCALL (SIGTRAP + 0x80) or something just > to make the +0x80 bit less magic-looking. O.K. I defined it in signal_user.h, hope it's OK there. Consequently the definition is used in arch/um/kernel/ptrace.c to replace the 0x80 at the call to ptrace_notify(). > > In the SIGTRAP case, you might as well inline do_sigtrap. One line functions > are pretty much a waste. Sorry, I tried to inline it, but it failed to compile. At the moment tracer.c contains functions without much knowledge about task structure only. Without including the specific headers, inlining cannot be used. So, attached you'll find the revised patches. Bodo |
From: Bodo S. <bst...@fu...> - 2004-10-25 15:13:28
Attachments:
patch-TRACESYSGOOD-1
|
Bodo Stroesser wrote: > Jeff Dike wrote: > >> >> I would be tempted to #define SIGSYSCALL (SIGTRAP + 0x80) or something >> just >> to make the +0x80 bit less magic-looking. > > O.K. I defined it in signal_user.h, hope it's OK there. Consequently the > definition is used in arch/um/kernel/ptrace.c to replace the 0x80 at the > call to ptrace_notify(). > >> >> In the SIGTRAP case, you might as well inline do_sigtrap. One line >> functions >> are pretty much a waste. > > Sorry, I tried to inline it, but it failed to compile. At the moment > tracer.c > contains functions without much knowledge about task structure only. > Without > including the specific headers, inlining cannot be used. > > So, attached you'll find the revised patches. > > Bodo Sorry, I missed one change. There still 0x80 is used in the first patch. Thus, attached the final version (hopefully ...) Bodo |