From: Elvira Khabirova <[email protected]>
PTRACE_GET_SYSCALL_INFO is a generic ptrace API that lets ptracer obtain
details of the syscall the tracee is blocked in.
There are two reasons for a special syscall-related ptrace request.
Firstly, with the current ptrace API there are cases when ptracer cannot
retrieve necessary information about syscalls. Some examples include:
* The notorious int-0x80-from-64-bit-task issue. See [1] for details.
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.
Common practice is to keep track of the sequence of ptrace-stops in order
not to mix the two syscall-stops up. But it is not as simple as it looks;
for example, strace had a (just recently fixed) long-standing bug where
attaching strace to a tracee that is performing the execve system call
led to the tracer identifying the following syscall-exit-stop as
syscall-enter-stop, which messed up all the state tracking.
* Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
and process_vm_readv become unavailable when the process dumpable flag
is cleared. On such architectures as ia64 this results in all syscall
arguments being unavailable for the tracer.
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.
ptrace(2) man page:
long ptrace(enum __ptrace_request request, pid_t pid,
void *addr, void *data);
...
PTRACE_GET_SYSCALL_INFO
Retrieve information about the syscall that caused the stop.
The information is placed into the buffer pointed by "data"
argument, which should be a pointer to a buffer of type
"struct ptrace_syscall_info".
The "addr" argument contains the size of the buffer pointed to
by "data" argument (i.e., sizeof(struct ptrace_syscall_info)).
The return value contains the number of bytes available
to be written by the kernel.
If the size of data to be written by the kernel exceeds the size
specified by "addr" argument, the output is truncated.
Co-authored-by: Dmitry V. Levin <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Jann Horn <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Elvira Khabirova <[email protected]>
Signed-off-by: Dmitry V. Levin <[email protected]>
---
Notes:
v5:
* Change PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} values as requested by Oleg.
* Change struct ptrace_syscall_info: generalize instruction_pointer,
stack_pointer, and frame_pointer fields by moving them from
ptrace_syscall_info.{entry,seccomp} substructures to ptrace_syscall_info
and initializing them for all stops.
* Add PTRACE_SYSCALL_INFO_NONE, set it when not in a syscall stop,
so e.g. "strace -i" could use PTRACE_SYSCALL_INFO_SECCOMP to obtain
instruction_pointer when the tracee is in a signal stop.
* Make available for all architectures: do not conditionalize on
CONFIG_HAVE_ARCH_TRACEHOOK since all syscall_get_* functions
are implemented on all architectures.
v4:
* Do not introduce task_struct.ptrace_event,
use child->last_siginfo->si_code instead.
* Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
ptrace_syscall_info.{entry,exit}.
v3:
* Change struct ptrace_syscall_info.
* Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
* Add proper defines for ptrace_syscall_info.op values.
* Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
* and move them to uapi.
v2:
* Do not use task->ptrace.
* Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
* Use addr argument of sys_ptrace to get expected size of the struct;
return full size of the struct.
include/linux/tracehook.h | 9 ++--
include/uapi/linux/ptrace.h | 39 +++++++++++++++
kernel/ptrace.c | 99 ++++++++++++++++++++++++++++++++++++-
3 files changed, 143 insertions(+), 4 deletions(-)
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index df20f8bdbfa3..6bc7a3d58e2f 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -57,13 +57,15 @@ struct linux_binprm;
/*
* ptrace report for syscall entry and exit looks identical.
*/
-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_message = message;
ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0));
/*
@@ -76,6 +78,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
current->exit_code = 0;
}
+ current->ptrace_message = 0;
return fatal_signal_pending(current);
}
@@ -101,7 +104,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
static inline __must_check int tracehook_report_syscall_entry(
struct pt_regs *regs)
{
- return ptrace_report_syscall(regs);
+ return ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_ENTRY);
}
/**
@@ -126,7 +129,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
if (step)
user_single_step_report(regs);
else
- ptrace_report_syscall(regs);
+ ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_EXIT);
}
/**
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index d5a1b8a492b9..f0af09fe4e17 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -73,6 +73,45 @@ struct seccomp_metadata {
__u64 flags; /* Output: filter's flags */
};
+#define PTRACE_GET_SYSCALL_INFO 0x420e
+#define PTRACE_SYSCALL_INFO_NONE 0
+#define PTRACE_SYSCALL_INFO_ENTRY 1
+#define PTRACE_SYSCALL_INFO_EXIT 2
+#define PTRACE_SYSCALL_INFO_SECCOMP 3
+
+struct ptrace_syscall_info {
+ __u8 op; /* PTRACE_SYSCALL_INFO_* */
+ __u8 __pad0[3];
+ __u32 arch;
+ __u64 instruction_pointer;
+ __u64 stack_pointer;
+ __u64 frame_pointer;
+ union {
+ struct {
+ __u64 nr;
+ __u64 args[6];
+ } entry;
+ struct {
+ __s64 rval;
+ __u8 is_error;
+ __u8 __pad1[7];
+ } exit;
+ struct {
+ __u64 nr;
+ __u64 args[6];
+ __u32 ret_data;
+ __u8 __pad2[4];
+ } seccomp;
+ };
+};
+
+/*
+ * These values are stored in task->ptrace_message
+ * by tracehook_report_syscall_* to describe the current syscall-stop.
+ */
+#define PTRACE_EVENTMSG_SYSCALL_ENTRY 1
+#define PTRACE_EVENTMSG_SYSCALL_EXIT 2
+
/* Read signals from a shared (process wide) queue */
#define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index c2cee9db5204..4562b2cb1087 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -30,6 +30,8 @@
#include <linux/cn_proc.h>
#include <linux/compat.h>
+#include <asm/syscall.h> /* For syscall_get_* */
+
/*
* Access another process' address space via ptrace.
* Source/target buffer must be kernel space,
@@ -878,7 +880,98 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
* to ensure no machine forgets it.
*/
EXPORT_SYMBOL_GPL(task_user_regset_view);
-#endif
+#endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
+
+static unsigned long
+ptrace_get_syscall_info_entry(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ unsigned long args[ARRAY_SIZE(info->entry.args)];
+ int i;
+
+ info->op = PTRACE_SYSCALL_INFO_ENTRY;
+ info->entry.nr = syscall_get_nr(child, regs);
+ syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
+ for (i = 0; i < ARRAY_SIZE(args); i++)
+ info->entry.args[i] = args[i];
+
+ return offsetofend(struct ptrace_syscall_info, entry);
+}
+
+static unsigned long
+ptrace_get_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ /*
+ * As struct ptrace_syscall_info.entry is currently a subset
+ * of struct ptrace_syscall_info.seccomp, it makes sense to
+ * initialize that subset using ptrace_get_syscall_info_entry().
+ * This can be reconsidered in the future if these structures
+ * diverge significantly enough.
+ */
+ ptrace_get_syscall_info_entry(child, regs, info);
+ info->op = PTRACE_SYSCALL_INFO_SECCOMP;
+ info->seccomp.ret_data = child->ptrace_message;
+
+ return offsetofend(struct ptrace_syscall_info, seccomp);
+}
+
+static unsigned long
+ptrace_get_syscall_info_exit(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ info->op = PTRACE_SYSCALL_INFO_EXIT;
+ info->exit.rval = syscall_get_error(child, regs);
+ info->exit.is_error = !!info->exit.rval;
+ if (!info->exit.is_error)
+ info->exit.rval = syscall_get_return_value(child, regs);
+
+ return offsetofend(struct ptrace_syscall_info, exit);
+}
+
+static int
+ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
+ void __user *datavp)
+{
+ struct pt_regs *regs = task_pt_regs(child);
+ struct ptrace_syscall_info info = {
+ .op = PTRACE_SYSCALL_INFO_NONE,
+ .arch = syscall_get_arch(child),
+ .instruction_pointer = instruction_pointer(regs),
+ .stack_pointer = user_stack_pointer(regs),
+ .frame_pointer = frame_pointer(regs)
+ };
+ unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
+ unsigned long write_size;
+
+ /*
+ * This does not need lock_task_sighand() to access
+ * child->last_siginfo because ptrace_freeze_traced()
+ * called earlier by ptrace_check_attach() ensures that
+ * the tracee cannot go away and clear its last_siginfo.
+ */
+ switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
+ case SIGTRAP | 0x80:
+ switch (child->ptrace_message) {
+ case PTRACE_EVENTMSG_SYSCALL_ENTRY:
+ actual_size = ptrace_get_syscall_info_entry(child, regs,
+ &info);
+ break;
+ case PTRACE_EVENTMSG_SYSCALL_EXIT:
+ actual_size = ptrace_get_syscall_info_exit(child, regs,
+ &info);
+ break;
+ }
+ break;
+ case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
+ actual_size = ptrace_get_syscall_info_seccomp(child, regs,
+ &info);
+ break;
+ }
+
+ write_size = min(actual_size, user_size);
+ return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
+}
int ptrace_request(struct task_struct *child, long request,
unsigned long addr, unsigned long data)
@@ -1095,6 +1188,10 @@ int ptrace_request(struct task_struct *child, long request,
ret = seccomp_get_metadata(child, addr, datavp);
break;
+ case PTRACE_GET_SYSCALL_INFO:
+ ret = ptrace_get_syscall_info(child, addr, datavp);
+ break;
+
default:
break;
}
--
ldv
On 12/10, Dmitry V. Levin wrote:
>
> +struct ptrace_syscall_info {
> + __u8 op; /* PTRACE_SYSCALL_INFO_* */
> + __u8 __pad0[3];
> + __u32 arch;
> + __u64 instruction_pointer;
> + __u64 stack_pointer;
> + __u64 frame_pointer;
> + union {
> + struct {
> + __u64 nr;
> + __u64 args[6];
> + } entry;
> + struct {
> + __s64 rval;
> + __u8 is_error;
> + __u8 __pad1[7];
> + } exit;
> + struct {
> + __u64 nr;
> + __u64 args[6];
> + __u32 ret_data;
> + __u8 __pad2[4];
> + } seccomp;
> + };
> +};
Could you explain why ptrace_syscall_info needs __pad{0,1,2} ? I simply can't
understand why...
Otherwise the patch looks good to me. I am not going to discuss the API and
data layout, I am fine with anything which suits user-space needs.
I think the patch is technically correct, feel free to add
Reviewed-by: Oleg Nesterov <[email protected]>
Hi Elvira,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc6]
[cannot apply to next-20181207]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-V-Levin/ptrace-add-PTRACE_GET_SYSCALL_INFO-request/20181210-174745
config: mips-malta_kvm_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=mips
All errors (new ones prefixed by >>):
kernel/ptrace.c: In function 'ptrace_get_syscall_info':
>> kernel/ptrace.c:942:20: error: implicit declaration of function 'frame_pointer'; did you mean 'trace_printk'? [-Werror=implicit-function-declaration]
.frame_pointer = frame_pointer(regs)
^~~~~~~~~~~~~
trace_printk
cc1: some warnings being treated as errors
vim +942 kernel/ptrace.c
931
932 static int
933 ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
934 void __user *datavp)
935 {
936 struct pt_regs *regs = task_pt_regs(child);
937 struct ptrace_syscall_info info = {
938 .op = PTRACE_SYSCALL_INFO_NONE,
939 .arch = syscall_get_arch(child),
940 .instruction_pointer = instruction_pointer(regs),
941 .stack_pointer = user_stack_pointer(regs),
> 942 .frame_pointer = frame_pointer(regs)
943 };
944 unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
945 unsigned long write_size;
946
947 /*
948 * This does not need lock_task_sighand() to access
949 * child->last_siginfo because ptrace_freeze_traced()
950 * called earlier by ptrace_check_attach() ensures that
951 * the tracee cannot go away and clear its last_siginfo.
952 */
953 switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
954 case SIGTRAP | 0x80:
955 switch (child->ptrace_message) {
956 case PTRACE_EVENTMSG_SYSCALL_ENTRY:
957 actual_size = ptrace_get_syscall_info_entry(child, regs,
958 &info);
959 break;
960 case PTRACE_EVENTMSG_SYSCALL_EXIT:
961 actual_size = ptrace_get_syscall_info_exit(child, regs,
962 &info);
963 break;
964 }
965 break;
966 case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
967 actual_size = ptrace_get_syscall_info_seccomp(child, regs,
968 &info);
969 break;
970 }
971
972 write_size = min(actual_size, user_size);
973 return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
974 }
975
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi, things are getting too complicated and we need some advice how to deal
with this frame_pointer issue.
On Mon, Dec 10, 2018 at 10:26:50PM +0800, kbuild test robot wrote:
> Hi Elvira,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.20-rc6]
> [cannot apply to next-20181207]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Dmitry-V-Levin/ptrace-add-PTRACE_GET_SYSCALL_INFO-request/20181210-174745
> config: mips-malta_kvm_defconfig (attached as .config)
> compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.2.0 make.cross ARCH=mips
>
> All errors (new ones prefixed by >>):
>
> kernel/ptrace.c: In function 'ptrace_get_syscall_info':
> >> kernel/ptrace.c:942:20: error: implicit declaration of function 'frame_pointer'; did you mean 'trace_printk'? [-Werror=implicit-function-declaration]
> .frame_pointer = frame_pointer(regs)
> ^~~~~~~~~~~~~
> trace_printk
> cc1: some warnings being treated as errors
>
> vim +942 kernel/ptrace.c
>
> 931
> 932 static int
> 933 ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> 934 void __user *datavp)
> 935 {
> 936 struct pt_regs *regs = task_pt_regs(child);
> 937 struct ptrace_syscall_info info = {
> 938 .op = PTRACE_SYSCALL_INFO_NONE,
> 939 .arch = syscall_get_arch(child),
> 940 .instruction_pointer = instruction_pointer(regs),
> 941 .stack_pointer = user_stack_pointer(regs),
> > 942 .frame_pointer = frame_pointer(regs)
> 943 };
> 944 unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
> 945 unsigned long write_size;
> 946
> 947 /*
> 948 * This does not need lock_task_sighand() to access
> 949 * child->last_siginfo because ptrace_freeze_traced()
> 950 * called earlier by ptrace_check_attach() ensures that
> 951 * the tracee cannot go away and clear its last_siginfo.
> 952 */
> 953 switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
> 954 case SIGTRAP | 0x80:
> 955 switch (child->ptrace_message) {
> 956 case PTRACE_EVENTMSG_SYSCALL_ENTRY:
> 957 actual_size = ptrace_get_syscall_info_entry(child, regs,
> 958 &info);
> 959 break;
> 960 case PTRACE_EVENTMSG_SYSCALL_EXIT:
> 961 actual_size = ptrace_get_syscall_info_exit(child, regs,
> 962 &info);
> 963 break;
> 964 }
> 965 break;
> 966 case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
> 967 actual_size = ptrace_get_syscall_info_seccomp(child, regs,
> 968 &info);
> 969 break;
> 970 }
> 971
> 972 write_size = min(actual_size, user_size);
> 973 return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
> 974 }
> 975
We decided to add .frame_pointer to struct ptrace_syscall_info just for
consistency with .instruction_pointer and .stack_pointer; I must have been
misled by comments in asm-generic/ptrace.h into thinking that
frame_pointer() is universally available across architectures.
Unlike .instruction_pointer and .stack_pointer that are actually needed
in strace, .frame_pointer is not used, so from strace PoV we don't really
need it.
So the question is, does anybody need a
struct ptrace_syscall_info.frame_pointer?
If yes, how can frame_pointer() be defined on MIPS?
Or should we just forget about making sense of frame_pointer() and remove
struct ptrace_syscall_info.frame_pointer from the proposed API?
--
ldv
On Mon, Dec 10, 2018 at 03:11:07PM +0100, Oleg Nesterov wrote:
> On 12/10, Dmitry V. Levin wrote:
> >
> > +struct ptrace_syscall_info {
> > + __u8 op; /* PTRACE_SYSCALL_INFO_* */
> > + __u8 __pad0[3];
> > + __u32 arch;
> > + __u64 instruction_pointer;
> > + __u64 stack_pointer;
> > + __u64 frame_pointer;
> > + union {
> > + struct {
> > + __u64 nr;
> > + __u64 args[6];
> > + } entry;
> > + struct {
> > + __s64 rval;
> > + __u8 is_error;
> > + __u8 __pad1[7];
> > + } exit;
> > + struct {
> > + __u64 nr;
> > + __u64 args[6];
> > + __u32 ret_data;
> > + __u8 __pad2[4];
> > + } seccomp;
> > + };
> > +};
>
> Could you explain why ptrace_syscall_info needs __pad{0,1,2} ? I simply can't
> understand why...
I suppose the idea behind the use of these pads was to make the structure
arch-independent.
I don't think we really need to keep it exactly the same on all
architectures - the only practical requirement is to avoid any compat
issues, but I don't mind keeping the structure arch-independent.
--
ldv
On Sun, Dec 9, 2018 at 8:31 PM Dmitry V. Levin <[email protected]> wrote:
>
> From: Elvira Khabirova <[email protected]>
>
> PTRACE_GET_SYSCALL_INFO is a generic ptrace API that lets ptracer obtain
> details of the syscall the tracee is blocked in.
>
> There are two reasons for a special syscall-related ptrace request.
>
> Firstly, with the current ptrace API there are cases when ptracer cannot
> retrieve necessary information about syscalls. Some examples include:
> * The notorious int-0x80-from-64-bit-task issue. See [1] for details.
> 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.
> Common practice is to keep track of the sequence of ptrace-stops in order
> not to mix the two syscall-stops up. But it is not as simple as it looks;
> for example, strace had a (just recently fixed) long-standing bug where
> attaching strace to a tracee that is performing the execve system call
> led to the tracer identifying the following syscall-exit-stop as
> syscall-enter-stop, which messed up all the state tracking.
> * Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
> ("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
> and process_vm_readv become unavailable when the process dumpable flag
> is cleared. On such architectures as ia64 this results in all syscall
> arguments being unavailable for the tracer.
>
> 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.
>
> ptrace(2) man page:
>
> long ptrace(enum __ptrace_request request, pid_t pid,
> void *addr, void *data);
> ...
> PTRACE_GET_SYSCALL_INFO
> Retrieve information about the syscall that caused the stop.
> The information is placed into the buffer pointed by "data"
> argument, which should be a pointer to a buffer of type
> "struct ptrace_syscall_info".
> The "addr" argument contains the size of the buffer pointed to
> by "data" argument (i.e., sizeof(struct ptrace_syscall_info)).
> The return value contains the number of bytes available
> to be written by the kernel.
> If the size of data to be written by the kernel exceeds the size
> specified by "addr" argument, the output is truncated.
>
> Co-authored-by: Dmitry V. Levin <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Eugene Syromyatnikov <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Jann Horn <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Elvira Khabirova <[email protected]>
> Signed-off-by: Dmitry V. Levin <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
-Kees
> ---
>
> Notes:
> v5:
> * Change PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} values as requested by Oleg.
> * Change struct ptrace_syscall_info: generalize instruction_pointer,
> stack_pointer, and frame_pointer fields by moving them from
> ptrace_syscall_info.{entry,seccomp} substructures to ptrace_syscall_info
> and initializing them for all stops.
> * Add PTRACE_SYSCALL_INFO_NONE, set it when not in a syscall stop,
> so e.g. "strace -i" could use PTRACE_SYSCALL_INFO_SECCOMP to obtain
> instruction_pointer when the tracee is in a signal stop.
> * Make available for all architectures: do not conditionalize on
> CONFIG_HAVE_ARCH_TRACEHOOK since all syscall_get_* functions
> are implemented on all architectures.
>
> v4:
> * Do not introduce task_struct.ptrace_event,
> use child->last_siginfo->si_code instead.
> * Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
> support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
> ptrace_syscall_info.{entry,exit}.
>
> v3:
> * Change struct ptrace_syscall_info.
> * Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
> * Add proper defines for ptrace_syscall_info.op values.
> * Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
> PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
> * and move them to uapi.
>
> v2:
> * Do not use task->ptrace.
> * Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
> * Use addr argument of sys_ptrace to get expected size of the struct;
> return full size of the struct.
>
> include/linux/tracehook.h | 9 ++--
> include/uapi/linux/ptrace.h | 39 +++++++++++++++
> kernel/ptrace.c | 99 ++++++++++++++++++++++++++++++++++++-
> 3 files changed, 143 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
> index df20f8bdbfa3..6bc7a3d58e2f 100644
> --- a/include/linux/tracehook.h
> +++ b/include/linux/tracehook.h
> @@ -57,13 +57,15 @@ struct linux_binprm;
> /*
> * ptrace report for syscall entry and exit looks identical.
> */
> -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_message = message;
> ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0));
>
> /*
> @@ -76,6 +78,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
> current->exit_code = 0;
> }
>
> + current->ptrace_message = 0;
> return fatal_signal_pending(current);
> }
>
> @@ -101,7 +104,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
> static inline __must_check int tracehook_report_syscall_entry(
> struct pt_regs *regs)
> {
> - return ptrace_report_syscall(regs);
> + return ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_ENTRY);
> }
>
> /**
> @@ -126,7 +129,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
> if (step)
> user_single_step_report(regs);
> else
> - ptrace_report_syscall(regs);
> + ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_EXIT);
> }
>
> /**
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index d5a1b8a492b9..f0af09fe4e17 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -73,6 +73,45 @@ struct seccomp_metadata {
> __u64 flags; /* Output: filter's flags */
> };
>
> +#define PTRACE_GET_SYSCALL_INFO 0x420e
> +#define PTRACE_SYSCALL_INFO_NONE 0
> +#define PTRACE_SYSCALL_INFO_ENTRY 1
> +#define PTRACE_SYSCALL_INFO_EXIT 2
> +#define PTRACE_SYSCALL_INFO_SECCOMP 3
> +
> +struct ptrace_syscall_info {
> + __u8 op; /* PTRACE_SYSCALL_INFO_* */
> + __u8 __pad0[3];
> + __u32 arch;
> + __u64 instruction_pointer;
> + __u64 stack_pointer;
> + __u64 frame_pointer;
> + union {
> + struct {
> + __u64 nr;
> + __u64 args[6];
> + } entry;
> + struct {
> + __s64 rval;
> + __u8 is_error;
> + __u8 __pad1[7];
> + } exit;
> + struct {
> + __u64 nr;
> + __u64 args[6];
> + __u32 ret_data;
> + __u8 __pad2[4];
> + } seccomp;
> + };
> +};
> +
> +/*
> + * These values are stored in task->ptrace_message
> + * by tracehook_report_syscall_* to describe the current syscall-stop.
> + */
> +#define PTRACE_EVENTMSG_SYSCALL_ENTRY 1
> +#define PTRACE_EVENTMSG_SYSCALL_EXIT 2
> +
> /* Read signals from a shared (process wide) queue */
> #define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index c2cee9db5204..4562b2cb1087 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -30,6 +30,8 @@
> #include <linux/cn_proc.h>
> #include <linux/compat.h>
>
> +#include <asm/syscall.h> /* For syscall_get_* */
> +
> /*
> * Access another process' address space via ptrace.
> * Source/target buffer must be kernel space,
> @@ -878,7 +880,98 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
> * to ensure no machine forgets it.
> */
> EXPORT_SYMBOL_GPL(task_user_regset_view);
> -#endif
> +#endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
> +
> +static unsigned long
> +ptrace_get_syscall_info_entry(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + unsigned long args[ARRAY_SIZE(info->entry.args)];
> + int i;
> +
> + info->op = PTRACE_SYSCALL_INFO_ENTRY;
> + info->entry.nr = syscall_get_nr(child, regs);
> + syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
> + for (i = 0; i < ARRAY_SIZE(args); i++)
> + info->entry.args[i] = args[i];
> +
> + return offsetofend(struct ptrace_syscall_info, entry);
> +}
> +
> +static unsigned long
> +ptrace_get_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + /*
> + * As struct ptrace_syscall_info.entry is currently a subset
> + * of struct ptrace_syscall_info.seccomp, it makes sense to
> + * initialize that subset using ptrace_get_syscall_info_entry().
> + * This can be reconsidered in the future if these structures
> + * diverge significantly enough.
> + */
> + ptrace_get_syscall_info_entry(child, regs, info);
> + info->op = PTRACE_SYSCALL_INFO_SECCOMP;
> + info->seccomp.ret_data = child->ptrace_message;
> +
> + return offsetofend(struct ptrace_syscall_info, seccomp);
> +}
> +
> +static unsigned long
> +ptrace_get_syscall_info_exit(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + info->op = PTRACE_SYSCALL_INFO_EXIT;
> + info->exit.rval = syscall_get_error(child, regs);
> + info->exit.is_error = !!info->exit.rval;
> + if (!info->exit.is_error)
> + info->exit.rval = syscall_get_return_value(child, regs);
> +
> + return offsetofend(struct ptrace_syscall_info, exit);
> +}
> +
> +static int
> +ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> + void __user *datavp)
> +{
> + struct pt_regs *regs = task_pt_regs(child);
> + struct ptrace_syscall_info info = {
> + .op = PTRACE_SYSCALL_INFO_NONE,
> + .arch = syscall_get_arch(child),
> + .instruction_pointer = instruction_pointer(regs),
> + .stack_pointer = user_stack_pointer(regs),
> + .frame_pointer = frame_pointer(regs)
> + };
> + unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
> + unsigned long write_size;
> +
> + /*
> + * This does not need lock_task_sighand() to access
> + * child->last_siginfo because ptrace_freeze_traced()
> + * called earlier by ptrace_check_attach() ensures that
> + * the tracee cannot go away and clear its last_siginfo.
> + */
> + switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
> + case SIGTRAP | 0x80:
> + switch (child->ptrace_message) {
> + case PTRACE_EVENTMSG_SYSCALL_ENTRY:
> + actual_size = ptrace_get_syscall_info_entry(child, regs,
> + &info);
> + break;
> + case PTRACE_EVENTMSG_SYSCALL_EXIT:
> + actual_size = ptrace_get_syscall_info_exit(child, regs,
> + &info);
> + break;
> + }
> + break;
> + case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
> + actual_size = ptrace_get_syscall_info_seccomp(child, regs,
> + &info);
> + break;
> + }
> +
> + write_size = min(actual_size, user_size);
> + return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
> +}
>
> int ptrace_request(struct task_struct *child, long request,
> unsigned long addr, unsigned long data)
> @@ -1095,6 +1188,10 @@ int ptrace_request(struct task_struct *child, long request,
> ret = seccomp_get_metadata(child, addr, datavp);
> break;
>
> + case PTRACE_GET_SYSCALL_INFO:
> + ret = ptrace_get_syscall_info(child, addr, datavp);
> + break;
> +
> default:
> break;
> }
> --
> ldv
--
Kees Cook
Hi Dmitry,
On Mon, Dec 10, 2018 at 07:09:40PM +0300, Dmitry V. Levin wrote:
> We decided to add .frame_pointer to struct ptrace_syscall_info just for
> consistency with .instruction_pointer and .stack_pointer; I must have been
> misled by comments in asm-generic/ptrace.h into thinking that
> frame_pointer() is universally available across architectures.
Is it correct to say that you're using frame_pointer() purely on user
register state, not kernel?
If so then one option would be to define it for MIPS as something like:
static inline unsigned long frame_pointer(struct pt_regs *regs)
{
return regs->regs[30];
}
My concern with that though would be that providing frame_pointer()
unconditionally might mislead people into thinking that the kernel
always has frame pointers, when in reality current MIPS kernels never
do. In fact a comment in MIPS' asm/ptrace.h seems to suggest the lack of
frame_pointer() is intentional for exactly that reason:
> Don't use asm-generic/ptrace.h it defines FP accessors that don't make
> sense on MIPS. We rather want an error if they get invoked.
Looking across architectures though MIPS isn't going to be the only one
missing frame_pointer(). With a little grepping it appears that these
architectures provide frame_pointer():
arm
arm64
hexagon
nds32
powerpc
riscv
sparc
um
x86
That leaves a whole bunch of other architectures (16) which don't have
frame_pointer(), or at least not in a way that I could see at a glance.
> Unlike .instruction_pointer and .stack_pointer that are actually needed
> in strace, .frame_pointer is not used, so from strace PoV we don't really
> need it.
>
> So the question is, does anybody need a
> struct ptrace_syscall_info.frame_pointer?
>
> If yes, how can frame_pointer() be defined on MIPS?
> Or should we just forget about making sense of frame_pointer() and remove
> struct ptrace_syscall_info.frame_pointer from the proposed API?
So, along these lines my suggestion would be to avoid it if you don't
really need it anyway.
Thanks,
Paul
> On Dec 10, 2018, at 8:09 AM, Dmitry V. Levin <[email protected]> wrote:
>
> Hi, things are getting too complicated and we need some advice how to deal
> with this frame_pointer issue.
>
>> On Mon, Dec 10, 2018 at 10:26:50PM +0800, kbuild test robot wrote:
>> Hi Elvira,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on linus/master]
>> [also build test ERROR on v4.20-rc6]
>> [cannot apply to next-20181207]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url: https://github.com/0day-ci/linux/commits/Dmitry-V-Levin/ptrace-add-PTRACE_GET_SYSCALL_INFO-request/20181210-174745
>> config: mips-malta_kvm_defconfig (attached as .config)
>> compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
>> reproduce:
>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> # save the attached .config to linux build tree
>> GCC_VERSION=7.2.0 make.cross ARCH=mips
>>
>> All errors (new ones prefixed by >>):
>>
>> kernel/ptrace.c: In function 'ptrace_get_syscall_info':
>>>> kernel/ptrace.c:942:20: error: implicit declaration of function 'frame_pointer'; did you mean 'trace_printk'? [-Werror=implicit-function-declaration]
>> .frame_pointer = frame_pointer(regs)
>> ^~~~~~~~~~~~~
>> trace_printk
>> cc1: some warnings being treated as errors
>>
>> vim +942 kernel/ptrace.c
>>
>> 931
>> 932 static int
>> 933 ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
>> 934 void __user *datavp)
>> 935 {
>> 936 struct pt_regs *regs = task_pt_regs(child);
>> 937 struct ptrace_syscall_info info = {
>> 938 .op = PTRACE_SYSCALL_INFO_NONE,
>> 939 .arch = syscall_get_arch(child),
>> 940 .instruction_pointer = instruction_pointer(regs),
>> 941 .stack_pointer = user_stack_pointer(regs),
>>> 942 .frame_pointer = frame_pointer(regs)
>> 943 };
>> 944 unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
>> 945 unsigned long write_size;
>> 946
>> 947 /*
>> 948 * This does not need lock_task_sighand() to access
>> 949 * child->last_siginfo because ptrace_freeze_traced()
>> 950 * called earlier by ptrace_check_attach() ensures that
>> 951 * the tracee cannot go away and clear its last_siginfo.
>> 952 */
>> 953 switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
>> 954 case SIGTRAP | 0x80:
>> 955 switch (child->ptrace_message) {
>> 956 case PTRACE_EVENTMSG_SYSCALL_ENTRY:
>> 957 actual_size = ptrace_get_syscall_info_entry(child, regs,
>> 958 &info);
>> 959 break;
>> 960 case PTRACE_EVENTMSG_SYSCALL_EXIT:
>> 961 actual_size = ptrace_get_syscall_info_exit(child, regs,
>> 962 &info);
>> 963 break;
>> 964 }
>> 965 break;
>> 966 case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
>> 967 actual_size = ptrace_get_syscall_info_seccomp(child, regs,
>> 968 &info);
>> 969 break;
>> 970 }
>> 971
>> 972 write_size = min(actual_size, user_size);
>> 973 return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
>> 974 }
>> 975
>
> We decided to add .frame_pointer to struct ptrace_syscall_info just for
> consistency with .instruction_pointer and .stack_pointer; I must have been
> misled by comments in asm-generic/ptrace.h into thinking that
> frame_pointer() is universally available across architectures.
>
> Unlike .instruction_pointer and .stack_pointer that are actually needed
> in strace, .frame_pointer is not used, so from strace PoV we don't really
> need it.
>
> So the question is, does anybody need a
> struct ptrace_syscall_info.frame_pointer?
>
> If yes, how can frame_pointer() be defined on MIPS?
> Or should we just forget about making sense of frame_pointer() and remove
> struct ptrace_syscall_info.frame_pointer from the proposed API?
>
I would suggest getting rid of frame_pointer. Anyone who needs that
degree of debugging can use existing ptrace APIs for it.
>
> --
> ldv
On Mon, 10 Dec 2018 10:04:22 PST (-0800), [email protected] wrote:
> Hi Dmitry,
>
> On Mon, Dec 10, 2018 at 07:09:40PM +0300, Dmitry V. Levin wrote:
>> We decided to add .frame_pointer to struct ptrace_syscall_info just for
>> consistency with .instruction_pointer and .stack_pointer; I must have been
>> misled by comments in asm-generic/ptrace.h into thinking that
>> frame_pointer() is universally available across architectures.
>
> Is it correct to say that you're using frame_pointer() purely on user
> register state, not kernel?
>
> If so then one option would be to define it for MIPS as something like:
>
> static inline unsigned long frame_pointer(struct pt_regs *regs)
> {
> return regs->regs[30];
> }
>
> My concern with that though would be that providing frame_pointer()
> unconditionally might mislead people into thinking that the kernel
> always has frame pointers, when in reality current MIPS kernels never
> do. In fact a comment in MIPS' asm/ptrace.h seems to suggest the lack of
> frame_pointer() is intentional for exactly that reason:
>
>> Don't use asm-generic/ptrace.h it defines FP accessors that don't make
>> sense on MIPS. We rather want an error if they get invoked.
>
> Looking across architectures though MIPS isn't going to be the only one
> missing frame_pointer(). With a little grepping it appears that these
> architectures provide frame_pointer():
>
> arm
> arm64
> hexagon
> nds32
> powerpc
> riscv
> sparc
> um
> x86
>
> That leaves a whole bunch of other architectures (16) which don't have
> frame_pointer(), or at least not in a way that I could see at a glance.
We (RISC-V) default to compiling without frame pointers. I'm not sure if it
even makes sense have frame_pointer() on RISC-V, as it'll usually return
garbage.
>> Unlike .instruction_pointer and .stack_pointer that are actually needed
>> in strace, .frame_pointer is not used, so from strace PoV we don't really
>> need it.
>>
>> So the question is, does anybody need a
>> struct ptrace_syscall_info.frame_pointer?
>>
>> If yes, how can frame_pointer() be defined on MIPS?
>> Or should we just forget about making sense of frame_pointer() and remove
>> struct ptrace_syscall_info.frame_pointer from the proposed API?
>
> So, along these lines my suggestion would be to avoid it if you don't
> really need it anyway.
>
> Thanks,
> Paul
On 12/10, Dmitry V. Levin wrote:
>
> On Mon, Dec 10, 2018 at 03:11:07PM +0100, Oleg Nesterov wrote:
> > On 12/10, Dmitry V. Levin wrote:
> > >
> > > +struct ptrace_syscall_info {
> > > + __u8 op; /* PTRACE_SYSCALL_INFO_* */
> > > + __u8 __pad0[3];
> > > + __u32 arch;
> > > + __u64 instruction_pointer;
> > > + __u64 stack_pointer;
> > > + __u64 frame_pointer;
> > > + union {
> > > + struct {
> > > + __u64 nr;
> > > + __u64 args[6];
> > > + } entry;
> > > + struct {
> > > + __s64 rval;
> > > + __u8 is_error;
> > > + __u8 __pad1[7];
> > > + } exit;
> > > + struct {
> > > + __u64 nr;
> > > + __u64 args[6];
> > > + __u32 ret_data;
> > > + __u8 __pad2[4];
> > > + } seccomp;
> > > + };
> > > +};
> >
> > Could you explain why ptrace_syscall_info needs __pad{0,1,2} ? I simply can't
> > understand why...
>
> I suppose the idea behind the use of these pads was to make the structure
> arch-independent.
Still can't understand... are you saying that without (say) __pad2[4]
sizeof(ptrace_syscall_info) or offsetofend(ptrace_syscall_info, seccomp)
will depend on arch? Or what? I am just curious.
> I don't think we really need to keep it exactly the same on all
> architectures - the only practical requirement is to avoid any compat
> issues, but I don't mind keeping the structure arch-independent.
OK, but may be you can add a short comment to explain these pads.
Oleg.
On Tue, Dec 11, 2018 at 04:29:54PM +0100, Oleg Nesterov wrote:
> On 12/10, Dmitry V. Levin wrote:
> > On Mon, Dec 10, 2018 at 03:11:07PM +0100, Oleg Nesterov wrote:
> > > On 12/10, Dmitry V. Levin wrote:
> > > >
> > > > +struct ptrace_syscall_info {
> > > > + __u8 op; /* PTRACE_SYSCALL_INFO_* */
> > > > + __u8 __pad0[3];
> > > > + __u32 arch;
> > > > + __u64 instruction_pointer;
> > > > + __u64 stack_pointer;
> > > > + __u64 frame_pointer;
> > > > + union {
> > > > + struct {
> > > > + __u64 nr;
> > > > + __u64 args[6];
> > > > + } entry;
> > > > + struct {
> > > > + __s64 rval;
> > > > + __u8 is_error;
> > > > + __u8 __pad1[7];
> > > > + } exit;
> > > > + struct {
> > > > + __u64 nr;
> > > > + __u64 args[6];
> > > > + __u32 ret_data;
> > > > + __u8 __pad2[4];
> > > > + } seccomp;
> > > > + };
> > > > +};
> > >
> > > Could you explain why ptrace_syscall_info needs __pad{0,1,2} ? I simply can't
> > > understand why...
> >
> > I suppose the idea behind the use of these pads was to make the structure
> > arch-independent.
>
> Still can't understand... are you saying that without (say) __pad2[4]
> sizeof(ptrace_syscall_info) or offsetofend(ptrace_syscall_info, seccomp)
> will depend on arch? Or what? I am just curious.
Yes, without padding these sizes will depend on architecture:
$ cat t.c
#include <linux/types.h>
int main() {
struct s {
__u64 nr;
__u64 args[6];
__u32 ret_data;
};
return sizeof(struct s);
}
$ gcc -m64 -Wall -O2 t.c && ./a.out; echo $?
64
$ gcc -m32 -Wall -O2 t.c && ./a.out; echo $?
60
This happens because __u64 has 32-bit alignment on some 32-bit
architectures like x86.
There is also m68k where __u32 has 16-bit alignment.
> > I don't think we really need to keep it exactly the same on all
> > architectures - the only practical requirement is to avoid any compat
> > issues, but I don't mind keeping the structure arch-independent.
>
> OK, but may be you can add a short comment to explain these pads.
Alternatively, we could use __attribute__((aligned(N))), e.g.
struct ptrace_syscall_info {
__u8 op; /* PTRACE_SYSCALL_INFO_* */
__u32 arch __attribute__((aligned(4)));
__u64 instruction_pointer;
__u64 stack_pointer;
union {
struct {
__u64 nr __attribute__((aligned(8)));
__u64 args[6];
} entry;
struct {
__s64 rval __attribute__((aligned(8)));
__u8 is_error;
} exit;
struct {
__u64 nr __attribute__((aligned(8)));
__u64 args[6];
__u32 ret_data;
} seccomp;
};
};
Do you prefer __attribute__((aligned(N))) to padding?
--
ldv
On Tue, Dec 11, 2018 at 07:23:05PM +0300, Dmitry V. Levin wrote:
> On Tue, Dec 11, 2018 at 04:29:54PM +0100, Oleg Nesterov wrote:
> > On 12/10, Dmitry V. Levin wrote:
> > > On Mon, Dec 10, 2018 at 03:11:07PM +0100, Oleg Nesterov wrote:
> > > > On 12/10, Dmitry V. Levin wrote:
> > > > >
> > > > > +struct ptrace_syscall_info {
> > > > > + __u8 op; /* PTRACE_SYSCALL_INFO_* */
> > > > > + __u8 __pad0[3];
> > > > > + __u32 arch;
> > > > > + __u64 instruction_pointer;
> > > > > + __u64 stack_pointer;
> > > > > + __u64 frame_pointer;
> > > > > + union {
> > > > > + struct {
> > > > > + __u64 nr;
> > > > > + __u64 args[6];
> > > > > + } entry;
> > > > > + struct {
> > > > > + __s64 rval;
> > > > > + __u8 is_error;
> > > > > + __u8 __pad1[7];
> > > > > + } exit;
> > > > > + struct {
> > > > > + __u64 nr;
> > > > > + __u64 args[6];
> > > > > + __u32 ret_data;
> > > > > + __u8 __pad2[4];
> > > > > + } seccomp;
> > > > > + };
> > > > > +};
> > > >
> > > > Could you explain why ptrace_syscall_info needs __pad{0,1,2} ? I simply can't
> > > > understand why...
> > >
> > > I suppose the idea behind the use of these pads was to make the structure
> > > arch-independent.
> >
> > Still can't understand... are you saying that without (say) __pad2[4]
> > sizeof(ptrace_syscall_info) or offsetofend(ptrace_syscall_info, seccomp)
> > will depend on arch? Or what? I am just curious.
>
> Yes, without padding these sizes will depend on architecture:
>
> $ cat t.c
> #include <linux/types.h>
> int main() {
> struct s {
> __u64 nr;
> __u64 args[6];
> __u32 ret_data;
> };
> return sizeof(struct s);
> }
>
> $ gcc -m64 -Wall -O2 t.c && ./a.out; echo $?
> 64
> $ gcc -m32 -Wall -O2 t.c && ./a.out; echo $?
> 60
>
> This happens because __u64 has 32-bit alignment on some 32-bit
> architectures like x86.
>
> There is also m68k where __u32 has 16-bit alignment.
Said that, I think it would be better if PTRACE_GET_SYSCALL_INFO
did not take these trailing pads into account, e.g.
- return offsetofend(struct ptrace_syscall_info, seccomp);
+ return offsetofend(struct ptrace_syscall_info, seccomp.ret_data);
...
- return offsetofend(struct ptrace_syscall_info, exit);
+ return offsetofend(struct ptrace_syscall_info, exit.is_error);
The reason is that it would allow to fill these trailing pads with
something useful in the future.
--
ldv
Hi Elvira,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc6]
[cannot apply to next-20181211]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-V-Levin/ptrace-add-PTRACE_GET_SYSCALL_INFO-request/20181210-174745
config: nds32-allmodconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 6.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=6.4.0 make.cross ARCH=nds32
All errors (new ones prefixed by >>):
kernel/ptrace.c: In function 'ptrace_get_syscall_info':
>> kernel/ptrace.c:942:20: error: implicit declaration of function 'frame_pointer' [-Werror=implicit-function-declaration]
.frame_pointer = frame_pointer(regs)
^~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/frame_pointer +942 kernel/ptrace.c
931
932 static int
933 ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
934 void __user *datavp)
935 {
936 struct pt_regs *regs = task_pt_regs(child);
937 struct ptrace_syscall_info info = {
938 .op = PTRACE_SYSCALL_INFO_NONE,
939 .arch = syscall_get_arch(child),
940 .instruction_pointer = instruction_pointer(regs),
941 .stack_pointer = user_stack_pointer(regs),
> 942 .frame_pointer = frame_pointer(regs)
943 };
944 unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
945 unsigned long write_size;
946
947 /*
948 * This does not need lock_task_sighand() to access
949 * child->last_siginfo because ptrace_freeze_traced()
950 * called earlier by ptrace_check_attach() ensures that
951 * the tracee cannot go away and clear its last_siginfo.
952 */
953 switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
954 case SIGTRAP | 0x80:
955 switch (child->ptrace_message) {
956 case PTRACE_EVENTMSG_SYSCALL_ENTRY:
957 actual_size = ptrace_get_syscall_info_entry(child, regs,
958 &info);
959 break;
960 case PTRACE_EVENTMSG_SYSCALL_EXIT:
961 actual_size = ptrace_get_syscall_info_exit(child, regs,
962 &info);
963 break;
964 }
965 break;
966 case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
967 actual_size = ptrace_get_syscall_info_seccomp(child, regs,
968 &info);
969 break;
970 }
971
972 write_size = min(actual_size, user_size);
973 return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
974 }
975
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 12/11, Dmitry V. Levin wrote:
>
> > > Still can't understand... are you saying that without (say) __pad2[4]
> > > sizeof(ptrace_syscall_info) or offsetofend(ptrace_syscall_info, seccomp)
> > > will depend on arch? Or what? I am just curious.
> >
> > Yes, without padding these sizes will depend on architecture:
>
> > $ cat t.c
> > #include <linux/types.h>
> > int main() {
> > struct s {
> > __u64 nr;
> > __u64 args[6];
> > __u32 ret_data;
> > };
> > return sizeof(struct s);
> > }
> >
> > $ gcc -m64 -Wall -O2 t.c && ./a.out; echo $?
> > 64
> > $ gcc -m32 -Wall -O2 t.c && ./a.out; echo $?
> > 60
> >
> > This happens because __u64 has 32-bit alignment on some 32-bit
> > architectures like x86.
> >
> > There is also m68k where __u32 has 16-bit alignment.
OK, thanks,
> Said that, I think it would be better if PTRACE_GET_SYSCALL_INFO
> did not take these trailing pads into account, e.g.
>
> - return offsetofend(struct ptrace_syscall_info, seccomp);
> + return offsetofend(struct ptrace_syscall_info, seccomp.ret_data);
> ...
> - return offsetofend(struct ptrace_syscall_info, exit);
> + return offsetofend(struct ptrace_syscall_info, exit.is_error);
>
> The reason is that it would allow to fill these trailing pads with
> something useful in the future.
Agreed.
But this way everything looks even more confusing. To me it would be
better to simply remove these pads, but I won't insist.
Oleg.