* [Linux-kernel-mentees] [PATCH v2] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
@ 2020-08-01 2:08 ` Peilin Ye
0 siblings, 0 replies; 18+ 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] 18+ messages in thread* Re: [Linux-kernel-mentees] [PATCH v2] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
2020-08-01 2:08 ` Peilin Ye
@ 2020-08-01 11:06 ` Dmitry V. Levin
-1 siblings, 0 replies; 18+ 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] 18+ 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
0 siblings, 0 replies; 18+ 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] 18+ 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
-1 siblings, 0 replies; 18+ 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] 18+ messages in thread* Re: [Linux-kernel-mentees] [PATCH v2] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
@ 2020-08-01 15:09 ` Peilin Ye
0 siblings, 0 replies; 18+ 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] 18+ messages in thread
* [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
2020-08-01 2:08 ` Peilin Ye
@ 2020-08-01 15:20 ` Peilin Ye
-1 siblings, 0 replies; 18+ 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] 18+ messages in thread* [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
@ 2020-08-01 15:20 ` Peilin Ye
0 siblings, 0 replies; 18+ 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] 18+ messages in thread* Re: [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
2020-08-01 15:20 ` Peilin Ye
@ 2020-08-01 16:08 ` Dmitry V. Levin
-1 siblings, 0 replies; 18+ 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] 18+ 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
0 siblings, 0 replies; 18+ 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] 18+ 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
-1 siblings, 0 replies; 18+ 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] 18+ messages in thread* Re: [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
@ 2020-08-01 20:10 ` Christian Brauner
0 siblings, 0 replies; 18+ 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] 18+ messages in thread