linux-kernel - Re: [PATCH v2] rslib: Remove VLAs by setting upper bound on nroots

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, 27 Mar 2018 16:45:32 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Segher Boessenkool <segher@...nel.crashing.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kernel Hardening <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH v2] rslib: Remove VLAs by setting upper bound on nroots

On Mon, 26 Mar 2018 16:17:57 -0700 Kees Cook <keescook@...omium.org> wrote:

> On Fri, Mar 16, 2018 at 11:25 PM, Kees Cook <keescook@...omium.org> wrote:
> > On Fri, Mar 16, 2018 at 3:59 PM, Andrew Morton
> > <akpm@...ux-foundation.org> wrote:
> >> On Thu, 15 Mar 2018 15:59:19 -0700 Kees Cook <keescook@...omium.org> wrote:
> >>
> >>> Avoid stack VLAs[1] by always allocating the upper bound of stack space
> >>> needed. The existing users of rslib appear to max out at 24 roots[2],
> >>> so use that as the upper bound until we have a reason to change it.
> >>>
> >>> Alternative considered: make init_rs() a true caller-instance and
> >>> pre-allocate the workspaces. This would possibly need locking and
> >>> a refactoring of the returned structure.
> >>>
> >>> Using kmalloc in this path doesn't look great, especially since at
> >>> least one caller (pstore) is sensitive to allocations during rslib
> >>> usage (it expects to run it during an Oops, for example).
> >>
> >> Oh.
> >>
> >> Could we allocate the storage during init_rs(), attach it to `struct
> >> rs_control'?
> >
> > No, because they're modified during decode, and struct rs_control is
> > shared between users. :(
> >
> > Doing those changes is possible, but it requires a rather extensive
> > analysis of callers, etc.
> >
> > Hence, the 24 ultimately.
> 
> Can this land in -mm, or does this need further discussion?

Grumble.  That share-the-rs_control-if-there's-already-a-matching-one
thing looks like premature optimization to me :(

I guess if we put this storage into the rs_control (rather than on the
stack) then we'd have to worry about concurrent uses of it.  It looks
like all the other fields are immutable once it's set up so there might
be such users.  In fact, I suspect there are...