linux-kernel - Re: [RFC PATCH 1/3] Fix: sched: task_rcu_dereference: check probe_kernel

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]

Date:   Tue, 3 Sep 2019 12:56:34 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Oleg Nesterov <oleg@...hat.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        "Russell King, ARM Linux" <linux@...linux.org.uk>,
        Chris Metcalf <cmetcalf@...hip.com>,
        Chris Lameter <cl@...ux.com>, Kirill Tkhai <tkhai@...dex.ru>,
        Mike Galbraith <efault@....de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/3] Fix: sched: task_rcu_dereference: check
 probe_kernel_address return value

----- On Sep 3, 2019, at 12:12 PM, Linus Torvalds torvalds@...ux-foundation.org wrote:

> On Tue, Sep 3, 2019 at 9:00 AM Mathieu Desnoyers
> <mathieu.desnoyers@...icios.com> wrote:
>>
>> probe_kernel_address can return -EFAULT on error, which leads to use of
>> an uninitialized or partially initialized sighand variable.
> 
> I think this comment and this code is actively misleading.
> 
> There is no "uninitialized or partially initialized sighand variable".
> That's completely wrong.
> 
> The sighand variable is always completely initialized. It's just that
> the check for "is it initialized" is _not_ the return value from
> probe_kernel_address(), because that return value is simply not
> sufficient.
> 
> So this is just wrong. Don't do it. You're just confusing the issue,
> and you're making statments that aren't true in the commit message,
> and making the code do a pointless and odd check.
> 
> If you want to change this code for legibility, you should just add a
> comment above the probe_kernel_address() about why the return value is
> ignored, and why the check _below_ that code verifies the value of
> sighand with a different check.

Then I must be misunderstanding something.

probe_kernel_address() is a macro wrapping probe_kernel_read().
mm/maccess.c:probe_kernel_read() calls probe_read_common()
mm/maccess.c:probe_read_common() calls __copy_from_user_inatomic()

include/linux/uaccess.h:__copy_from_user_inatomic() documents:

 * NOTE: only copy_from_user() zero-pads the destination in case of short copy.
 * Neither __copy_from_user() nor __copy_from_user_inatomic() zero anything
 * at all; their callers absolutely must check the return value.

So considering that comment, I suspect the on-stack sighand variable
within task_rcu_dereference() can be left either uninitialized or
(less likely) partially initialized if probe_kernel_address() returns
-EFAULT.

Is there anything else that prevents probe_kernel_address from failing ?
If so, why use probe_kernel_address in the first place ?

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com