LKML Archive on lore.kernel.org
 help / color / Atom feed
* [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
@ 2020-07-27 21:36 Peilin Ye
  2020-08-01  0:21 ` Dmitry V. Levin
  2020-08-01  2:08 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
  0 siblings, 2 replies; 9+ messages in thread
From: Peilin Ye @ 2020-07-27 21:36 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Peilin Ye, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, linux-kernel

ptrace_get_syscall_info() is copying uninitialized stack memory to
userspace due to the compiler not initializing holes in statically
allocated structures. Fix it by initializing `info` with memset().

Cc: stable@vger.kernel.org
Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
 kernel/ptrace.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 43d6179508d6..e48d05b765b5 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -960,15 +960,17 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
 			void __user *datavp)
 {
 	struct pt_regs *regs = task_pt_regs(child);
-	struct ptrace_syscall_info info = {
-		.op = PTRACE_SYSCALL_INFO_NONE,
-		.arch = syscall_get_arch(child),
-		.instruction_pointer = instruction_pointer(regs),
-		.stack_pointer = user_stack_pointer(regs),
-	};
+	struct ptrace_syscall_info info;
 	unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
 	unsigned long write_size;
 
+	memset(&info, 0, sizeof(info));
+
+	info.op	= PTRACE_SYSCALL_INFO_NONE;
+	info.arch = syscall_get_arch(child);
+	info.instruction_pointer = instruction_pointer(regs);
+	info.stack_pointer = user_stack_pointer(regs);
+
 	/*
 	 * This does not need lock_task_sighand() to access
 	 * child->last_siginfo because ptrace_freeze_traced()
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
  2020-07-27 21:36 [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() Peilin Ye
@ 2020-08-01  0:21 ` Dmitry V. Levin
  2020-08-01  1:28   ` Peilin Ye
  2020-08-01  2:08 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
  1 sibling, 1 reply; 9+ messages in thread
From: Dmitry V. Levin @ 2020-08-01  0:21 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Elvira Khabirova, Oleg Nesterov, Dan Carpenter, Arnd Bergmann,
	Greg Kroah-Hartman, linux-kernel-mentees, linux-kernel

On Mon, Jul 27, 2020 at 05:36:44PM -0400, Peilin Ye wrote:
> ptrace_get_syscall_info() is copying uninitialized stack memory to
> userspace due to the compiler not initializing holes in statically
> allocated structures. Fix it by initializing `info` with memset().
> 
> Cc: stable@vger.kernel.org
> Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
>  kernel/ptrace.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 43d6179508d6..e48d05b765b5 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -960,15 +960,17 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
>  			void __user *datavp)
>  {
>  	struct pt_regs *regs = task_pt_regs(child);
> -	struct ptrace_syscall_info info = {
> -		.op = PTRACE_SYSCALL_INFO_NONE,
> -		.arch = syscall_get_arch(child),
> -		.instruction_pointer = instruction_pointer(regs),
> -		.stack_pointer = user_stack_pointer(regs),
> -	};
> +	struct ptrace_syscall_info info;
>  	unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
>  	unsigned long write_size;
>  
> +	memset(&info, 0, sizeof(info));
> +
> +	info.op	= PTRACE_SYSCALL_INFO_NONE;
> +	info.arch = syscall_get_arch(child);
> +	info.instruction_pointer = instruction_pointer(regs);
> +	info.stack_pointer = user_stack_pointer(regs);
> +

No, please don't do it this way.  If there is a hole in the structure that
the compiler is unable to initialize properly (and there is a 3-byte hole
in the beginning indeed), please plug the hole by turning it into
something that the compiler is capable of initializing.

Also, please do not forget to Cc authors of the commit you are fixing.


-- 
ldv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
  2020-08-01  0:21 ` Dmitry V. Levin
@ 2020-08-01  1:28   ` Peilin Ye
  0 siblings, 0 replies; 9+ messages in thread
From: Peilin Ye @ 2020-08-01  1:28 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Elvira Khabirova, Oleg Nesterov, Dan Carpenter, Arnd Bergmann,
	Greg Kroah-Hartman, linux-kernel-mentees, linux-kernel

On Sat, Aug 01, 2020 at 03:21:42AM +0300, Dmitry V. Levin wrote:
> On Mon, Jul 27, 2020 at 05:36:44PM -0400, Peilin Ye wrote:
> > ptrace_get_syscall_info() is copying uninitialized stack memory to
> > userspace due to the compiler not initializing holes in statically
> > allocated structures. Fix it by initializing `info` with memset().
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > ---
> >  kernel/ptrace.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > index 43d6179508d6..e48d05b765b5 100644
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -960,15 +960,17 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> >  			void __user *datavp)
> >  {
> >  	struct pt_regs *regs = task_pt_regs(child);
> > -	struct ptrace_syscall_info info = {
> > -		.op = PTRACE_SYSCALL_INFO_NONE,
> > -		.arch = syscall_get_arch(child),
> > -		.instruction_pointer = instruction_pointer(regs),
> > -		.stack_pointer = user_stack_pointer(regs),
> > -	};
> > +	struct ptrace_syscall_info info;
> >  	unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
> >  	unsigned long write_size;
> >  
> > +	memset(&info, 0, sizeof(info));
> > +
> > +	info.op	= PTRACE_SYSCALL_INFO_NONE;
> > +	info.arch = syscall_get_arch(child);
> > +	info.instruction_pointer = instruction_pointer(regs);
> > +	info.stack_pointer = user_stack_pointer(regs);
> > +
> 
> No, please don't do it this way.  If there is a hole in the structure that
> the compiler is unable to initialize properly (and there is a 3-byte hole
> in the beginning indeed), please plug the hole by turning it into
> something that the compiler is capable of initializing.

I see. I will do that and send a v2.

> Also, please do not forget to Cc authors of the commit you are fixing.

Sorry, I forgot about that. Thank you for pointing it out!

Peilin Ye

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Linux-kernel-mentees] [PATCH v2] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
  2020-07-27 21:36 [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info() Peilin Ye
  2020-08-01  0:21 ` Dmitry V. Levin
@ 2020-08-01  2:08 ` Peilin Ye
  2020-08-01 11:06   ` Dmitry V. Levin
  2020-08-01 15:20   ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye
  1 sibling, 2 replies; 9+ messages in thread
From: Peilin Ye @ 2020-08-01  2:08 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Peilin Ye, Dmitry V. Levin, Elvira Khabirova, Dan Carpenter,
	Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees,
	linux-kernel

ptrace_get_syscall_info() is potentially copying uninitialized stack
memory to userspace, since the compiler may leave a 3-byte hole near the
beginning of `info`. Fix it by adding a padding field to `struct
ptrace_syscall_info`.

Cc: stable@vger.kernel.org
Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
Change in v2:
    - Add a padding field to `struct ptrace_syscall_info`, instead of
      doing memset() on `info`. (Suggested by Dmitry V. Levin
      <ldv@altlinux.org>)

Reference: https://lwn.net/Articles/417989/

$ # before:
$ pahole -C "ptrace_syscall_info" kernel/ptrace.o
struct ptrace_syscall_info {
	__u8                       op;                   /*     0     1 */

	/* XXX 3 bytes hole, try to pack */

	__u32                      arch __attribute__((__aligned__(4))); /*     4     4 */
	__u64                      instruction_pointer;  /*     8     8 */
	__u64                      stack_pointer;        /*    16     8 */
	union {
		struct {
			__u64      nr;                   /*    24     8 */
			__u64      args[6];              /*    32    48 */
		} entry;                                 /*    24    56 */
		struct {
			__s64      rval;                 /*    24     8 */
			__u8       is_error;             /*    32     1 */
		} exit;                                  /*    24    16 */
		struct {
			__u64      nr;                   /*    24     8 */
			__u64      args[6];              /*    32    48 */
			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
			__u32      ret_data;             /*    80     4 */
		} seccomp;                               /*    24    64 */
	};                                               /*    24    64 */

	/* size: 88, cachelines: 2, members: 5 */
	/* sum members: 85, holes: 1, sum holes: 3 */
	/* forced alignments: 1, forced holes: 1, sum forced holes: 3 */
	/* last cacheline: 24 bytes */
} __attribute__((__aligned__(8)));
$
$ # after:
$ pahole -C "ptrace_syscall_info" kernel/ptrace.o
struct ptrace_syscall_info {
	__u8                       op;                   /*     0     1 */
	__u8                       pad[3];               /*     1     3 */
	__u32                      arch __attribute__((__aligned__(4))); /*     4     4 */
	__u64                      instruction_pointer;  /*     8     8 */
	__u64                      stack_pointer;        /*    16     8 */
	union {
		struct {
			__u64      nr;                   /*    24     8 */
			__u64      args[6];              /*    32    48 */
		} entry;                                 /*    24    56 */
		struct {
			__s64      rval;                 /*    24     8 */
			__u8       is_error;             /*    32     1 */
		} exit;                                  /*    24    16 */
		struct {
			__u64      nr;                   /*    24     8 */
			__u64      args[6];              /*    32    48 */
			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
			__u32      ret_data;             /*    80     4 */
		} seccomp;                               /*    24    64 */
	};                                               /*    24    64 */

	/* size: 88, cachelines: 2, members: 6 */
	/* forced alignments: 1 */
	/* last cacheline: 24 bytes */
} __attribute__((__aligned__(8)));
$ _

 include/uapi/linux/ptrace.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index a71b6e3b03eb..a518ba514bac 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -81,6 +81,7 @@ struct seccomp_metadata {
 
 struct ptrace_syscall_info {
 	__u8 op;	/* PTRACE_SYSCALL_INFO_* */
+	__u8 pad[3];
 	__u32 arch __attribute__((__aligned__(sizeof(__u32))));
 	__u64 instruction_pointer;
 	__u64 stack_pointer;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
  2020-08-01  2:08 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
@ 2020-08-01 11:06   ` Dmitry V. Levin
  2020-08-01 15:09     ` Peilin Ye
  2020-08-01 15:20   ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye
  1 sibling, 1 reply; 9+ messages in thread
From: Dmitry V. Levin @ 2020-08-01 11:06 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Oleg Nesterov, Elvira Khabirova, Eugene Syromyatnikov,
	Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, linux-kernel

On Fri, Jul 31, 2020 at 10:08:41PM -0400, Peilin Ye wrote:
> ptrace_get_syscall_info() is potentially copying uninitialized stack
> memory to userspace, since the compiler may leave a 3-byte hole near the
> beginning of `info`. Fix it by adding a padding field to `struct
> ptrace_syscall_info`.
> 
> Cc: stable@vger.kernel.org
> Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> Change in v2:
>     - Add a padding field to `struct ptrace_syscall_info`, instead of
>       doing memset() on `info`. (Suggested by Dmitry V. Levin
>       <ldv@altlinux.org>)
> 
> Reference: https://lwn.net/Articles/417989/
> 
> $ # before:
> $ pahole -C "ptrace_syscall_info" kernel/ptrace.o
> struct ptrace_syscall_info {
> 	__u8                       op;                   /*     0     1 */
> 
> 	/* XXX 3 bytes hole, try to pack */
> 
> 	__u32                      arch __attribute__((__aligned__(4))); /*     4     4 */
> 	__u64                      instruction_pointer;  /*     8     8 */
> 	__u64                      stack_pointer;        /*    16     8 */
> 	union {
> 		struct {
> 			__u64      nr;                   /*    24     8 */
> 			__u64      args[6];              /*    32    48 */
> 		} entry;                                 /*    24    56 */
> 		struct {
> 			__s64      rval;                 /*    24     8 */
> 			__u8       is_error;             /*    32     1 */
> 		} exit;                                  /*    24    16 */
> 		struct {
> 			__u64      nr;                   /*    24     8 */
> 			__u64      args[6];              /*    32    48 */
> 			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> 			__u32      ret_data;             /*    80     4 */
> 		} seccomp;                               /*    24    64 */
> 	};                                               /*    24    64 */
> 
> 	/* size: 88, cachelines: 2, members: 5 */
> 	/* sum members: 85, holes: 1, sum holes: 3 */
> 	/* forced alignments: 1, forced holes: 1, sum forced holes: 3 */
> 	/* last cacheline: 24 bytes */
> } __attribute__((__aligned__(8)));
> $
> $ # after:
> $ pahole -C "ptrace_syscall_info" kernel/ptrace.o
> struct ptrace_syscall_info {
> 	__u8                       op;                   /*     0     1 */
> 	__u8                       pad[3];               /*     1     3 */
> 	__u32                      arch __attribute__((__aligned__(4))); /*     4     4 */
> 	__u64                      instruction_pointer;  /*     8     8 */
> 	__u64                      stack_pointer;        /*    16     8 */
> 	union {
> 		struct {
> 			__u64      nr;                   /*    24     8 */
> 			__u64      args[6];              /*    32    48 */
> 		} entry;                                 /*    24    56 */
> 		struct {
> 			__s64      rval;                 /*    24     8 */
> 			__u8       is_error;             /*    32     1 */
> 		} exit;                                  /*    24    16 */
> 		struct {
> 			__u64      nr;                   /*    24     8 */
> 			__u64      args[6];              /*    32    48 */
> 			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> 			__u32      ret_data;             /*    80     4 */
> 		} seccomp;                               /*    24    64 */
> 	};                                               /*    24    64 */
> 
> 	/* size: 88, cachelines: 2, members: 6 */
> 	/* forced alignments: 1 */
> 	/* last cacheline: 24 bytes */
> } __attribute__((__aligned__(8)));
> $ _
> 
>  include/uapi/linux/ptrace.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index a71b6e3b03eb..a518ba514bac 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -81,6 +81,7 @@ struct seccomp_metadata {
>  
>  struct ptrace_syscall_info {
>  	__u8 op;	/* PTRACE_SYSCALL_INFO_* */
> +	__u8 pad[3];
>  	__u32 arch __attribute__((__aligned__(sizeof(__u32))));
>  	__u64 instruction_pointer;
>  	__u64 stack_pointer;

Funnily enough, but in first editions of PTRACE_GET_SYSCALL_INFO
patchset [1] this was looking very similar:

+struct ptrace_syscall_info {
+	__u8 op;        /* PTRACE_SYSCALL_INFO_* */
+	__u8 __pad0[3];
+	__u32 arch;

But later we decided [2][3] to replace the pad with a hole.

Note that the sole purpose of the __aligned__ attribute on the field that
follows the hole is to guarantee that the hole has the same size across
architectures.  As this hole is being replaced back with a pad, that
__aligned__ attribute is no longer needed and can be omitted along with
adding the pad.

[1] https://lore.kernel.org/linux-api/20181125022150.46258a20@akathisia/
[2] https://lore.kernel.org/linux-api/20181211162305.GA480@altlinux.org/
[3] https://lore.kernel.org/linux-api/20181213171833.GA5240@altlinux.org/


-- 
ldv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
  2020-08-01 11:06   ` Dmitry V. Levin
@ 2020-08-01 15:09     ` Peilin Ye
  0 siblings, 0 replies; 9+ messages in thread
From: Peilin Ye @ 2020-08-01 15:09 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Oleg Nesterov, Elvira Khabirova, Eugene Syromyatnikov,
	Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, linux-kernel

On Sat, Aug 01, 2020 at 02:06:46PM +0300, Dmitry V. Levin wrote:
> On Fri, Jul 31, 2020 at 10:08:41PM -0400, Peilin Ye wrote:
> > ptrace_get_syscall_info() is potentially copying uninitialized stack
> > memory to userspace, since the compiler may leave a 3-byte hole near the
> > beginning of `info`. Fix it by adding a padding field to `struct
> > ptrace_syscall_info`.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > ---
> > Change in v2:
> >     - Add a padding field to `struct ptrace_syscall_info`, instead of
> >       doing memset() on `info`. (Suggested by Dmitry V. Levin
> >       <ldv@altlinux.org>)
> > 
> > Reference: https://lwn.net/Articles/417989/
> > 
> > $ # before:
> > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o
> > struct ptrace_syscall_info {
> > 	__u8                       op;                   /*     0     1 */
> > 
> > 	/* XXX 3 bytes hole, try to pack */
> > 
> > 	__u32                      arch __attribute__((__aligned__(4))); /*     4     4 */
> > 	__u64                      instruction_pointer;  /*     8     8 */
> > 	__u64                      stack_pointer;        /*    16     8 */
> > 	union {
> > 		struct {
> > 			__u64      nr;                   /*    24     8 */
> > 			__u64      args[6];              /*    32    48 */
> > 		} entry;                                 /*    24    56 */
> > 		struct {
> > 			__s64      rval;                 /*    24     8 */
> > 			__u8       is_error;             /*    32     1 */
> > 		} exit;                                  /*    24    16 */
> > 		struct {
> > 			__u64      nr;                   /*    24     8 */
> > 			__u64      args[6];              /*    32    48 */
> > 			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> > 			__u32      ret_data;             /*    80     4 */
> > 		} seccomp;                               /*    24    64 */
> > 	};                                               /*    24    64 */
> > 
> > 	/* size: 88, cachelines: 2, members: 5 */
> > 	/* sum members: 85, holes: 1, sum holes: 3 */
> > 	/* forced alignments: 1, forced holes: 1, sum forced holes: 3 */
> > 	/* last cacheline: 24 bytes */
> > } __attribute__((__aligned__(8)));
> > $
> > $ # after:
> > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o
> > struct ptrace_syscall_info {
> > 	__u8                       op;                   /*     0     1 */
> > 	__u8                       pad[3];               /*     1     3 */
> > 	__u32                      arch __attribute__((__aligned__(4))); /*     4     4 */
> > 	__u64                      instruction_pointer;  /*     8     8 */
> > 	__u64                      stack_pointer;        /*    16     8 */
> > 	union {
> > 		struct {
> > 			__u64      nr;                   /*    24     8 */
> > 			__u64      args[6];              /*    32    48 */
> > 		} entry;                                 /*    24    56 */
> > 		struct {
> > 			__s64      rval;                 /*    24     8 */
> > 			__u8       is_error;             /*    32     1 */
> > 		} exit;                                  /*    24    16 */
> > 		struct {
> > 			__u64      nr;                   /*    24     8 */
> > 			__u64      args[6];              /*    32    48 */
> > 			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> > 			__u32      ret_data;             /*    80     4 */
> > 		} seccomp;                               /*    24    64 */
> > 	};                                               /*    24    64 */
> > 
> > 	/* size: 88, cachelines: 2, members: 6 */
> > 	/* forced alignments: 1 */
> > 	/* last cacheline: 24 bytes */
> > } __attribute__((__aligned__(8)));
> > $ _
> > 
> >  include/uapi/linux/ptrace.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> > index a71b6e3b03eb..a518ba514bac 100644
> > --- a/include/uapi/linux/ptrace.h
> > +++ b/include/uapi/linux/ptrace.h
> > @@ -81,6 +81,7 @@ struct seccomp_metadata {
> >  
> >  struct ptrace_syscall_info {
> >  	__u8 op;	/* PTRACE_SYSCALL_INFO_* */
> > +	__u8 pad[3];
> >  	__u32 arch __attribute__((__aligned__(sizeof(__u32))));
> >  	__u64 instruction_pointer;
> >  	__u64 stack_pointer;
> 
> Funnily enough, but in first editions of PTRACE_GET_SYSCALL_INFO
> patchset [1] this was looking very similar:
> 
> +struct ptrace_syscall_info {
> +	__u8 op;        /* PTRACE_SYSCALL_INFO_* */
> +	__u8 __pad0[3];
> +	__u32 arch;
> 
> But later we decided [2][3] to replace the pad with a hole.
> 
> Note that the sole purpose of the __aligned__ attribute on the field that
> follows the hole is to guarantee that the hole has the same size across
> architectures.  As this hole is being replaced back with a pad, that
> __aligned__ attribute is no longer needed and can be omitted along with
> adding the pad.

Ah, I see. I will remove that in v3.

Thank you,
Peilin Ye

> [1] https://lore.kernel.org/linux-api/20181125022150.46258a20@akathisia/
> [2] https://lore.kernel.org/linux-api/20181211162305.GA480@altlinux.org/
> [3] https://lore.kernel.org/linux-api/20181213171833.GA5240@altlinux.org/
> 
> 
> -- 
> ldv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
  2020-08-01  2:08 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
  2020-08-01 11:06   ` Dmitry V. Levin
@ 2020-08-01 15:20   ` Peilin Ye
  2020-08-01 16:08     ` Dmitry V. Levin
  1 sibling, 1 reply; 9+ messages in thread
From: Peilin Ye @ 2020-08-01 15:20 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Peilin Ye, Dmitry V. Levin, Elvira Khabirova, Dan Carpenter,
	Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees,
	linux-kernel

ptrace_get_syscall_info() is potentially copying uninitialized stack
memory to userspace, since the compiler may leave a 3-byte hole near the
beginning of `info`. Fix it by adding a padding field to `struct
ptrace_syscall_info`.

Cc: stable@vger.kernel.org
Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
Change in v3:
    - Remove unnecessary `__aligned__` attribute. (Suggested by
      Dmitry V. Levin <ldv@altlinux.org>)

Change in v2:
    - Add a padding field to `struct ptrace_syscall_info`, instead of
      doing memset() on `info`. (Suggested by Dmitry V. Levin
      <ldv@altlinux.org>)

Reference: https://lwn.net/Articles/417989/

$ # before:
$ pahole -C "ptrace_syscall_info" kernel/ptrace.o
struct ptrace_syscall_info {
	__u8                       op;                   /*     0     1 */

	/* XXX 3 bytes hole, try to pack */

	__u32                      arch __attribute__((__aligned__(4))); /*     4     4 */
	__u64                      instruction_pointer;  /*     8     8 */
	__u64                      stack_pointer;        /*    16     8 */
	union {
		struct {
			__u64      nr;                   /*    24     8 */
			__u64      args[6];              /*    32    48 */
		} entry;                                 /*    24    56 */
		struct {
			__s64      rval;                 /*    24     8 */
			__u8       is_error;             /*    32     1 */
		} exit;                                  /*    24    16 */
		struct {
			__u64      nr;                   /*    24     8 */
			__u64      args[6];              /*    32    48 */
			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
			__u32      ret_data;             /*    80     4 */
		} seccomp;                               /*    24    64 */
	};                                               /*    24    64 */

	/* size: 88, cachelines: 2, members: 5 */
	/* sum members: 85, holes: 1, sum holes: 3 */
	/* forced alignments: 1, forced holes: 1, sum forced holes: 3 */
	/* last cacheline: 24 bytes */
} __attribute__((__aligned__(8)));
$
$ # after:
$ pahole -C "ptrace_syscall_info" kernel/ptrace.o
struct ptrace_syscall_info {
	__u8                       op;                   /*     0     1 */
	__u8                       pad[3];               /*     1     3 */
	__u32                      arch;                 /*     4     4 */
	__u64                      instruction_pointer;  /*     8     8 */
	__u64                      stack_pointer;        /*    16     8 */
	union {
		struct {
			__u64      nr;                   /*    24     8 */
			__u64      args[6];              /*    32    48 */
		} entry;                                 /*    24    56 */
		struct {
			__s64      rval;                 /*    24     8 */
			__u8       is_error;             /*    32     1 */
		} exit;                                  /*    24    16 */
		struct {
			__u64      nr;                   /*    24     8 */
			__u64      args[6];              /*    32    48 */
			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
			__u32      ret_data;             /*    80     4 */
		} seccomp;                               /*    24    64 */
	};                                               /*    24    64 */

	/* size: 88, cachelines: 2, members: 6 */
	/* last cacheline: 24 bytes */
};
$ _

 include/uapi/linux/ptrace.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index a71b6e3b03eb..83ee45fa634b 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -81,7 +81,8 @@ struct seccomp_metadata {
 
 struct ptrace_syscall_info {
 	__u8 op;	/* PTRACE_SYSCALL_INFO_* */
-	__u32 arch __attribute__((__aligned__(sizeof(__u32))));
+	__u8 pad[3];
+	__u32 arch;
 	__u64 instruction_pointer;
 	__u64 stack_pointer;
 	union {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
  2020-08-01 15:20   ` [Linux-kernel-mentees] [PATCH v3] " Peilin Ye
@ 2020-08-01 16:08     ` Dmitry V. Levin
  2020-08-01 20:10       ` Christian Brauner
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry V. Levin @ 2020-08-01 16:08 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Oleg Nesterov, Elvira Khabirova, Eugene Syromyatnikov,
	Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, linux-api, linux-kernel

On Sat, Aug 01, 2020 at 11:20:44AM -0400, Peilin Ye wrote:
> ptrace_get_syscall_info() is potentially copying uninitialized stack
> memory to userspace, since the compiler may leave a 3-byte hole near the
> beginning of `info`. Fix it by adding a padding field to `struct
> ptrace_syscall_info`.
> 
> Cc: stable@vger.kernel.org
> Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> Change in v3:
>     - Remove unnecessary `__aligned__` attribute. (Suggested by
>       Dmitry V. Levin <ldv@altlinux.org>)
> 
> Change in v2:
>     - Add a padding field to `struct ptrace_syscall_info`, instead of
>       doing memset() on `info`. (Suggested by Dmitry V. Levin
>       <ldv@altlinux.org>)
> 
> Reference: https://lwn.net/Articles/417989/
> 
> $ # before:
> $ pahole -C "ptrace_syscall_info" kernel/ptrace.o
> struct ptrace_syscall_info {
> 	__u8                       op;                   /*     0     1 */
> 
> 	/* XXX 3 bytes hole, try to pack */
> 
> 	__u32                      arch __attribute__((__aligned__(4))); /*     4     4 */
> 	__u64                      instruction_pointer;  /*     8     8 */
> 	__u64                      stack_pointer;        /*    16     8 */
> 	union {
> 		struct {
> 			__u64      nr;                   /*    24     8 */
> 			__u64      args[6];              /*    32    48 */
> 		} entry;                                 /*    24    56 */
> 		struct {
> 			__s64      rval;                 /*    24     8 */
> 			__u8       is_error;             /*    32     1 */
> 		} exit;                                  /*    24    16 */
> 		struct {
> 			__u64      nr;                   /*    24     8 */
> 			__u64      args[6];              /*    32    48 */
> 			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> 			__u32      ret_data;             /*    80     4 */
> 		} seccomp;                               /*    24    64 */
> 	};                                               /*    24    64 */
> 
> 	/* size: 88, cachelines: 2, members: 5 */
> 	/* sum members: 85, holes: 1, sum holes: 3 */
> 	/* forced alignments: 1, forced holes: 1, sum forced holes: 3 */
> 	/* last cacheline: 24 bytes */
> } __attribute__((__aligned__(8)));
> $
> $ # after:
> $ pahole -C "ptrace_syscall_info" kernel/ptrace.o
> struct ptrace_syscall_info {
> 	__u8                       op;                   /*     0     1 */
> 	__u8                       pad[3];               /*     1     3 */
> 	__u32                      arch;                 /*     4     4 */
> 	__u64                      instruction_pointer;  /*     8     8 */
> 	__u64                      stack_pointer;        /*    16     8 */
> 	union {
> 		struct {
> 			__u64      nr;                   /*    24     8 */
> 			__u64      args[6];              /*    32    48 */
> 		} entry;                                 /*    24    56 */
> 		struct {
> 			__s64      rval;                 /*    24     8 */
> 			__u8       is_error;             /*    32     1 */
> 		} exit;                                  /*    24    16 */
> 		struct {
> 			__u64      nr;                   /*    24     8 */
> 			__u64      args[6];              /*    32    48 */
> 			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> 			__u32      ret_data;             /*    80     4 */
> 		} seccomp;                               /*    24    64 */
> 	};                                               /*    24    64 */
> 
> 	/* size: 88, cachelines: 2, members: 6 */
> 	/* last cacheline: 24 bytes */
> };
> $ _
> 
>  include/uapi/linux/ptrace.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index a71b6e3b03eb..83ee45fa634b 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -81,7 +81,8 @@ struct seccomp_metadata {
>  
>  struct ptrace_syscall_info {
>  	__u8 op;	/* PTRACE_SYSCALL_INFO_* */
> -	__u32 arch __attribute__((__aligned__(sizeof(__u32))));
> +	__u8 pad[3];
> +	__u32 arch;
>  	__u64 instruction_pointer;
>  	__u64 stack_pointer;
>  	union {

Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>

Thanks,


-- 
ldv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3] ptrace: Prevent kernel-infoleak in ptrace_get_syscall_info()
  2020-08-01 16:08     ` Dmitry V. Levin
@ 2020-08-01 20:10       ` Christian Brauner
  0 siblings, 0 replies; 9+ messages in thread
From: Christian Brauner @ 2020-08-01 20:10 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Peilin Ye, Oleg Nesterov, Elvira Khabirova, Eugene Syromyatnikov,
	Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, linux-api, linux-kernel

On Sat, Aug 01, 2020 at 07:08:19PM +0300, Dmitry V. Levin wrote:
> On Sat, Aug 01, 2020 at 11:20:44AM -0400, Peilin Ye wrote:
> > ptrace_get_syscall_info() is potentially copying uninitialized stack
> > memory to userspace, since the compiler may leave a 3-byte hole near the
> > beginning of `info`. Fix it by adding a padding field to `struct
> > ptrace_syscall_info`.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > ---
> > Change in v3:
> >     - Remove unnecessary `__aligned__` attribute. (Suggested by
> >       Dmitry V. Levin <ldv@altlinux.org>)
> > 
> > Change in v2:
> >     - Add a padding field to `struct ptrace_syscall_info`, instead of
> >       doing memset() on `info`. (Suggested by Dmitry V. Levin
> >       <ldv@altlinux.org>)
> > 
> > Reference: https://lwn.net/Articles/417989/
> > 
> > $ # before:
> > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o
> > struct ptrace_syscall_info {
> > 	__u8                       op;                   /*     0     1 */
> > 
> > 	/* XXX 3 bytes hole, try to pack */
> > 
> > 	__u32                      arch __attribute__((__aligned__(4))); /*     4     4 */
> > 	__u64                      instruction_pointer;  /*     8     8 */
> > 	__u64                      stack_pointer;        /*    16     8 */
> > 	union {
> > 		struct {
> > 			__u64      nr;                   /*    24     8 */
> > 			__u64      args[6];              /*    32    48 */
> > 		} entry;                                 /*    24    56 */
> > 		struct {
> > 			__s64      rval;                 /*    24     8 */
> > 			__u8       is_error;             /*    32     1 */
> > 		} exit;                                  /*    24    16 */
> > 		struct {
> > 			__u64      nr;                   /*    24     8 */
> > 			__u64      args[6];              /*    32    48 */
> > 			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> > 			__u32      ret_data;             /*    80     4 */
> > 		} seccomp;                               /*    24    64 */
> > 	};                                               /*    24    64 */
> > 
> > 	/* size: 88, cachelines: 2, members: 5 */
> > 	/* sum members: 85, holes: 1, sum holes: 3 */
> > 	/* forced alignments: 1, forced holes: 1, sum forced holes: 3 */
> > 	/* last cacheline: 24 bytes */
> > } __attribute__((__aligned__(8)));
> > $
> > $ # after:
> > $ pahole -C "ptrace_syscall_info" kernel/ptrace.o
> > struct ptrace_syscall_info {
> > 	__u8                       op;                   /*     0     1 */
> > 	__u8                       pad[3];               /*     1     3 */
> > 	__u32                      arch;                 /*     4     4 */
> > 	__u64                      instruction_pointer;  /*     8     8 */
> > 	__u64                      stack_pointer;        /*    16     8 */
> > 	union {
> > 		struct {
> > 			__u64      nr;                   /*    24     8 */
> > 			__u64      args[6];              /*    32    48 */
> > 		} entry;                                 /*    24    56 */
> > 		struct {
> > 			__s64      rval;                 /*    24     8 */
> > 			__u8       is_error;             /*    32     1 */
> > 		} exit;                                  /*    24    16 */
> > 		struct {
> > 			__u64      nr;                   /*    24     8 */
> > 			__u64      args[6];              /*    32    48 */
> > 			/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> > 			__u32      ret_data;             /*    80     4 */
> > 		} seccomp;                               /*    24    64 */
> > 	};                                               /*    24    64 */
> > 
> > 	/* size: 88, cachelines: 2, members: 6 */
> > 	/* last cacheline: 24 bytes */
> > };
> > $ _
> > 
> >  include/uapi/linux/ptrace.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> > index a71b6e3b03eb..83ee45fa634b 100644
> > --- a/include/uapi/linux/ptrace.h
> > +++ b/include/uapi/linux/ptrace.h
> > @@ -81,7 +81,8 @@ struct seccomp_metadata {
> >  
> >  struct ptrace_syscall_info {
> >  	__u8 op;	/* PTRACE_SYSCALL_INFO_* */
> > -	__u32 arch __attribute__((__aligned__(sizeof(__u32))));
> > +	__u8 pad[3];
> > +	__u32 arch;
> >  	__u64 instruction_pointer;
> >  	__u64 stack_pointer;
> >  	union {
> 
> Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Oh fun.
I'd pick this up and run the ptrace tests that we have for this. If they
pass I'd apply to my fixes branch and send after the merge window unless
I hear objections.

Fwiw, what was the original reason for using
__attribute__((__aligned__(sizeof(__u32))))?
b4 mbox is failing to download the relevant thread(s) for me.

Thanks!
Christian

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git