linux-kernel - Re: [RFC PATCH] ptrace: add PTRACE_GET_SYSCALL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Nov 2018 06:06:43 -0800
From:   Andy Lutomirski <luto@...capital.net>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Elvira Khabirova <lineprinter@...linux.org>, rostedt@...dmis.org,
        mingo@...hat.com, linux-kernel@...r.kernel.org, ldv@...linux.org,
        esyr@...hat.com, luto@...nel.org, strace-devel@...ts.strace.io
Subject: Re: [RFC PATCH] ptrace: add PTRACE_GET_SYSCALL_INFO request



> On Nov 7, 2018, at 3:21 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> 
>> On 11/07, Elvira Khabirova wrote:
>> 
>> In short, if a 64-bit task performs a syscall through int 0x80, its tracer
>> has no reliable means to find out that the syscall was, in fact,
>> a compat syscall, and misidentifies it.
>> * Syscall-enter-stop and syscall-exit-stop look the same for the tracer.
> 
> Yes, this was discussed many times...
> 
> So perhaps it makes sense to encode compat/is_enter in ->ptrace_message,
> debugger can use PTRACE_GETEVENTMSG to get this info.

As I said before, I strongly object to the use of “compat” here.  Compat meant “not the kernel’s native syscall API — uses the 32-bit structure format instead”.  This does not have a sensible meaning to user code, especially in the case where the tracer is 32-bit.

> 
>> Secondly, ptracers also have to support a lot of arch-specific code for
>> obtaining information about the tracee. For some architectures, this
>> requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
>> argument and return value.
> 
> I am not sure about this change... I won't really argue, but imo this
> needs a separate patch.

Why?  Having a single struct that the tracer can read to get the full state is extremely helpful.

Also, we really want it to work for seccomp events as well as PTRACE_SYSCALL, and the event info trick doesn’t make sense for seccomp events.

> 
>> +#define PT_IN_SYSCALL_STOP    0x00000004    /* task is in a syscall-stop */
> ...
>> -static inline int ptrace_report_syscall(struct pt_regs *regs)
>> +static inline int ptrace_report_syscall(struct pt_regs *regs,
>> +                    unsigned long message)
>> {
>>    int ptrace = current->ptrace;
>> 
>>    if (!(ptrace & PT_PTRACED))
>>        return 0;
>> +    current->ptrace |= PT_IN_SYSCALL_STOP;
>> 
>> +    current->ptrace_message = message;
>>    ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0));
>> 
>>    /*
>> @@ -76,6 +79,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
>>        current->exit_code = 0;
>>    }
>> 
>> +    current->ptrace &= ~PT_IN_SYSCALL_STOP;
>>    return fatal_signal_pending(current);
> ...
> 
>> +    case PTRACE_GET_SYSCALL_INFO:
>> +        if (child->ptrace & PT_IN_SYSCALL_STOP)
>> +            ret = ptrace_get_syscall(child, datavp);
>> +        break;
> 
> Why? If debugger uses PTRACE_O_TRACESYSGOOD it can know if the tracee reported
> syscall entry/exit or not. PTRACE_GET_SYSCALL_INFO is pointless if not, but
> nothing bad can happen.
> 
> 

I think it’s considerably nicer to the user to avoid reporting garbage if the user misused the API.  (And Elvira got this right in the patch — I just missed it.)

>