linux-kernel - Re: [RFC PATCH] asm/generic: introduce if_nospec and nospec

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:   Fri, 5 Jan 2018 14:40:35 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     "torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "alan@...ux.intel.com" <alan@...ux.intel.com>,
        "Reshetova, Elena" <elena.reshetova@...el.com>,
        "gnomes@...rguk.ukuu.org.uk" <gnomes@...rguk.ukuu.org.uk>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "jikos@...nel.org" <jikos@...nel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>
Subject: Re: [RFC PATCH] asm/generic: introduce if_nospec and nospec_barrier

On Thu, Jan 04, 2018 at 02:09:52PM -0800, Dan Williams wrote:
> On Thu, Jan 4, 2018 at 3:47 AM, Mark Rutland <mark.rutland@....com> wrote:
> > Hi Dan,
> >
> > Thanks for these examples.
> >
> > On Thu, Jan 04, 2018 at 03:10:51AM +0000, Williams, Dan J wrote:
> >> Note, the following is Elena's work, I'm just helping poke the upstream
> >> discussion along while she's offline.
> >>
> >> Elena audited the static analysis reports down to the following
> >> locations where userspace provides a value for a conditional branch and
> >> then later creates a dependent load on that same userspace controlled
> >> value.
> >>
> >> 'osb()', observable memory barrier, resolves to an lfence on x86. My
> >> concern with these changes is that it is not clear what content within
> >> the branch block is of concern. Peter's 'if_nospec' proposal combined
> >> with Mark's 'nospec_load' would seem to clear that up, from a self
> >> documenting code perspective, and let archs optionally protect entire
> >> conditional blocks or individual loads within those blocks.
> >
> > I'm a little concerned by having to use two helpers that need to be paired. It
> > means that we have to duplicate the bounds information, and I suspect in
> > practice that they'll often be paired improperly.
> 
> Hmm, will they be often mispaired? All of the examples to date the
> load occurs in the same code block as the bound checking if()
> statement.

Practically speaking, barriers get misused all the time, and having a
single helper minimizes the risk or misuse.

> > I think that we can avoid those problems if we use nospec_ptr() on its own. It
> > returns NULL if the pointer would be out-of-bounds, so we can use it in the
> > if-statement.
> >
> > On x86, that can contain the bounds checks followed be an OSB / lfence, like
> > if_nospec(). On arm we can use our architected sequence. In either case,
> > nospec_ptr() returns NULL if the pointer would be out-of-bounds.
> >
> > Then, rather than sequences like:
> >
> >         if_nospec(idx < max) {
> >                 val = nospec_array_load(array, idx, max);
> >                 ...
> >         }
> >
> > ... we could have:
> >
> >         if ((elem_ptr = nospec_array_ptr(array, idx, max)) {
> >                 val = *elem_ptr;
> >                 ...
> >         }
> >
> > ... which also means we don't have to annotate every single load in the branch
> > if the element is a structure with multiple fields.
> 
> We wouldn't need to annotate each load in that case, right? Just the
> instance that uses idx to calculate an address.

Correct.

> if_nospec (idx < max_idx) {
>    elem_ptr = nospec_array_ptr(array, idx, max);
>    val = elem_ptr->val;
>    val2 = elem_ptr->val2;
> }
> 
> > Does that sound workable to you?
> 
> Relative to the urgency of getting fixes upstream I'm ok with whatever
> approach generates the least debate from sub-system maintainers.
> 
> One concern is that on x86 the:
> 
>     if ((elem_ptr = nospec_array_ptr(array, idx, max)) {
> 
> ...approach produces two conditional branches whereas:
> 
>     if_nospec (idx < max_idx) {
>         elem_ptr = nospec_array_ptr(array, idx, max);
> 
> ....can be optimized to one conditional with the barrier.

Do you mean because the NULL check is redundant? I was hoping that the
compiler would have the necessary visibility to fold that with the
bounds check (on the assumption that the array base must not be NULL).

Thanks,
Mark.