linux-kernel - Re: [PATCH] kernel: sys: fix potential Spectre v1

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]

Message-ID: <20180523150737.ycuulapggtu3hpc3@lakrids.cambridge.arm.com>
Date:   Wed, 23 May 2018 16:07:37 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>
Subject: Re: [PATCH] kernel: sys: fix potential Spectre v1

On Wed, May 23, 2018 at 11:08:40AM +0200, Peter Zijlstra wrote:
> 
> Sorry for being late to the party..

Likewise!

> On Wed, May 23, 2018 at 12:03:57AM -0500, Gustavo A. R. Silva wrote:
> > +#define validate_index_nospec(index, size)                            \
> > +({                                                                    \
> > +       bool ret = true;                                               \
> > +       typeof(index) *ptr = &(index);                                 \
> > +       typeof(size) _s = (size);                                      \
> > +                                                                      \
> > +       BUILD_BUG_ON(sizeof(*ptr) > sizeof(long));                     \
> > +       BUILD_BUG_ON(sizeof(_s) > sizeof(long));                       \
> > +                                                                      \
> > +       if (*ptr >= size)                                              \
> > +               ret = false;                                           \
> > +                                                                      \
> > +       *ptr = array_index_nospec(*ptr, _s);                           \
> > +                                                                      \
> > +       ret;                                                           \
> > +})
> 
> Would not something like:
> 
> 	bool ret = false;
> 
> 	....
> 
> 	if (*ptr < _s) {
> 		*ptr = array_index_nospec(*ptr, _s);
> 		ret = true;
> 	}
> 
> 	ret;
> 
> be more obvious?

I think that either way, we have a potential problem if the compiler
generates a branch dependent on the result of validate_index_nospec().

In that case, we could end up with codegen approximating:

	bool safe = false;

	if (idx < bound) {
		idx = array_index_nospec(idx, bound);
		safe = true;
	}

	// this branch can be mispredicted
	if (safe) {
		foo = array[idx];
	}

... and thus we lose the nospec protection.

I also suspect that compiler transformations mean that this might
already be the case for patterns like:

	if (idx < bound)  {
		safe_idx = array_index_nospec(idx, bound)];
		...
		foo = array[safe_idx];
	}

... if the compiler can transform that to something like:

	if (idx < bound) {
		idx = array_index_nospec(idx, bound);
	}

	// can be mispredicted
	if (idx < bound) {
		foo = array[idx];
	}

... which I think a compiler might be capable of, depending on the rest
of the function body (e.g. if there's a common portion shared with the
else case).

I'll see if I can trigger that in a test case. :/

Thanks,
Mark.