linux-kernel - Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK

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:   Mon, 2 Jul 2018 14:20:16 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Paul McKenney <paulmck@...ux.vnet.ibm.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Andy Lutomirski <luto@...capital.net>,
        Dave Watson <davejwatson@...com>, Paul Turner <pjt@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Russell King - ARM Linux <linux@....linux.org.uk>,
        Ingo Molnar <mingo@...hat.com>, Peter Anvin <hpa@...or.com>,
        Andi Kleen <andi@...stfloor.org>,
        Christoph Lameter <cl@...ux.com>, Ben Maurer <bmaurer@...com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Josh Triplett <josh@...htriplett.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Joel Fernandes <joelaf@...gle.com>
Subject: Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields,
 validate abort_ip < TASK_SIZE

On Mon, Jul 2, 2018 at 2:03 PM Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
>
>         /* Ensure that abort_ip is not in the critical section. */
>         if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
>                 return -EINVAL;
> ...
> What underflow issues are you concerned with ?

That.

Looking closer, it looks like what you want to do is

     if (rseq_cs->abort_ip >= rseq_cs->start_ip && rseq_cs->abort_ip <
rseq_cs->start_ip + rseq_cs->post_commit_offset)

but you're not actually verifying that the range you're testing is
even vlid, because "rseq_cs->start_ip + rseq_cs->post_commit_offset"
could be something invalid that overflowed (or, put another way, the
subtraction you did on both sides to get the simplified version
underflowed).

So to actually get the range check you want, you should check the
overflow/underflow condition. Maybe it ends up being

        if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
                return -EINVAL;

after which your simplified conditional looks fine.

But I think you should also do

        if (rseq_cs->start_ip + rseq_cs->post_commit_offset > TASK_SIZE)
                return -EINVAL;

to make sure the range is valid in the first place.

             Linus