Bug in the syscall tracing code

Bug in the syscall tracing code

[Gleb O. Raiko]

Hello,

The story continues. The last fix of the syscall tracing code was wrong, 
unfortunately. (The bug was a user could invoke any function in the 
kernel. The fix was not to use t2 as pointer to a syscall, s0 was chosen 
for it.) The problem we discovered is a few syscalls do SAVE_STATIC 
(those declared as save_static_function), so s0 (which holds pointer to 
the syscall at the time the syscall is invoked) is saved on the stack 
overwriting a value saved from the process being traced. No wonder, s0 
that restored on syscall exit differs from s0 saved on syscall enter.

See, arch/mips/kernel/scall32-o32.S, syscall_trace_entry, for example. 
The rest of ABIs are the same.

There are several ways to fix this:

1. Make syscall handling code to be close to other arches. I mean, check 
for the trace flag first, then parse arguments and invoke a syscall.

2. Remove save_static_functions and do SAVE_STATIC early for several 
syscalls (yes, one big switch or its asm equivalent).

3. Store t2 in pt_regs (it means we have to expand this structure).

4. I know there should be yet another way.

Any ideas ?

Regards,
Gleb.

Re: Bug in the syscall tracing code

[Thiemo Seufer]

Gleb O. Raiko wrote:
> Hello,
> 
> The story continues. The last fix of the syscall tracing code was wrong, 
> unfortunately. (The bug was a user could invoke any function in the 
> kernel. The fix was not to use t2 as pointer to a syscall, s0 was chosen 
> for it.) The problem we discovered is a few syscalls do SAVE_STATIC 
> (those declared as save_static_function), so s0 (which holds pointer to 
> the syscall at the time the syscall is invoked) is saved on the stack 
> overwriting a value saved from the process being traced. No wonder, s0 
> that restored on syscall exit differs from s0 saved on syscall enter.
> 
> See, arch/mips/kernel/scall32-o32.S, syscall_trace_entry, for example. 
> The rest of ABIs are the same.
> 
> There are several ways to fix this:
> 
> 1. Make syscall handling code to be close to other arches. I mean, check 
> for the trace flag first, then parse arguments and invoke a syscall.
> 
> 2. Remove save_static_functions and do SAVE_STATIC early for several 
> syscalls (yes, one big switch or its asm equivalent).
> 
> 3. Store t2 in pt_regs (it means we have to expand this structure).
> 
> 4. I know there should be yet another way.

- Use the k1 slot instead of s0 to save the function pointer.


Thiemo

Re: Bug in the syscall tracing code

[David Daney]

Thiemo Seufer wrote:
> Gleb O. Raiko wrote:
> 
>>Hello,
>>
>>The story continues. The last fix of the syscall tracing code was wrong, 
>>unfortunately. (The bug was a user could invoke any function in the 
>>kernel. The fix was not to use t2 as pointer to a syscall, s0 was chosen 
>>for it.) The problem we discovered is a few syscalls do SAVE_STATIC 
>>(those declared as save_static_function), so s0 (which holds pointer to 
>>the syscall at the time the syscall is invoked) is saved on the stack 
>>overwriting a value saved from the process being traced. No wonder, s0 
>>that restored on syscall exit differs from s0 saved on syscall enter.
>>
>>See, arch/mips/kernel/scall32-o32.S, syscall_trace_entry, for example. 
>>The rest of ABIs are the same.
>>
>>There are several ways to fix this:
>>
>>1. Make syscall handling code to be close to other arches. I mean, check 
>>for the trace flag first, then parse arguments and invoke a syscall.
>>
>>2. Remove save_static_functions and do SAVE_STATIC early for several 
>>syscalls (yes, one big switch or its asm equivalent).
>>
>>3. Store t2 in pt_regs (it means we have to expand this structure).
>>
>>4. I know there should be yet another way.
> 
> 
> - Use the k1 slot instead of s0 to save the function pointer.
> 
That is the conclusion I came to in:

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=4207C3E0.7070405%40avtrex.com

IIRC, k0 is already used for something.

David daney.

Re: Bug in the syscall tracing code

[Gleb O. Raiko]

David Daney wrote:
> That is the conclusion I came to in:
> 
> http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=4207C3E0.7070405%40avtrex.com 

Saving in the PT_SCRATCH area (pad0 in C) was a solution for 2.4. 
Unfortunately, syscall arguments are stored there (and that's why pad0 
exists in pt_regs after all). So, using PT_SCRATCH as a temporary 
storage for t2 will break tracing syscalls with more than 4 args for o32 
ABI.

Regards,
Gleb.

Re: Bug in the syscall tracing code

[David Daney]

Gleb O. Raiko wrote:
> David Daney wrote:
> 
>> That is the conclusion I came to in:
>>
>> http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=4207C3E0.7070405%40avtrex.com 
> 
> 
> 
> Saving in the PT_SCRATCH area (pad0 in C) was a solution for 2.4. 
> Unfortunately, syscall arguments are stored there (and that's why pad0 
> exists in pt_regs after all). So, using PT_SCRATCH as a temporary 
> storage for t2 will break tracing syscalls with more than 4 args for o32 
> ABI.

I know.  I meant for you to look at the very end of the message (The 
part where I said to store it in the slots for k0 or k1).

David Daney.

Re: Bug in the syscall tracing code

[Gleb O. Raiko]

Thiemo Seufer wrote:
> - Use the k1 slot instead of s0 to save the function pointer.

Unfortunately, k0, k1 cannot be used. We shall withstand 
do_syscall_trace. It implies going to the user mode and back.

Regards,
Gleb.

Re: Bug in the syscall tracing code

[Gleb O. Raiko]

Hello,

> 4. I know there should be yet another way.

The way is to load a saved register in the delay slot of jalr. The saved 
register shall not be s0, of course, because it's saved by the first 
instruction in save_static_function. So the proposed patch is

arch/mips/kernel/scall32-o32.S:

syscall_trace_entry:
         SAVE_STATIC
-	move	s0, t2
+	move	s1, t2
         move    a0, sp
         li      a1, 0
         jal     do_syscall_trace

         lw      a0, PT_R4(sp)           # Restore argument registers
         lw      a1, PT_R5(sp)
         lw      a2, PT_R6(sp)
         lw      a3, PT_R7(sp)
-        jalr	s0
+	.set push
+	.set noreorder
+	jalr	s1
+	 lw	s1, PT_R17(sp)
+	.set pop

The rest of ABIs shall be implemented in the same way.

Regards,
Gleb.