linux-kernel - Re: [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak in ptrace_get_syscall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200801002142.GA27762@altlinux.org>
Date:   Sat, 1 Aug 2020 03:21:42 +0300
From:   "Dmitry V. Levin" <ldv@...linux.org>
To:     Peilin Ye <yepeilin.cs@...il.com>
Cc:     Elvira Khabirova <lineprinter@...linux.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Linux-kernel-mentees] [PATCH] ptrace: Prevent kernel-infoleak
 in ptrace_get_syscall_info()

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@...r.kernel.org
> Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> Suggested-by: Dan Carpenter <dan.carpenter@...cle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@...il.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