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 10:14:44 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
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 Tue, Sep 3, 2019 at 9:56 AM Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
>
> Then I must be misunderstanding something.
>
> probe_kernel_address() is a macro wrapping probe_kernel_read().

Don't look at probe_kernel_address().

As long as you only look at that, you will be missing the big picture.

Instead, look at the code below it:

        /*
         * Pairs with atomic_dec_and_test() in put_task_struct(). If this task
         * was already freed we can not miss the preceding update of this
         * pointer.
         */
        smp_rmb();
        if (unlikely(task != READ_ONCE(*ptask)))
                goto retry;

That code is the code that verifies "ok, the pointer was valid over
the whole sequence, so the probe_kernel_address() must have succeeded"

So the code *does* check for success, but it does so using a
*stronger* check than the return value of probe_kernel_address().

If the task on the runqueue hasn't changed, then the
probe_kernel_read() cannot have failed.

But the reverse test is not true: if the probe_kernel_read()
succeeded, that doesn't guarantee that the value we read was
consistent.

So the check for failure is there, and the check that does exist is
the correct and stronger check.

Which is why checking the return value of probe_kernel_read() is
immaterial and pointless.

But a comment about this above the probe_kernel_read() may indeed be
worth it, since it seems to be unclear to so many people.

The code basically just wants to do a kernel memory access, knowing
that it's speculative. And the _only_ reason for using
probe_kernel_read() is that with DEBUG_PAGEALLOC you might have a page
fault on the speculative access.

But we do the speculation verification check afterwards, and that's
the important part.

                      Linus