Re: [PATCH v2 6/6] arm64: ptrace: add support for syscall emulation
On 2019/3/18 18:49, Sudeep Holla wrote:
> Add PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP support on arm64.
> We can just make sure of the generic ptrace_syscall_enter hook to
> support PTRACE_SYSEMU. We don't need any special handling for
> PTRACE_SYSEMU_SINGLESTEP.
This looks good to me. But it'd be better to add the same logic to handle
PTRACE_SYSEMU_SINGLESTEP as that of x86 in case we may need enable the single
step trace function in the future.
>
> Cc: Catalin Marinas
> Cc: Will Deacon
> Signed-off-by: Sudeep Holla
> ---
> arch/arm64/include/asm/thread_info.h | 5 -
> arch/arm64/kernel/ptrace.c | 3 +++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/thread_info.h
> b/arch/arm64/include/asm/thread_info.h
> index eb3ef73e07cf..c285d1ce7186 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -75,6 +75,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> * TIF_SYSCALL_TRACE- syscall trace active
> * TIF_SYSCALL_TRACEPOINT - syscall tracepoint for ftrace
> * TIF_SYSCALL_AUDIT- syscall auditing
> + * TIF_SYSCALL_EMU - syscall emulation active
> * TIF_SECOMP- syscall secure computing
> * TIF_SIGPENDING- signal pending
> * TIF_NEED_RESCHED- rescheduling necessary
> @@ -91,6 +92,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define TIF_SYSCALL_AUDIT9
> #define TIF_SYSCALL_TRACEPOINT10
> #define TIF_SECCOMP11
> +#define TIF_SYSCALL_EMU12
> #define TIF_MEMDIE18/* is terminating due to OOM killer */
> #define TIF_FREEZE19
> #define TIF_RESTORE_SIGMASK20
> @@ -109,6 +111,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define _TIF_SYSCALL_AUDIT(1 << TIF_SYSCALL_AUDIT)
> #define _TIF_SYSCALL_TRACEPOINT(1 << TIF_SYSCALL_TRACEPOINT)
> #define _TIF_SECCOMP(1 << TIF_SECCOMP)
> +#define _TIF_SYSCALL_EMU(1 << TIF_SYSCALL_EMU)
> #define _TIF_UPROBE(1 << TIF_UPROBE)
> #define _TIF_FSCHECK(1 << TIF_FSCHECK)
> #define _TIF_32BIT(1 << TIF_32BIT)
> @@ -120,7 +123,7 @@ void arch_release_task_struct(struct task_struct *tsk);
>
> #define _TIF_SYSCALL_WORK(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
> _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \
> - _TIF_NOHZ)
> + _TIF_NOHZ | _TIF_SYSCALL_EMU)
>
> #define INIT_THREAD_INFO(tsk)\
> {\
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index b82e0a9b3da3..cf29275cd4d9 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -1819,6 +1819,9 @@ static void tracehook_report_syscall(struct pt_regs
> *regs,
>
> int syscall_trace_enter(struct pt_regs *regs)
> {
> +if (unlikely(ptrace_syscall_enter(regs)))
> +return -1;
> +
> if (test_thread_flag(TIF_SYSCALL_TRACE))
> tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
Re: [PATCH 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook
On 2019/3/14 18:51, Sudeep Holla wrote: > On Wed, Mar 13, 2019 at 01:03:18AM +0000, Haibo Xu (Arm Technology China) > wrote: > [...] > >> Since ptrace() system call do have so many request type, I'm not sure >> whether the test cases have covered all of that. But here we'd better make >> sure the PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP requests are work >> correctly. May be you can verify them with tests from Bin Lu(bin...@arm.com). > > Sure happy to try them. Can you point me to them ? > I did end up writing few more tests. > > -- > Regards, > Sudeep > You can get them from Steve Capper. BTW, I also have a program to verify the PTRACE_SYSEMU function, and will send to you in a separate email loop. Regards, Haibo IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Re: [PATCH 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook
On 2019/3/12 20:09, Sudeep Holla wrote:
> On Mon, Mar 11, 2019 at 08:04:39PM -0700, Andy Lutomirski wrote:
>> On Mon, Mar 11, 2019 at 6:35 PM Haibo Xu (Arm Technology China)
>> wrote:
>>>
>
> [...]
>
>>> For the PTRACE_SYSEMU_SINGLESTEP request, ptrace only need to report(send
>>> SIGTRAP) at the entry of a system call, no need to report at the exit of a
>>> system call.That's why the old logic-{step = ((flags & (_TIF_SINGLESTEP |
>>> _TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP)} here try to filter out the special
>>> case(PTRACE_SYSEMU_SINGLESTEP).
>>>
>>> Another way to make sure the logic is fine, you can run some tests with
>>> respect to both logic, and to check whether they have the same behavior.
>>
>> tools/testing/selftests/x86/ptrace_syscall.c has a test intended to
>> exercise this. Can one of you either confirm that it does exercise it
>> and that it still passes or can you improve the test?
>>
> I did run the tests which didn't flag anything. I haven't looked at the
> details of test implementation, but seem to miss this case. I will see
> what can be improved(if it's possible). Also I think single_step_syscall
> is the one I need to look for this particular one. Both single_step_syscall
> ptrace_syscall reported no errors.
>
> --
> Regards,
> Sudeep
>
Since ptrace() system call do have so many request type, I'm not sure whether
the
test cases have covered all of that. But here we'd better make sure the
PTRACE_SYSEMU
and PTRACE_SYSEMU_SINGLESTEP requests are work correctly. May be you can verify
them with
tests from Bin Lu(bin...@arm.com).
Regards,
Haibo
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
Re: [PATCH 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook
On 2019/3/12 2:34, Sudeep Holla wrote:
> (I thought I had sent this email, last Tuesday itself, but saw this in my
> draft today, something went wrong, sorry for the delay)
>
> On Tue, Mar 05, 2019 at 02:14:47AM +0000, Haibo Xu (Arm Technology China)
> wrote:
>> On 2019/3/4 18:12, Sudeep Holla wrote:
>>> On Mon, Mar 04, 2019 at 08:25:28AM +0000, Haibo Xu (Arm Technology China)
>>> wrote:
>>>> On 2019/3/1 2:32, Sudeep Holla wrote:
>>>>> Now that we have a new hook ptrace_syscall_enter that can be called from
>>>>> syscall entry code and it handles PTRACE_SYSEMU in generic code, we
>>>>> can do some cleanup using the same in syscall_trace_enter.
>>>>>
>>>>> Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
>>>>> in syscall_slow_exit_work seems unnecessary. Let's remove the same.
>>>>
>>>> I think we should not change the logic here. Is so, it will double the
>>>> report of syscall
>>>> when PTRACE_SYSEMU_SINGLESTEP is enabled.
>>>>
>>>
>>> I don't think that should happen, but I may be missing something.
>>> Can you explain how ?
>>>
>>
>> When PTRACE_SYSEMU_SINGLESTEP is enabled, both the _TIF_SYSCALL_EMU and
>> _TIF_SINGLESTEP flags are set, but ptrace only need to report(send SIGTRAP)
>> at the entry of a system call, no need to report at the exit of a system
>> call.
>>
> Sorry, but I still not get it, we have:
>
> step = ((flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP);
>
> For me, this is same as:
> step = ((flags & _TIF_SINGLESTEP) == _TIF_SINGLESTEP)
> or
> if (flags & _TIF_SINGLESTEP)
> step = true;
>
I don't think so! As I mentioned in the last email loop, when
PTRACE_SYSEMU_SINGLESTEP
is enabled, both the _TIF_SYSCALL_EMU and _TIF_SINGLESTEP flags are set, in
which case
the step should be "false" for the old logic. But with the new logic, the step
is "true".
> So when PTRACE_SYSEMU_SINGLESTEP, _TIF_SYSCALL_EMU and _TIF_SINGLESTEP
> are set and step evaluates to true.
>
> So dropping _TIF_SYSCALL_EMU here should be fine. Am I still missing
> something ?
>
> --
> Regards,
> Sudeep
>
For the PTRACE_SYSEMU_SINGLESTEP request, ptrace only need to report(send
SIGTRAP)
at the entry of a system call, no need to report at the exit of a system
call.That's
why the old logic-{step = ((flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU)) ==
_TIF_SINGLESTEP)}
here try to filter out the special case(PTRACE_SYSEMU_SINGLESTEP).
Another way to make sure the logic is fine, you can run some tests with respect
to both logic,
and to check whether they have the same behavior.
Regards,
Haibo
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
Re: [PATCH 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook
On 2019/3/4 18:12, Sudeep Holla wrote: > On Mon, Mar 04, 2019 at 08:25:28AM +0000, Haibo Xu (Arm Technology China) > wrote: >> On 2019/3/1 2:32, Sudeep Holla wrote: >>> Now that we have a new hook ptrace_syscall_enter that can be called from >>> syscall entry code and it handles PTRACE_SYSEMU in generic code, we >>> can do some cleanup using the same in syscall_trace_enter. >>> >>> Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP >>> in syscall_slow_exit_work seems unnecessary. Let's remove the same. >> >> I think we should not change the logic here. Is so, it will double the >> report of syscall >> when PTRACE_SYSEMU_SINGLESTEP is enabled. >> > > I don't think that should happen, but I may be missing something. > Can you explain how ? > > -- > Regards, > Sudeep > When PTRACE_SYSEMU_SINGLESTEP is enabled, both the _TIF_SYSCALL_EMU and _TIF_SINGLESTEP flags are set, but ptrace only need to report(send SIGTRAP) at the entry of a system call, no need to report at the exit of a system call. Regards, Haibo IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Re: [PATCH 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU
On 2019/3/1 2:32, Sudeep Holla wrote:
> Now that we have a new hook ptrace_syscall_enter that can be called from
> syscall entry code and it handles PTRACE_SYSEMU in generic code, we
> can do some cleanup using the same in do_syscall_trace_enter.
>
> Cc: Oleg Nesterov
> Cc: Paul Mackerras
> Cc: Michael Ellerman
> Signed-off-by: Sudeep Holla
> ---
> arch/powerpc/kernel/ptrace.c | 50
> 1 file changed, 22 insertions(+), 28 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index cb7e1439cafb..978cd2aac29e 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3264,37 +3264,31 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> {
> u32 flags;
>
> -user_exit();
We'd better keep the user_exit() at here in case both context tracking and
SYSCALL_EMU
are enabled.
> -
> -flags = READ_ONCE(current_thread_info()->flags) &
> -(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> +if (unlikely(ptrace_syscall_enter(regs))) {
> +/*
> + * A nonzero return code from tracehook_report_syscall_entry()
> + * tells us to prevent the syscall execution, but we are not
> + * going to execute it anyway.
> + *
> + * Returning -1 will skip the syscall execution. We want to
> + * avoid clobbering any registers, so we don't goto the skip
> + * label below.
> + */
> +return -1;
> +}
>
> -if (flags) {
> -int rc = tracehook_report_syscall_entry(regs);
> +user_exit();
>
> -if (unlikely(flags & _TIF_SYSCALL_EMU)) {
> -/*
> - * A nonzero return code from
> - * tracehook_report_syscall_entry() tells us to prevent
> - * the syscall execution, but we are not going to
> - * execute it anyway.
> - *
> - * Returning -1 will skip the syscall execution. We want
> - * to avoid clobbering any registers, so we don't goto
> - * the skip label below.
> - */
> -return -1;
> -}
> +flags = READ_ONCE(current_thread_info()->flags) & _TIF_SYSCALL_TRACE;
>
> -if (rc) {
> -/*
> - * The tracer decided to abort the syscall. Note that
> - * the tracer may also just change regs->gpr[0] to an
> - * invalid syscall number, that is handled below on the
> - * exit path.
> - */
> -goto skip;
> -}
> +if (flags && tracehook_report_syscall_entry(regs)) {
> +/*
> + * The tracer decided to abort the syscall. Note that
> + * the tracer may also just change regs->gpr[0] to an
> + * invalid syscall number, that is handled below on the
> + * exit path.
> + */
> +goto skip;
> }
>
> /* Run seccomp after ptrace; allow it to set gpr[3]. */
>
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
Re: [PATCH 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook
On 2019/3/1 2:32, Sudeep Holla wrote: > Now that we have a new hook ptrace_syscall_enter that can be called from > syscall entry code and it handles PTRACE_SYSEMU in generic code, we > can do some cleanup using the same in syscall_trace_enter. > > Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP > in syscall_slow_exit_work seems unnecessary. Let's remove the same. I think we should not change the logic here. Is so, it will double the report of syscall when PTRACE_SYSEMU_SINGLESTEP is enabled. > > Cc: Andy Lutomirski > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: Borislav Petkov > Signed-off-by: Sudeep Holla > --- > arch/x86/entry/common.c | 22 -- > 1 file changed, 4 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c > index 7bc105f47d21..36457c1f87d2 100644 > --- a/arch/x86/entry/common.c > +++ b/arch/x86/entry/common.c > @@ -70,22 +70,16 @@ static long syscall_trace_enter(struct pt_regs *regs) > > struct thread_info *ti = current_thread_info(); > unsigned long ret = 0; > -bool emulated = false; > u32 work; > > if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) > BUG_ON(regs != task_pt_regs(current)); > > -work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; > - > -if (unlikely(work & _TIF_SYSCALL_EMU)) > -emulated = true; > - > -if ((emulated || (work & _TIF_SYSCALL_TRACE)) && > -tracehook_report_syscall_entry(regs)) > +if (unlikely(ptrace_syscall_enter(regs))) > return -1L; > > -if (emulated) > +work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; > +if ((work & _TIF_SYSCALL_TRACE) && tracehook_report_syscall_entry(regs)) > return -1L; > > #ifdef CONFIG_SECCOMP > @@ -227,15 +221,7 @@ static void syscall_slow_exit_work(struct pt_regs *regs, > u32 cached_flags) > if (cached_flags & _TIF_SYSCALL_TRACEPOINT) > trace_sys_exit(regs, regs->ax); > > -/* > - * If TIF_SYSCALL_EMU is set, we only get here because of > - * TIF_SINGLESTEP (i.e. this is PTRACE_SYSEMU_SINGLESTEP). > - * We already reported this syscall instruction in > - * syscall_trace_enter(). > - */ > -step = unlikely( > -(cached_flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU)) > -== _TIF_SINGLESTEP); > +step = unlikely((cached_flags & _TIF_SINGLESTEP)); > if (step || cached_flags & _TIF_SYSCALL_TRACE) > tracehook_report_syscall_exit(regs, step); > } > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Re: [PATCH 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling
On 2019/3/1 2:32, Sudeep Holla wrote:
> Currently each architecture handles PTRACE_SYSEMU in very similar way.
> It's completely arch independent and can be handled in the code helping
> to consolidate PTRACE_SYSEMU handling.
>
> Let's introduce a hook 'ptrace_syscall_enter' that arch specific syscall
> entry code can call.
>
The 'ptrace_syscall_enter' is dedicated for PTRACE_SYSEMU flag,
So I suggest to rename the function to something like
'ptrace_syscall_emu_enter".
> +/*
> + * Hook to check and report for PTRACE_SYSEMU, can be called from arch
> + * arch syscall entry code
> + */
> +long ptrace_syscall_enter(struct pt_regs *regs)
> +{
> +#ifdef TIF_SYSCALL_EMU
> +if (test_thread_flag(TIF_SYSCALL_EMU)) {
> +if (tracehook_report_syscall_entry(regs));
Shall we remove the semi-colon at end of the above line?
> +return -1L;
> +}
> +#endif
> +return 0;
> +}
> +
> /*
> * Detach all tasks we were using ptrace on. Called with tasklist held
> * for writing.
>
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
