linux-kernel - Re: [RFC] fs/kcore: change copy_to_user to copy_in

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] [day] [month] [year] [list]

Message-ID: <CA+55aFyZTMcOhr2Z4TZ6p3GLQ30gw4sxSkOFXX1-8EuPFzLGJg@mail.gmail.com>
Date:	Thu, 20 Aug 2015 17:05:47 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	yalin wang <yalin.wang2010@...il.com>
Cc:	Borislav Petkov <bp@...e.de>, Ingo Molnar <mingo@...nel.org>,
	Dave Hansen <dave@...1.net>, bhe@...hat.com,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] fs/kcore: change copy_to_user to copy_in_user

On Thu, Aug 20, 2015 at 1:59 AM, yalin wang <yalin.wang2010@...il.com> wrote:
> -
> -                               n = copy_to_user(buffer, (char *)start, tsz);
> +                               if ((start + tsz < tsz) ||
> +                                               (start + tsz) > TASK_SIZE)
> +                                       return -EFAULT;

This is wrong. You apparently want to have

        if (!access_ok(start, tsz))
                return -EFAULT;

> +                               set_fs(KERNEL_DS);
> +                               n = copy_in_user(buffer, (char *)start, tsz);
> +                               set_fs(USER_DS);

.. and this is actually worse and even less portable than what we have
now, in that it's actively wrong on platforms that may have a user
address and a kernel address with the same value (ie they have
explicitly separate kernel/user address spaces).

Now, that's admittedly unusual, but I think sparc32 actually can do that.

Anyway, I absolutely detest this patch. It replaces one piece of code
that admittedly doesn't work on all architectures because kernel
memory is accessed without testing, with another hack that happens to
work on other architectures and is fragile and prone to be a security
issue.

In other words, I think the end result is _worse_ than the current situation.

You probably want to use "probe_kernel_read()" and do it into a
temporary buffer, and then just do the copy_to_user() from the
temporary buffer. Sure, it's less efficient, but at least it's not
actively wrong and a possible security problem in the long run.

                    Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/