* [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
@ 2020-07-27 21:36 Peilin Ye
2020-08-01 0:21 ` Dmitry V. Levin
2020-08-01 2:08 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
0 siblings, 2 replies; 9+ messages in thread
From: Peilin Ye @ 2020-07-27 21:36 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Peilin Ye, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel-mentees, linux-kernel
ptrace_get_syscall_info() is copying uninitialized stack memory to
userspace due to the compiler not initializing holes in statically
allocated structures. Fix it by initializing `info` with memset().
Cc: stable@vger.kernel.org
Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
kernel/ptrace.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 43d6179508d6..e48d05b765b5 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -960,15 +960,17 @@ 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),
- };
+ struct ptrace_syscall_info info;
unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
unsigned long write_size;
+ memset(&info, 0, sizeof(info));
+
+ info.op = PTRACE_SYSCALL_INFO_NONE;
+ info.arch = syscall_get_arch(child);
+ info.instruction_pointer = instruction_pointer(regs);
+ info.stack_pointer = user_stack_pointer(regs);
+
/*
* This does not need lock_task_sighand() to access
* child->last_siginfo because ptrace_freeze_traced()
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() 2020-07-27 21:36 [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() Peilin Ye @ 2020-08-01 0:21 ` Dmitry V. Levin 2020-08-01 1:28 ` Peilin Ye 2020-08-01 2:08 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye 1 sibling, 1 reply; 9+ messages in thread From: Dmitry V. Levin @ 2020-08-01 0:21 UTC (permalink / raw) To: Peilin Ye Cc: Elvira Khabirova, Oleg Nesterov, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, linux-kernel On Mon, Jul 27, 2020 at 05:36:44PM -0400, Peilin Ye wrote: > ptrace_get_syscall_info() is copying uninitialized stack memory to > userspace due to the compiler not initializing holes in statically > allocated structures. Fix it by initializing `info` with memset(). > > Cc: stable@vger.kernel.org > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request") > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > --- > kernel/ptrace.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > index 43d6179508d6..e48d05b765b5 100644 > --- a/kernel/ptrace.c > +++ b/kernel/ptrace.c > @@ -960,15 +960,17 @@ 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), > - }; > + struct ptrace_syscall_info info; > unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry); > unsigned long write_size; > > + memset(&info, 0, sizeof(info)); > + > + info.op = PTRACE_SYSCALL_INFO_NONE; > + info.arch = syscall_get_arch(child); > + info.instruction_pointer = instruction_pointer(regs); > + info.stack_pointer = user_stack_pointer(regs); > + No, please don't do it this way. If there is a hole in the structure that the compiler is unable to initialize properly (and there is a 3-byte hole in the beginning indeed), please plug the hole by turning it into something that the compiler is capable of initializing. Also, please do not forget to Cc authors of the commit you are fixing. -- ldv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() 2020-08-01 0:21 ` Dmitry V. Levin @ 2020-08-01 1:28 ` Peilin Ye 0 siblings, 0 replies; 9+ messages in thread From: Peilin Ye @ 2020-08-01 1:28 UTC (permalink / raw) To: Dmitry V. Levin Cc: Elvira Khabirova, Oleg Nesterov, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, linux-kernel On Sat, Aug 01, 2020 at 03:21:42AM +0300, Dmitry V. Levin wrote: > On Mon, Jul 27, 2020 at 05:36:44PM -0400, Peilin Ye wrote: > > ptrace_get_syscall_info() is copying uninitialized stack memory to > > userspace due to the compiler not initializing holes in statically > > allocated structures. Fix it by initializing `info` with memset(). > > > > Cc: stable@vger.kernel.org > > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request") > > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > > --- > > kernel/ptrace.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > > index 43d6179508d6..e48d05b765b5 100644 > > --- a/kernel/ptrace.c > > +++ b/kernel/ptrace.c > > @@ -960,15 +960,17 @@ 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), > > - }; > > + struct ptrace_syscall_info info; > > unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry); > > unsigned long write_size; > > > > + memset(&info, 0, sizeof(info)); > > + > > + info.op = PTRACE_SYSCALL_INFO_NONE; > > + info.arch = syscall_get_arch(child); > > + info.instruction_pointer = instruction_pointer(regs); > > + info.stack_pointer = user_stack_pointer(regs); > > + > > No, please don't do it this way. If there is a hole in the structure that > the compiler is unable to initialize properly (and there is a 3-byte hole > in the beginning indeed), please plug the hole by turning it into > something that the compiler is capable of initializing. I see. I will do that and send a v2. > Also, please do not forget to Cc authors of the commit you are fixing. Sorry, I forgot about that. Thank you for pointing it out! Peilin Ye ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Linux-kernel-mentees] [PATCH v2] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() 2020-07-27 21:36 [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() Peilin Ye 2020-08-01 0:21 ` Dmitry V. Levin @ 2020-08-01 2:08 ` Peilin Ye 2020-08-01 11:06 ` Dmitry V. Levin 2020-08-01 15:20 ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye 1 sibling, 2 replies; 9+ messages in thread From: Peilin Ye @ 2020-08-01 2:08 UTC (permalink / raw) To: Oleg Nesterov Cc: Peilin Ye, Dmitry V. Levin, Elvira Khabirova, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, linux-kernel ptrace_get_syscall_info() is potentially copying uninitialized stack memory to userspace, since the compiler may leave a 3-byte hole near the beginning of `info`. Fix it by adding a padding field to `struct ptrace_syscall_info`. Cc: stable@vger.kernel.org Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request") Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> --- Change in v2: - Add a padding field to `struct ptrace_syscall_info`, instead of doing memset() on `info`. (Suggested by Dmitry V. Levin <ldv@altlinux.org>) Reference: https://lwn.net/Articles/417989/ $ # before: $ pahole -C "ptrace_syscall_info" kernel/ptrace.o struct ptrace_syscall_info { __u8 op; /* 0 1 */ /* XXX 3 bytes hole, try to pack */ __u32 arch __attribute__((__aligned__(4))); /* 4 4 */ __u64 instruction_pointer; /* 8 8 */ __u64 stack_pointer; /* 16 8 */ union { struct { __u64 nr; /* 24 8 */ __u64 args[6]; /* 32 48 */ } entry; /* 24 56 */ struct { __s64 rval; /* 24 8 */ __u8 is_error; /* 32 1 */ } exit; /* 24 16 */ struct { __u64 nr; /* 24 8 */ __u64 args[6]; /* 32 48 */ /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ __u32 ret_data; /* 80 4 */ } seccomp; /* 24 64 */ }; /* 24 64 */ /* size: 88, cachelines: 2, members: 5 */ /* sum members: 85, holes: 1, sum holes: 3 */ /* forced alignments: 1, forced holes: 1, sum forced holes: 3 */ /* last cacheline: 24 bytes */ } __attribute__((__aligned__(8))); $ $ # after: $ pahole -C "ptrace_syscall_info" kernel/ptrace.o struct ptrace_syscall_info { __u8 op; /* 0 1 */ __u8 pad[3]; /* 1 3 */ __u32 arch __attribute__((__aligned__(4))); /* 4 4 */ __u64 instruction_pointer; /* 8 8 */ __u64 stack_pointer; /* 16 8 */ union { struct { __u64 nr; /* 24 8 */ __u64 args[6]; /* 32 48 */ } entry; /* 24 56 */ struct { __s64 rval; /* 24 8 */ __u8 is_error; /* 32 1 */ } exit; /* 24 16 */ struct { __u64 nr; /* 24 8 */ __u64 args[6]; /* 32 48 */ /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ __u32 ret_data; /* 80 4 */ } seccomp; /* 24 64 */ }; /* 24 64 */ /* size: 88, cachelines: 2, members: 6 */ /* forced alignments: 1 */ /* last cacheline: 24 bytes */ } __attribute__((__aligned__(8))); $ _ include/uapi/linux/ptrace.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h index a71b6e3b03eb..a518ba514bac 100644 --- a/include/uapi/linux/ptrace.h +++ b/include/uapi/linux/ptrace.h @@ -81,6 +81,7 @@ struct seccomp_metadata { struct ptrace_syscall_info { __u8 op; /* PTRACE_SYSCALL_INFO_* */ + __u8 pad[3]; __u32 arch __attribute__((__aligned__(sizeof(__u32)))); __u64 instruction_pointer; __u64 stack_pointer; -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() 2020-08-01 2:08 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye @ 2020-08-01 11:06 ` Dmitry V. Levin 2020-08-01 15:09 ` Peilin Ye 2020-08-01 15:20 ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye 1 sibling, 1 reply; 9+ messages in thread From: Dmitry V. Levin @ 2020-08-01 11:06 UTC (permalink / raw) To: Peilin Ye Cc: Oleg Nesterov, Elvira Khabirova, Eugene Syromyatnikov, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, linux-kernel On Fri, Jul 31, 2020 at 10:08:41PM -0400, Peilin Ye wrote: > ptrace_get_syscall_info() is potentially copying uninitialized stack > memory to userspace, since the compiler may leave a 3-byte hole near the > beginning of `info`. Fix it by adding a padding field to `struct > ptrace_syscall_info`. > > Cc: stable@vger.kernel.org > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request") > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > --- > Change in v2: > - Add a padding field to `struct ptrace_syscall_info`, instead of > doing memset() on `info`. (Suggested by Dmitry V. Levin > <ldv@altlinux.org>) > > Reference: https://lwn.net/Articles/417989/ > > $ # before: > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o > struct ptrace_syscall_info { > __u8 op; /* 0 1 */ > > /* XXX 3 bytes hole, try to pack */ > > __u32 arch __attribute__((__aligned__(4))); /* 4 4 */ > __u64 instruction_pointer; /* 8 8 */ > __u64 stack_pointer; /* 16 8 */ > union { > struct { > __u64 nr; /* 24 8 */ > __u64 args[6]; /* 32 48 */ > } entry; /* 24 56 */ > struct { > __s64 rval; /* 24 8 */ > __u8 is_error; /* 32 1 */ > } exit; /* 24 16 */ > struct { > __u64 nr; /* 24 8 */ > __u64 args[6]; /* 32 48 */ > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ > __u32 ret_data; /* 80 4 */ > } seccomp; /* 24 64 */ > }; /* 24 64 */ > > /* size: 88, cachelines: 2, members: 5 */ > /* sum members: 85, holes: 1, sum holes: 3 */ > /* forced alignments: 1, forced holes: 1, sum forced holes: 3 */ > /* last cacheline: 24 bytes */ > } __attribute__((__aligned__(8))); > $ > $ # after: > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o > struct ptrace_syscall_info { > __u8 op; /* 0 1 */ > __u8 pad[3]; /* 1 3 */ > __u32 arch __attribute__((__aligned__(4))); /* 4 4 */ > __u64 instruction_pointer; /* 8 8 */ > __u64 stack_pointer; /* 16 8 */ > union { > struct { > __u64 nr; /* 24 8 */ > __u64 args[6]; /* 32 48 */ > } entry; /* 24 56 */ > struct { > __s64 rval; /* 24 8 */ > __u8 is_error; /* 32 1 */ > } exit; /* 24 16 */ > struct { > __u64 nr; /* 24 8 */ > __u64 args[6]; /* 32 48 */ > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ > __u32 ret_data; /* 80 4 */ > } seccomp; /* 24 64 */ > }; /* 24 64 */ > > /* size: 88, cachelines: 2, members: 6 */ > /* forced alignments: 1 */ > /* last cacheline: 24 bytes */ > } __attribute__((__aligned__(8))); > $ _ > > include/uapi/linux/ptrace.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h > index a71b6e3b03eb..a518ba514bac 100644 > --- a/include/uapi/linux/ptrace.h > +++ b/include/uapi/linux/ptrace.h > @@ -81,6 +81,7 @@ struct seccomp_metadata { > > struct ptrace_syscall_info { > __u8 op; /* PTRACE_SYSCALL_INFO_* */ > + __u8 pad[3]; > __u32 arch __attribute__((__aligned__(sizeof(__u32)))); > __u64 instruction_pointer; > __u64 stack_pointer; Funnily enough, but in first editions of PTRACE_GET_SYSCALL_INFO patchset [1] this was looking very similar: +struct ptrace_syscall_info { + __u8 op; /* PTRACE_SYSCALL_INFO_* */ + __u8 __pad0[3]; + __u32 arch; But later we decided [2][3] to replace the pad with a hole. Note that the sole purpose of the __aligned__ attribute on the field that follows the hole is to guarantee that the hole has the same size across architectures. As this hole is being replaced back with a pad, that __aligned__ attribute is no longer needed and can be omitted along with adding the pad. [1] https://lore.kernel.org/linux-api/20181125022150.46258a20@akathisia/ [2] https://lore.kernel.org/linux-api/20181211162305.GA480@altlinux.org/ [3] https://lore.kernel.org/linux-api/20181213171833.GA5240@altlinux.org/ -- ldv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() 2020-08-01 11:06 ` Dmitry V. Levin @ 2020-08-01 15:09 ` Peilin Ye 0 siblings, 0 replies; 9+ messages in thread From: Peilin Ye @ 2020-08-01 15:09 UTC (permalink / raw) To: Dmitry V. Levin Cc: Oleg Nesterov, Elvira Khabirova, Eugene Syromyatnikov, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, linux-kernel On Sat, Aug 01, 2020 at 02:06:46PM +0300, Dmitry V. Levin wrote: > On Fri, Jul 31, 2020 at 10:08:41PM -0400, Peilin Ye wrote: > > ptrace_get_syscall_info() is potentially copying uninitialized stack > > memory to userspace, since the compiler may leave a 3-byte hole near the > > beginning of `info`. Fix it by adding a padding field to `struct > > ptrace_syscall_info`. > > > > Cc: stable@vger.kernel.org > > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request") > > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > > --- > > Change in v2: > > - Add a padding field to `struct ptrace_syscall_info`, instead of > > doing memset() on `info`. (Suggested by Dmitry V. Levin > > <ldv@altlinux.org>) > > > > Reference: https://lwn.net/Articles/417989/ > > > > $ # before: > > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o > > struct ptrace_syscall_info { > > __u8 op; /* 0 1 */ > > > > /* XXX 3 bytes hole, try to pack */ > > > > __u32 arch __attribute__((__aligned__(4))); /* 4 4 */ > > __u64 instruction_pointer; /* 8 8 */ > > __u64 stack_pointer; /* 16 8 */ > > union { > > struct { > > __u64 nr; /* 24 8 */ > > __u64 args[6]; /* 32 48 */ > > } entry; /* 24 56 */ > > struct { > > __s64 rval; /* 24 8 */ > > __u8 is_error; /* 32 1 */ > > } exit; /* 24 16 */ > > struct { > > __u64 nr; /* 24 8 */ > > __u64 args[6]; /* 32 48 */ > > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ > > __u32 ret_data; /* 80 4 */ > > } seccomp; /* 24 64 */ > > }; /* 24 64 */ > > > > /* size: 88, cachelines: 2, members: 5 */ > > /* sum members: 85, holes: 1, sum holes: 3 */ > > /* forced alignments: 1, forced holes: 1, sum forced holes: 3 */ > > /* last cacheline: 24 bytes */ > > } __attribute__((__aligned__(8))); > > $ > > $ # after: > > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o > > struct ptrace_syscall_info { > > __u8 op; /* 0 1 */ > > __u8 pad[3]; /* 1 3 */ > > __u32 arch __attribute__((__aligned__(4))); /* 4 4 */ > > __u64 instruction_pointer; /* 8 8 */ > > __u64 stack_pointer; /* 16 8 */ > > union { > > struct { > > __u64 nr; /* 24 8 */ > > __u64 args[6]; /* 32 48 */ > > } entry; /* 24 56 */ > > struct { > > __s64 rval; /* 24 8 */ > > __u8 is_error; /* 32 1 */ > > } exit; /* 24 16 */ > > struct { > > __u64 nr; /* 24 8 */ > > __u64 args[6]; /* 32 48 */ > > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ > > __u32 ret_data; /* 80 4 */ > > } seccomp; /* 24 64 */ > > }; /* 24 64 */ > > > > /* size: 88, cachelines: 2, members: 6 */ > > /* forced alignments: 1 */ > > /* last cacheline: 24 bytes */ > > } __attribute__((__aligned__(8))); > > $ _ > > > > include/uapi/linux/ptrace.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h > > index a71b6e3b03eb..a518ba514bac 100644 > > --- a/include/uapi/linux/ptrace.h > > +++ b/include/uapi/linux/ptrace.h > > @@ -81,6 +81,7 @@ struct seccomp_metadata { > > > > struct ptrace_syscall_info { > > __u8 op; /* PTRACE_SYSCALL_INFO_* */ > > + __u8 pad[3]; > > __u32 arch __attribute__((__aligned__(sizeof(__u32)))); > > __u64 instruction_pointer; > > __u64 stack_pointer; > > Funnily enough, but in first editions of PTRACE_GET_SYSCALL_INFO > patchset [1] this was looking very similar: > > +struct ptrace_syscall_info { > + __u8 op; /* PTRACE_SYSCALL_INFO_* */ > + __u8 __pad0[3]; > + __u32 arch; > > But later we decided [2][3] to replace the pad with a hole. > > Note that the sole purpose of the __aligned__ attribute on the field that > follows the hole is to guarantee that the hole has the same size across > architectures. As this hole is being replaced back with a pad, that > __aligned__ attribute is no longer needed and can be omitted along with > adding the pad. Ah, I see. I will remove that in v3. Thank you, Peilin Ye > [1] https://lore.kernel.org/linux-api/20181125022150.46258a20@akathisia/ > [2] https://lore.kernel.org/linux-api/20181211162305.GA480@altlinux.org/ > [3] https://lore.kernel.org/linux-api/20181213171833.GA5240@altlinux.org/ > > > -- > ldv ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() 2020-08-01 2:08 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye 2020-08-01 11:06 ` Dmitry V. Levin @ 2020-08-01 15:20 ` Peilin Ye 2020-08-01 16:08 ` Dmitry V. Levin 1 sibling, 1 reply; 9+ messages in thread From: Peilin Ye @ 2020-08-01 15:20 UTC (permalink / raw) To: Oleg Nesterov Cc: Peilin Ye, Dmitry V. Levin, Elvira Khabirova, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, linux-kernel ptrace_get_syscall_info() is potentially copying uninitialized stack memory to userspace, since the compiler may leave a 3-byte hole near the beginning of `info`. Fix it by adding a padding field to `struct ptrace_syscall_info`. Cc: stable@vger.kernel.org Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request") Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> --- Change in v3: - Remove unnecessary `__aligned__` attribute. (Suggested by Dmitry V. Levin <ldv@altlinux.org>) Change in v2: - Add a padding field to `struct ptrace_syscall_info`, instead of doing memset() on `info`. (Suggested by Dmitry V. Levin <ldv@altlinux.org>) Reference: https://lwn.net/Articles/417989/ $ # before: $ pahole -C "ptrace_syscall_info" kernel/ptrace.o struct ptrace_syscall_info { __u8 op; /* 0 1 */ /* XXX 3 bytes hole, try to pack */ __u32 arch __attribute__((__aligned__(4))); /* 4 4 */ __u64 instruction_pointer; /* 8 8 */ __u64 stack_pointer; /* 16 8 */ union { struct { __u64 nr; /* 24 8 */ __u64 args[6]; /* 32 48 */ } entry; /* 24 56 */ struct { __s64 rval; /* 24 8 */ __u8 is_error; /* 32 1 */ } exit; /* 24 16 */ struct { __u64 nr; /* 24 8 */ __u64 args[6]; /* 32 48 */ /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ __u32 ret_data; /* 80 4 */ } seccomp; /* 24 64 */ }; /* 24 64 */ /* size: 88, cachelines: 2, members: 5 */ /* sum members: 85, holes: 1, sum holes: 3 */ /* forced alignments: 1, forced holes: 1, sum forced holes: 3 */ /* last cacheline: 24 bytes */ } __attribute__((__aligned__(8))); $ $ # after: $ pahole -C "ptrace_syscall_info" kernel/ptrace.o struct ptrace_syscall_info { __u8 op; /* 0 1 */ __u8 pad[3]; /* 1 3 */ __u32 arch; /* 4 4 */ __u64 instruction_pointer; /* 8 8 */ __u64 stack_pointer; /* 16 8 */ union { struct { __u64 nr; /* 24 8 */ __u64 args[6]; /* 32 48 */ } entry; /* 24 56 */ struct { __s64 rval; /* 24 8 */ __u8 is_error; /* 32 1 */ } exit; /* 24 16 */ struct { __u64 nr; /* 24 8 */ __u64 args[6]; /* 32 48 */ /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ __u32 ret_data; /* 80 4 */ } seccomp; /* 24 64 */ }; /* 24 64 */ /* size: 88, cachelines: 2, members: 6 */ /* last cacheline: 24 bytes */ }; $ _ include/uapi/linux/ptrace.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h index a71b6e3b03eb..83ee45fa634b 100644 --- a/include/uapi/linux/ptrace.h +++ b/include/uapi/linux/ptrace.h @@ -81,7 +81,8 @@ struct seccomp_metadata { struct ptrace_syscall_info { __u8 op; /* PTRACE_SYSCALL_INFO_* */ - __u32 arch __attribute__((__aligned__(sizeof(__u32)))); + __u8 pad[3]; + __u32 arch; __u64 instruction_pointer; __u64 stack_pointer; union { -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() 2020-08-01 15:20 ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye @ 2020-08-01 16:08 ` Dmitry V. Levin 2020-08-01 20:10 ` Christian Brauner 0 siblings, 1 reply; 9+ messages in thread From: Dmitry V. Levin @ 2020-08-01 16:08 UTC (permalink / raw) To: Peilin Ye Cc: Oleg Nesterov, Elvira Khabirova, Eugene Syromyatnikov, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, linux-api, linux-kernel On Sat, Aug 01, 2020 at 11:20:44AM -0400, Peilin Ye wrote: > ptrace_get_syscall_info() is potentially copying uninitialized stack > memory to userspace, since the compiler may leave a 3-byte hole near the > beginning of `info`. Fix it by adding a padding field to `struct > ptrace_syscall_info`. > > Cc: stable@vger.kernel.org > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request") > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > --- > Change in v3: > - Remove unnecessary `__aligned__` attribute. (Suggested by > Dmitry V. Levin <ldv@altlinux.org>) > > Change in v2: > - Add a padding field to `struct ptrace_syscall_info`, instead of > doing memset() on `info`. (Suggested by Dmitry V. Levin > <ldv@altlinux.org>) > > Reference: https://lwn.net/Articles/417989/ > > $ # before: > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o > struct ptrace_syscall_info { > __u8 op; /* 0 1 */ > > /* XXX 3 bytes hole, try to pack */ > > __u32 arch __attribute__((__aligned__(4))); /* 4 4 */ > __u64 instruction_pointer; /* 8 8 */ > __u64 stack_pointer; /* 16 8 */ > union { > struct { > __u64 nr; /* 24 8 */ > __u64 args[6]; /* 32 48 */ > } entry; /* 24 56 */ > struct { > __s64 rval; /* 24 8 */ > __u8 is_error; /* 32 1 */ > } exit; /* 24 16 */ > struct { > __u64 nr; /* 24 8 */ > __u64 args[6]; /* 32 48 */ > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ > __u32 ret_data; /* 80 4 */ > } seccomp; /* 24 64 */ > }; /* 24 64 */ > > /* size: 88, cachelines: 2, members: 5 */ > /* sum members: 85, holes: 1, sum holes: 3 */ > /* forced alignments: 1, forced holes: 1, sum forced holes: 3 */ > /* last cacheline: 24 bytes */ > } __attribute__((__aligned__(8))); > $ > $ # after: > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o > struct ptrace_syscall_info { > __u8 op; /* 0 1 */ > __u8 pad[3]; /* 1 3 */ > __u32 arch; /* 4 4 */ > __u64 instruction_pointer; /* 8 8 */ > __u64 stack_pointer; /* 16 8 */ > union { > struct { > __u64 nr; /* 24 8 */ > __u64 args[6]; /* 32 48 */ > } entry; /* 24 56 */ > struct { > __s64 rval; /* 24 8 */ > __u8 is_error; /* 32 1 */ > } exit; /* 24 16 */ > struct { > __u64 nr; /* 24 8 */ > __u64 args[6]; /* 32 48 */ > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ > __u32 ret_data; /* 80 4 */ > } seccomp; /* 24 64 */ > }; /* 24 64 */ > > /* size: 88, cachelines: 2, members: 6 */ > /* last cacheline: 24 bytes */ > }; > $ _ > > include/uapi/linux/ptrace.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h > index a71b6e3b03eb..83ee45fa634b 100644 > --- a/include/uapi/linux/ptrace.h > +++ b/include/uapi/linux/ptrace.h > @@ -81,7 +81,8 @@ struct seccomp_metadata { > > struct ptrace_syscall_info { > __u8 op; /* PTRACE_SYSCALL_INFO_* */ > - __u32 arch __attribute__((__aligned__(sizeof(__u32)))); > + __u8 pad[3]; > + __u32 arch; > __u64 instruction_pointer; > __u64 stack_pointer; > union { Reviewed-by: Dmitry V. Levin <ldv@altlinux.org> Thanks, -- ldv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() 2020-08-01 16:08 ` Dmitry V. Levin @ 2020-08-01 20:10 ` Christian Brauner 0 siblings, 0 replies; 9+ messages in thread From: Christian Brauner @ 2020-08-01 20:10 UTC (permalink / raw) To: Dmitry V. Levin Cc: Peilin Ye, Oleg Nesterov, Elvira Khabirova, Eugene Syromyatnikov, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, linux-api, linux-kernel On Sat, Aug 01, 2020 at 07:08:19PM +0300, Dmitry V. Levin wrote: > On Sat, Aug 01, 2020 at 11:20:44AM -0400, Peilin Ye wrote: > > ptrace_get_syscall_info() is potentially copying uninitialized stack > > memory to userspace, since the compiler may leave a 3-byte hole near the > > beginning of `info`. Fix it by adding a padding field to `struct > > ptrace_syscall_info`. > > > > Cc: stable@vger.kernel.org > > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request") > > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > > --- > > Change in v3: > > - Remove unnecessary `__aligned__` attribute. (Suggested by > > Dmitry V. Levin <ldv@altlinux.org>) > > > > Change in v2: > > - Add a padding field to `struct ptrace_syscall_info`, instead of > > doing memset() on `info`. (Suggested by Dmitry V. Levin > > <ldv@altlinux.org>) > > > > Reference: https://lwn.net/Articles/417989/ > > > > $ # before: > > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o > > struct ptrace_syscall_info { > > __u8 op; /* 0 1 */ > > > > /* XXX 3 bytes hole, try to pack */ > > > > __u32 arch __attribute__((__aligned__(4))); /* 4 4 */ > > __u64 instruction_pointer; /* 8 8 */ > > __u64 stack_pointer; /* 16 8 */ > > union { > > struct { > > __u64 nr; /* 24 8 */ > > __u64 args[6]; /* 32 48 */ > > } entry; /* 24 56 */ > > struct { > > __s64 rval; /* 24 8 */ > > __u8 is_error; /* 32 1 */ > > } exit; /* 24 16 */ > > struct { > > __u64 nr; /* 24 8 */ > > __u64 args[6]; /* 32 48 */ > > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ > > __u32 ret_data; /* 80 4 */ > > } seccomp; /* 24 64 */ > > }; /* 24 64 */ > > > > /* size: 88, cachelines: 2, members: 5 */ > > /* sum members: 85, holes: 1, sum holes: 3 */ > > /* forced alignments: 1, forced holes: 1, sum forced holes: 3 */ > > /* last cacheline: 24 bytes */ > > } __attribute__((__aligned__(8))); > > $ > > $ # after: > > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o > > struct ptrace_syscall_info { > > __u8 op; /* 0 1 */ > > __u8 pad[3]; /* 1 3 */ > > __u32 arch; /* 4 4 */ > > __u64 instruction_pointer; /* 8 8 */ > > __u64 stack_pointer; /* 16 8 */ > > union { > > struct { > > __u64 nr; /* 24 8 */ > > __u64 args[6]; /* 32 48 */ > > } entry; /* 24 56 */ > > struct { > > __s64 rval; /* 24 8 */ > > __u8 is_error; /* 32 1 */ > > } exit; /* 24 16 */ > > struct { > > __u64 nr; /* 24 8 */ > > __u64 args[6]; /* 32 48 */ > > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ > > __u32 ret_data; /* 80 4 */ > > } seccomp; /* 24 64 */ > > }; /* 24 64 */ > > > > /* size: 88, cachelines: 2, members: 6 */ > > /* last cacheline: 24 bytes */ > > }; > > $ _ > > > > include/uapi/linux/ptrace.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h > > index a71b6e3b03eb..83ee45fa634b 100644 > > --- a/include/uapi/linux/ptrace.h > > +++ b/include/uapi/linux/ptrace.h > > @@ -81,7 +81,8 @@ struct seccomp_metadata { > > > > struct ptrace_syscall_info { > > __u8 op; /* PTRACE_SYSCALL_INFO_* */ > > - __u32 arch __attribute__((__aligned__(sizeof(__u32)))); > > + __u8 pad[3]; > > + __u32 arch; > > __u64 instruction_pointer; > > __u64 stack_pointer; > > union { > > Reviewed-by: Dmitry V. Levin <ldv@altlinux.org> Acked-by: Christian Brauner <christian.brauner@ubuntu.com> Oh fun. I'd pick this up and run the ptrace tests that we have for this. If they pass I'd apply to my fixes branch and send after the merge window unless I hear objections. Fwiw, what was the original reason for using __attribute__((__aligned__(sizeof(__u32))))? b4 mbox is failing to download the relevant thread(s) for me. Thanks! Christian ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-01 20:10 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-07-27 21:36 [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() Peilin Ye 2020-08-01 0:21 ` Dmitry V. Levin 2020-08-01 1:28 ` Peilin Ye 2020-08-01 2:08 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye 2020-08-01 11:06 ` Dmitry V. Levin 2020-08-01 15:09 ` Peilin Ye 2020-08-01 15:20 ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye 2020-08-01 16:08 ` Dmitry V. Levin 2020-08-01 20:10 ` Christian Brauner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox