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: [email protected]
Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
Suggested-by: Dan Carpenter <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
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
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: [email protected]
> Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> Suggested-by: Dan Carpenter <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> 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
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: [email protected]
> > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> > Suggested-by: Dan Carpenter <[email protected]>
> > Signed-off-by: Peilin Ye <[email protected]>
> > ---
> > 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
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: [email protected]
Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
Suggested-by: Dan Carpenter <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
Change in v2:
- Add a padding field to `struct ptrace_syscall_info`, instead of
doing memset() on `info`. (Suggested by Dmitry V. Levin
<[email protected]>)
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
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: [email protected]
> Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> Suggested-by: Dan Carpenter <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> Change in v2:
> - Add a padding field to `struct ptrace_syscall_info`, instead of
> doing memset() on `info`. (Suggested by Dmitry V. Levin
> <[email protected]>)
>
> 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/[email protected]/
[3] https://lore.kernel.org/linux-api/[email protected]/
--
ldv
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: [email protected]
> > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> > Suggested-by: Dan Carpenter <[email protected]>
> > Signed-off-by: Peilin Ye <[email protected]>
> > ---
> > Change in v2:
> > - Add a padding field to `struct ptrace_syscall_info`, instead of
> > doing memset() on `info`. (Suggested by Dmitry V. Levin
> > <[email protected]>)
> >
> > 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/[email protected]/
> [3] https://lore.kernel.org/linux-api/[email protected]/
>
>
> --
> ldv
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: [email protected]
Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
Suggested-by: Dan Carpenter <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
Change in v3:
- Remove unnecessary `__aligned__` attribute. (Suggested by
Dmitry V. Levin <[email protected]>)
Change in v2:
- Add a padding field to `struct ptrace_syscall_info`, instead of
doing memset() on `info`. (Suggested by Dmitry V. Levin
<[email protected]>)
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
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: [email protected]
> Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> Suggested-by: Dan Carpenter <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> Change in v3:
> - Remove unnecessary `__aligned__` attribute. (Suggested by
> Dmitry V. Levin <[email protected]>)
>
> Change in v2:
> - Add a padding field to `struct ptrace_syscall_info`, instead of
> doing memset() on `info`. (Suggested by Dmitry V. Levin
> <[email protected]>)
>
> 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 <[email protected]>
Thanks,
--
ldv
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: [email protected]
> > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> > Suggested-by: Dan Carpenter <[email protected]>
> > Signed-off-by: Peilin Ye <[email protected]>
> > ---
> > Change in v3:
> > - Remove unnecessary `__aligned__` attribute. (Suggested by
> > Dmitry V. Levin <[email protected]>)
> >
> > Change in v2:
> > - Add a padding field to `struct ptrace_syscall_info`, instead of
> > doing memset() on `info`. (Suggested by Dmitry V. Levin
> > <[email protected]>)
> >
> > 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 <[email protected]>
Acked-by: Christian Brauner <[email protected]>
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