lkml.org 
[lkml]   [2019]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO
On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>
> Recently syzbot in conjunction with KMSAN reported that
> ptrace_peek_siginfo can copy an uninitialized siginfo to userspace.
> Inspecting ptrace_peek_siginfo confirms this.
>
> The problem is that off when initialized from args.off can be
> initialized to a negaive value. At which point the "if (off >= 0)"
> test to see if off became negative fails because off started off
> negative.
>
> Prevent the core problem by adding a variable found that is only true
> if a siginfo is found and copied to a temporary in preparation for
> being copied to userspace.
>
> Prevent args.off from being truncated when being assigned to off by
> testing that off is <= the maximum possible value of off. Convert off
> to an unsigned long so that we should not have to truncate args.off,
> we have well defined overflow behavior so if we add another check we
> won't risk fighting undefined compiler behavior, and so that we have a
> type whose maximum value is easy to test for.
>

Hello Eric,

Thank you for fixing this issue. Sorry for the late response.
I thought it was fixed a few month ago, I remembered that we discussed it:
https://lkml.org/lkml/2018/10/10/251

Here are two inline comments.


> Cc: Andrei Vagin <avagin@gmail.com>
> Cc: stable@vger.kernel.org
> Reported-by: syzbot+0d602a1b0d8c95bdf299@syzkaller.appspotmail.com
> Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing from a queue (v4)")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>
> Comments?
> Concerns?
>
> Otherwise I will queue this up and send it to Linus.
>
> kernel/ptrace.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 6f357f4fc859..4c2b24a885d3 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -704,6 +704,10 @@ static int ptrace_peek_siginfo(struct task_struct *child,
> if (arg.nr < 0)
> return -EINVAL;
>
> + /* Ensure arg.off fits in an unsigned */
> + if (arg.off > ULONG_MAX)

if (arg.off > ULONG_MAX - arg.nr)

> + return 0;

maybe we should return EINVAL in this case

> +
> if (arg.flags & PTRACE_PEEKSIGINFO_SHARED)
> pending = &child->signal->shared_pending;
> else
> @@ -711,18 +715,20 @@ static int ptrace_peek_siginfo(struct task_struct *child,
>
> for (i = 0; i < arg.nr; ) {
> kernel_siginfo_t info;
> - s32 off = arg.off + i;
> + unsigned long off = arg.off + i;
> + bool found = false;
>
> spin_lock_irq(&child->sighand->siglock);
> list_for_each_entry(q, &pending->list, list) {
> if (!off--) {
> + found = true;
> copy_siginfo(&info, &q->info);
> break;
> }
> }
> spin_unlock_irq(&child->sighand->siglock);
>
> - if (off >= 0) /* beyond the end of the list */
> + if (!found) /* beyond the end of the list */
> break;
>
> #ifdef CONFIG_COMPAT
> --
> 2.21.0.dirty
>

\
 
 \ /
  Last update: 2019-06-04 20:34    [W:0.235 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site