* [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: Arnd Bergmann, linux-kernel, linux-kernel-mentees, Peilin Ye,
Dan Carpenter
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ 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: Arnd Bergmann, Oleg Nesterov, linux-kernel, Elvira Khabirova,
linux-kernel-mentees, Dan Carpenter
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ 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: Arnd Bergmann, Oleg Nesterov, linux-kernel, Elvira Khabirova,
linux-kernel-mentees, Dan Carpenter
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ 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: Arnd Bergmann, linux-kernel, Dmitry V. Levin, Elvira Khabirova,
linux-kernel-mentees, Peilin Ye, Dan Carpenter
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ 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: Arnd Bergmann, Oleg Nesterov, linux-kernel, Elvira Khabirova,
linux-kernel-mentees, Eugene Syromyatnikov, Dan Carpenter
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ 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: Arnd Bergmann, Oleg Nesterov, linux-kernel, Elvira Khabirova,
linux-kernel-mentees, Eugene Syromyatnikov, Dan Carpenter
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ 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: Arnd Bergmann, linux-kernel, Dmitry V. Levin, Elvira Khabirova,
linux-kernel-mentees, Peilin Ye, Dan Carpenter
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ 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: Arnd Bergmann, Oleg Nesterov, linux-kernel, Elvira Khabirova,
linux-api, linux-kernel-mentees, Eugene Syromyatnikov,
Dan Carpenter
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ 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: linux-kernel, Arnd Bergmann, Oleg Nesterov, Peilin Ye,
Elvira Khabirova, linux-api, linux-kernel-mentees,
Eugene Syromyatnikov, Dan Carpenter
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ 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;
as well as URLs for NNTP newsgroup(s).