
Teardown of a Failed Linux LTS Spectre Fix - pentestercrab
https://grsecurity.net/teardown_of_a_failed_linux_lts_spectre_fix.php
======
sdrothrock
> The takeaways here are that there is a real benefit to having an
> external/independent review and backporting process like the one we perform
> for our customers

That's great for sales for these people, but I think the real takeaway here
is:

> when the actual merge of the tree was performed, no mention was made of
> [Linus's] correction to the [original] fix, and with no specific commit
> mentioning the correction and fixing it alone, everyone else's processes
> that depended on cherry-picking specific commits ended up grabbing the bad
> warning-inducing change. As a further failure, instead of looking at Linus'
> correct fix (observable by checking out the master tree at the time), the
> approach employed in the LTS kernels seems to have been to naively silence
> the warning

There are a LOT of process failures in this description of what happened. At
the kernel development level, that should be very concerning.

It's good that an external auditor found it (hooray open source), but this was
almost definitely an easily preventable mistake with a better process.

> everyone else's processes that depended on cherry-picking specific commits

This alone sounds horrifying.

I don't know anything about kernel development, so I don't want to sit here
and judge and offer advice out of ignorance, but I really do feel like there's
a better process that would work for them that doesn't involve people "cherry-
picking specific commits."

~~~
richardwhiuk
Can you make a recommendation of a process which doesn't involve cherry-pick
commits (or more often patch-sets)?

Note, your process needs to allow a fix to be developed against V5.1, and
back-ported to V4.1.

I suspect one of the problems here is a lack of bug tracker, which would have
associated the fix to the merge and the fix itself together.

~~~
CalChris
> Can you make a recommendation of a process which doesn't involve cherry-pick
> commits (or more often patch-sets)?

This is for LTS, Long Term Support. Why would the maintainers be cherry
picking candidate patch sets at all? Shouldn't they wait for a release and
back port from that? Regular release intervals are 8-10 weeks.

That would be my recommendation, backport only from releases. This is
especially the case here because it was a security fix.

~~~
cyphar
Patches have to be in Linus's tree before they land in -stable. That's been
the policy for at least as long as I remember. Also, Greg does releases of
-stable every week or two -- waiting 8 weeks for patches (that Linus has
already merged) makes very little sense.

Yes, fixup patches are something that need to be handled separately but Greg
has always handled those properly (and good kernel devs use the Fixes: tags
and always Cc: stable on those fixup patches). Not to mention you'd need to
handle fixup patches even if you did depend on releases (if a bug was
introduced in a released version it's usually fixed by the next -rc1 -- so
waiting means the newest kernel is broken needlessly for 8 weeks).

------
Jach
> Finally, it should be noted that the "many eyes" of the upstream community
> failed to notice this flaw; without this blog post it would have likely
> persisted for many years.

Such a spicy remark, this whole post is great. (Though "many eyes" typically
refers to an oft-claimed quality of open source generally with no arbitrary
distinctions of upstream / downstream. grsecurity makes up some of those
valued eyes for Linux!)

This makes me wonder what other logic errors were introduced into the kernel
because of an incorrect resolution to a warning...

------
justinjlynn
It's wonderful of grsecurity to produce a review like this - it'll be
extremely helpful in addressing any issues in the development and patch
management process. One can only think what improvements we might see in
regards to Linux security, as a whole, if they'd work more proactively with
the rest of the community. Of course, they're under no obligation to do so.

Unfortunately, I doubt that we'll ever have their participation - or even see
or review any of grsecurity's modifications - all because of grsecurity's
"Access Agreement". [0] Essentially, if I understand it correctly, even if
grsecurity's customers wanted to, by sharing grsecurity's work with us or
anyone else, those customers stand to have their access to the latest
grsecurity created derivatives of the Linux kernel revoked. Of course, if
that's true, facing a penalty for sharing code grsecurity received and
modified per the GPL just doesn't sound right or just to me.

It seems obvious to me that one must carefully consider the wider picture when
evaluating linux security posture evaluations, as presented by grsecurity, as
there may be conflicts of interest in effect. I take what grsecurity says
about the security of the Linux Kernel with a very large grain of salt, and
you should as well.

Further, I'm not a legal expert and I use measured tones as grsecurity have
taken legal action against open source community members in the past for
expressing their opinions [1] on the matter of the access agreement [2]. While
those matters were dismissed by the court [3], I am still hesistant to say
anything, but find speaking on this matter a neccessary thing to do, for what
I perceive to be the good of the community.

[0]
[https://grsecurity.net/agree/agreement_faq.php](https://grsecurity.net/agree/agreement_faq.php)

[1] [https://perens.com/2017/06/28/warning-grsecurity-
potential-c...](https://perens.com/2017/06/28/warning-grsecurity-potential-
contributory-infringement-risk-for-customers/)

[2]
[https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecu...](https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecurity_sues_bruce_perens_for_defamation/)

[3] [https://perens.com/wp-
content/uploads/sites/4/2017/12/file0....](https://perens.com/wp-
content/uploads/sites/4/2017/12/file0.43502131597246.pdf)

~~~
progval
> Of course, if that's true, facing a penalty for sharing code grsecurity
> received and modified per the GPL just doesn't sound right or just to me.

The GPL requires distributing the modified source to people you distributed a
binary to. If grsecurity's clients don't distributed a binary, they don't have
to distribute a source.

EDIT: though there's this clause, and I'm not quite sure what to make of it:

> Each time you redistribute the Program (or any work based on the Program),
> the recipient automatically receives a license from the original licensor to
> copy, distribute or modify the Program subject to these terms and
> conditions. You may not impose any further restrictions on the recipients'
> exercise of the rights granted herein.

~~~
fragmede
GRSecurity has the same, odious, if legal posturing as RedHat - the GPL
requires the _current_ version's source (which customers have access to), be
made available to customers. Upon sharing of the current version's source in a
useful format to the public, _licensing_ for future version is denied - ie,
you cannot be a future customer of RedHat. Thus you don't have a right to the
binaries and thus neither the source.

RedHat, gets away with distributing the source to their modified kernel as a
giant patch file to the vanilla kernel because adheres to the letter of the
GPL but not the "spirit", insofar as a law can have a spirit, and insofar as I
can claim what the spirit even is. Business-wise though, RedHat is able to get
away with it as a billion dollar company. Spender (of GRSecurity) is smarter
than I, but has tried to build his product upon something that requires more
smarts and more resources - that he is too fiercrely protective of - than he
is willing to trade rights to, in order for his contributions to be available
to the wider community, as he purports to desire. Facebook's kernel
modifications, nor Google's "interesting" kernel modifications (as defined by
those requisite for running Google.com and other properties) aren't made
available, nor are they t required to, by the GPL. (Android's Linux kernel
sources and select other kernel changes _are_ made available.)

The GPL allows building a business by providing support, and/or tuning of
kernel parameters (eg VM swappiness, or disk scheduler deadlines) for running
highly specialized workloads, eg Oracle DB. If providing a highly successful
product, eg Google.com or Facebook internal knowledge will naturally follow.
External benefits not necessarily so. The fat client of the pre and early
Internet made the GPL (and LGPL) sufficient for the time, but things are
different on the cloudy Internet, and arguably the broader community suffers
for it.

~~~
misc228
Redhat distributes it's sources to everyone, thus making any violation of the
no-restrictions-on-redistribution clause acedemic to the linux copyright
holders who would be the claimants.

GRSecurity successfully prevents redistribution: it's violation is blatant and
NOT academic.

A court can be shown the attempt and it's successful conclusion.

Quite different in substance from RedHat's possible violation.

------
pornel
C90 is partly to blame is here. It rejects sensible code due to limitations of
hardware and compilers older than Linux itself.

I find it hard to believe that a decades old compiler with an actual compile-
in-one-pass limitation would produce something useful from the current kernel
source. It's affecting the the entire kernel development just so that someone
somewhere with an abandonware compiler doesn't get a shock of updating it once
every 30 years.

~~~
temac
Linux only compiles with at least semi-recent compilers. The minimum version
for GCC even has been bumped not too long ago. They certainly are able to
handle C99 style declarations. Why Linus hate them, I don't know, probably he
could be able to formulate some pseudo justifications when in reality it is
just what he is used to.

The only thing I could find quickly was from 2005:
[https://lkml.org/lkml/2005/12/13/223](https://lkml.org/lkml/2005/12/13/223)

Maybe it made sense in 2005, but 14 years later this _might_ not be that much
important anymore. To be honest I'm radically against the opinion that
"Putting variables in the middle of code only improves readability when you
have messy code", but I kind of understand where it comes from, with an
hardcore old-school C mindset. With experience with more modern programming
languages and considering code like more abstract level object instead of low
level immediate imperative instructions, not putting declarations before the
variable are needed makes sense. This is more in line with how modern compiler
work AND more importantly with what we need to do to _reason_ on code (not
surprising because part of the reasoning is the same for a modern compiler and
for us), and I _suspect_ that this actually leads to less programming errors,
including the silly ones we saw here.

~~~
ncmncm
It is of a piece with forbidding link-compatible C++ components. Destructors
prevent far more mistakes than they cause, and templates, used correctly, are
much more specific and less error-prone than macros.

~~~
kevin_thibedeau
C++ is broken without exceptions. Exceptions aren't ever going to enter the
kernel even if you tried to hide them behind a C ABI.

~~~
nickpsecurity
The folks behind the Pistachio microkernel said they used C++:

[https://github.com/l4ka/pistachio](https://github.com/l4ka/pistachio)

I don't know if it has exceptions or an unusual style since I don't know C++.
It is written by CompSci researchers which usually means a different style.
Just throwing it out there for anyone considering C++ in kernels since the L4
kernels were all really fast and lean.

~~~
kevin_thibedeau
And Google merrily uses C++ with exceptions disabled. That doesn't make it
right when the shit hits the fan. "Works most of the time" is not acceptable
in an OS.

~~~
nickpsecurity
Your statement equates not using exceptions with no error handling at all. Are
exceptions the only way C++ can handle errors? Or can it do things like return
codes used in C or advanced constructs via its metaprogramming?

~~~
tomjakubowski
You can use error codes or, better, a Result<T, E> template in C++, but
throwing an exception is the only sane way to signal an error from a C++
constructor that I know of. The alternatives are awkward: you either stick an
error code on the struct, which the caller can check after construction, or
write an error code to the caller through an out parameter. In either case,
the class implementation now must be concerned with handling objects in some
partly-initialized, "error" state.

------
jandeboevrie
Man that attitude of the grsecurity folks is bad. Oh look how great our
process and fix is and how fast we are. And look how bad them kernel folks
are. Yuk.

~~~
ncmncm
But they are correct.

Correctness should forgive any amount of "tone", if indeed we care about
correctness.

~~~
rndgermandude
No, it should not. I am no stranger to be borderline rude sometimes, also in
part due to cultural differences and language barrier, so I am pretty
forgiving when it comes to other people doing the same.

But this... what they did is tooting their own horn, at the expense of others,
and that annoys me. "look how great we are and how much the others suck".

------
misc228
GRSecurity is violating the GPL, and thus violating the linux programmers
copyrights, as Bruce Perens explained:
[https://linux.slashdot.org/story/17/07/09/188246/bruce-
peren...](https://linux.slashdot.org/story/17/07/09/188246/bruce-perens-warns-
grsecurity-breaches-the-linux-kernels-gpl-license)

[http://perens.com/blog/2017/06/28/warning-grsecurity-
potenti...](http://perens.com/blog/2017/06/28/warning-grsecurity-potential-
contributory-infringement-risk-for-customers/)

Brad Spengler / PaxTeam / GRSecurity have no right to modify the linux code
nor redistribute derivative works of it thusly.

Placing a "no redistribution or else" clause in a "separate writing" does not
absolve one from the requirement NOT to stymie the linux programmer's
stipulation in their license that such restrictions NOT be created by
distributees or those who have created derivative works.

The linux programmers should sue Spengler, or simply revoke his license
(another option: gratis licenses are freely revocable: they are not secured by
an interest, and as-far-as-we-know Spengler has not payed the linux licensors
for a license).

------
archi42
There was another post on this here:
[https://news.ycombinator.com/item?id=20871727](https://news.ycombinator.com/item?id=20871727)
(10h ago, pointing to a short excerpt on lwn.net; 3 comments)

Though I like this one more, as it points to the source.

------
sambe
I'm struggling to see how the backporters thought this was an acceptable
transformation. I don't know any of this code, and even the warning seems a
little opaque to me initially. But it seems very obvious that you can't just
reorder the lines.

~~~
human20190310
The most surprising thing to me is that one submitter gets dinged for a line
break in review, yet another submitter can re-order logic, (seemingly) without
being reviewed at all.

~~~
mzs
It's almost as if grsecurity is implying this was not an innocent mistake on
Greg KH's part and I may not be the only person that noticed this today.
[https://flak.tedunangst.com/post/warning-implicit-
backdoor](https://flak.tedunangst.com/post/warning-implicit-backdoor)

------
atheowaway4z
Does anybody know what triggered the original fix in the first place? Just
grep-ing?

If the kernel folks did what this company did . I.e have the compiler detect
vulnerable code . The issue would have been avoided as well right ?

~~~
wyldfire
The article describes grsecurity's 'respectre' compiler plugin [1].

[1]
[https://grsecurity.net/respectre_announce.php](https://grsecurity.net/respectre_announce.php)

~~~
mannykannot
While they say that Respectre would have caught it, the actual discovery was
apparently made this way:

"We independently backported the fix on July 9th 2019 and on noticing the
warning, fixed it correctly. When the upstream kernels later backported their
bad fix, it created a conflict in our git repo that led to us immediately
spotting their flaw (and keeping our existing fixes)."

------
mehrdadn
Is it just me or there an elephant in the room no one is talking about?

To me, so much blame lies with C's arcane rules, in this case for forcing you
to declare variables earlier than you need them. If the compiler hadn't
complained pointlessly and just let the programmer declare variables where
they're needed, people wouldn't have had to try to "fix" it to begin with.

Apparently people would prefer to blame the kernel developers than the
immaculate language that is C though...

------
phil9987
So Linus fucked it up?!

~~~
segfaultbuserr
No. According to the author, Linus Torvalds was the only person who've noticed
the problem in the final merge and applied a fix manually before merging it to
the mainline. However, the stable kernel trees picked up the old patch, and
the broken version was merged to all stable trees silently. The author blames
Greg K.H. for fxxking it up.

~~~
kanox
> Linus Torvalds was the only person who've noticed the problem in the final
> merge and applied a fix manually before merging it to the mainline

The fix was done as part of the merge commit itself, it should have been a
separate patch.

~~~
phil9987
So one could argue that Linus kinda failed in fixing the problem he has
identified correctly.

------
unnouinceput
So the only question remains, based on history of Chinese government (well,
not only them but in this case the developer was a Chinese) wanting to weaken
the security of software - was this intentional, and then the Chinese
developer is a mole from them, or was a honest mistake?

~~~
microtherion
The fix as submitted by the Chinese author was secure, but it triggered a
compiler warning because it was not standard conforming C.

A kernel maintainer applied an incorrect fix for this warning, which ended up
defeating the purpose of the fix.

I don’t think you can blame the author for the bad fix.

~~~
tedunangst
They're clearly playing 4D chess and submitted a patch with a warning in
anticipation of the next guy breaking it while trying to fix it.

~~~
segfaultbuserr
Or that the author of the article and his infosec company has connection to
the Chinese government, so he asked the Chinese security service to do an
inside job, and he would expose it later to increase the sales of grsecurity,
which in turns support the secret service...

It is one of the only few logical conclusions one can draw if one insists on
OP's opinion that the Chinese attackers are playing 4D chess. The chance is
non-zero, but doesn't make much sense.

