> 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."
> Can you make a recommendation of a process which doesn't involve cherry-pick commits (or more often patch-sets)?
Thank you! You just made me realize that there some ambiguity in the original article and thus in my reply; I have nothing against cherry-pick in git and first thought the original article was using the phrase idiomatically, as in "developers picked specific commits with their personal judgment."
The rest of this response will be written with that idiomatic reading in mind since that's what I originally intended with my criticism.
> 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.
I agree with this. This would make it immediately obvious that there were multiple fixes for one issue, which should be strange.
I also think there should be an overall review process where both:
1. Commits that have been chosen are reviewed by the group
2. Commits that have NOT been chosen should be reviewed by the submitters -- if there's something critical being left out, this is the time to speak up
They may already be doing something like this, for all I know, and there may be other reasons that this happened.
Additionally, I'm only coming to this conclusion because of the way it was presented in the article; the people involved may have a different view of why this happened entirely.
But for me, the way it was presented in the article, it definitely looks like a process failure.
You can only have so much process before you're too far out of the bazaar. This isn't to bring up the relative value tradeoffs of that old split, just to highlight that one shouldn't expect a 'process adjustment' reaction from a fundamentally anti-process collective or for members to even view it primarily through the lens of a process failure. (I'd sooner expect a rant on basic C coding competence even though those days are gone.) That's even if the LKML reflects on this particular timeline of patches much at all; there's no good reason to, really, it's a curiosity. And unlike those of us in a proprietary enterprise no one expects or can insist on an RCA doc with promises to do better backed by planned process tweaks (what issues can't ultimately be recast as process failures?) or new processes.
When I was a developer on some enterprise software, our pattern was that while features were developed against the newest codebase, bug-fixing was done against the oldest supported version that exhibited the bug, and then rolled forward from there to all other releases.
This avoided the cherry picking entirely.
(I understand this wouldn't be a good fit for linux kernel development, but it is an approach I've seen work well)
Maybe linus' fix should have been added as separate commit to a branch instead of putting the fix in the merge commit itself? Then the entire branch could be taken and rebased, or at least the range cherry-picked.
Indeed, this is called an "evil merge" [1], and here's [2] even an email with Linus mentioning it. I'm surprised the blog post doesn't mention evil merges by name, as this seems to be a prime example of why they're called evil.
> 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.
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).
Because a significant portion of those changes will conflict, and one will have to go back to each commit that made the changes to figure out the intention of the change.
Easier to back port each set of changes while knowing what the point of the changes are.
> 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...
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.
> 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.
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.
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.
The recipients have the right without further restrictions for each copy they receive, but the workaround is that if that right is exercised, grsecurity will never send them anything again. Thus, if one values the patchset, which their customers who are paying real money presumably do, they'll refrain from exercising their right to redistribute in order to stay off the blacklist.
That is not a "work around". That is a BLATANT in-writing VIOLATION of the no-restrictions-on-redistribution clause GRSecurity MUST follow in-order to have the PERMISSION to create derivative works.
GRSecurity IS violating the copyrights of the linux copyright holders upon whom it's derivative work is derived from.
They are blatantly violating the copyrights of the linux licensors, as far as we know. The linux licensors should sue.
Another option is to simply revoke the license: Brad Spengler has (as far as we know) a gratis (free) license from the many linux licensors.
Such a license is NOT secured: it's similar to a gratis property license where one can have their permission to use land ended at any time by the property owner (in this case the linux licensors (copyright holders).
According to the GPL text itself: the license has been withdrawn upon violation (note: with a gratis license a violation is /not/ required for revocation: the licensor can simply /decide/ he doesn't want you to continue using his property. A paid GPL license would be an example of a non-gratis free-software license situation where the licensees "rights" are secured and irrevocable (generally), a gratis free-software license simply extends /requirements/ onto the licensee and no /duties/ upon the licensor)
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.
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.
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.
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.
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.
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.
Would you seriously argue that "works most of the time" does not describe C? Or are you just saying that accepting it for another language is not okay, and anything new allowed into the kernel needs to be perfect instead of just better than C?
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?
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.
Error handling without exceptions tends to 3x to 5x as much raw code, with corresponding numbers of errors and poorly-exercised paths. Exception paths rely mostly on destructors which are otherwise well-tested in normal operation.
Yeah, the blame is about 50-50 on C89/90 vs the developer. There are at least 2 different ways to fix the warning that are brainless and semantically identical:
{
SNIP1
X Y = Z;
SNIP2
}
Either:
{
X Y;
SNIP1
Y=Z;
SNIP2
}
or
{
SNIP1
{
X Y = Z;
SNIP2
}
}
But the need for doing this really should be removed; I agree that every compiler that can compile the linux kernel can compile the original code correctly and that most of them can do so without wholesale opting in to C99. Furthermore VLAs are the only truly crazy part of C99, so it would be nice to transition to C99 minus VLA, which is something that most compilers can enforce (gcc has -Wvla).
Yeah - the original code would have been just fine, wouldn't it? It looks like the warning actually made things worse (although I can't imagine what was going through the mind of the guy who swapped the lines to silence the warning...)
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.
Speaking as someone who at work (big corp) has to do semantic loop-de-loops and contortions to sugarcoat even the mildest criticism of sometimes utterly stupid ideas, just to avoid ruffling feathers and be politically correct, I find this refreshing. It seems to me that the kernel developers plain and simple fucked up, and grsecurity people are right to call them out on this.
I don't find anything but objective summary of what happened in their post. No hostility at all. But that's just me.
They are often in the "very correct, but unnecessarily mildly hostile" area. I wonder if they could gain way more public support just by having someone moderate their posts for PR.
That is kinda their business model though, isn't it? I mean, they're trying to sell you their added security patches, so it's in their interest to convince you that they alone know what they're doing and everyone else incompetent.
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".
I have a guy at work who acts very stern and has strong opinions in discussions; Often he isn't right. Being stern and asshole-like doesn't make your right.
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).
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.
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.
I think there are some static analysis tool. At first it was some proprietary tools ran by who knows who, but this was obviously an enormous problem in the context of Linux dev, so IIRC somebody wrote something (that might be quite primitive but that is better than nothing) to detect potential cases. Anyway, a compiler plugin is a neat idea. It won't detect everything but at least it can detect (and fix!) local cases. Not sure if the open source analyzers (if any) can detect less local cases, though -- but then with this pattern most cases should be quite local, so I might actually be the best to have something simple.
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)."
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...
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.
I did read it till the end, but I got confused by the sentence "To clarify why this fix is bad, ..." in the paragraph after Linus's fix, so from there on I thought the article is talking about his fix of the fix. My bad.
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?
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.
C people don't like to think about how much C took from C++. const, "//" comments, function prototypes, more recently declarations anywhere in the block.
The author's intention is criticizing the ad-hoc, error-prone approach on fixing Spectre vulnerabilities. It said,
> 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). But what if we hadn't backported the fix, or what if the issue was one of the many other Spectre instances lurking in the kernel?
I bet for 100 dollars, that at least another unfixed Spectre vulnerability already exists in the kernel, the Chinese government has no reason to waste energy on inserting a broken fix.
Note: The author talks about the private complier solution of his company at the end of the article (unfortunately, on one hand, the author is well-known for developing various unique security solutions, on the other hand the hostile relation between the author and the mainline kernel community is well-known, it's unfortunately that no other independent developers are working with mainline kernel community towards a similar solution).
Saving mention of the proprietary tool to the end showed restraint. That such failures are detectable by tooling is important information, even when not everybody has the tool. Maybe especially then.
Actually, you missed my point. I was not talking about the article's author, I was talking about the Chinese developer who "fixed" the code, that triggered the entire avalanche in the code repository.
First. I said "the author's intention is criticizing the ad-hoc, error-prone approach" on fixing Spectre vulnerabilities. Please consider what did I really mean. I meant: It is believed that this type of mistake is inevitable, regardless of who writes the code, in the end, a mistake like this one will be made, and probably have already been made, much earlier than the reported one, and still undetected. If you get this message, "who did it" becomes a unimportant question.
Second, it's not some 0day fixes, but a Spectre fix, precisely, a Spectre variant 1 fix. This distinction is important - everything today is still largely vulnerable to Spectre to an extent, and this situation is unlikely to change. For Spectre v1, the outcome is a simple infoleak, among all Spectre vulnerabilities, its risk considered relatively low.
The current Linux kernel's approach to Spectre v1 is plugging them one by one. As the Linux kernel documentation explains,
> Vulnerable code uses nospec accessor macros for "bounds clipping", to avoid any usable disclosure gadgets. However, this may not cover all variant 1 attack vectors.
It means a lot of potential Spectre v1 vulnerability already exist in the kernel (and elsewhere), but they're probably not too useful for the attack without another exploit.
Third, the defective patch does not introduce any vulnerability. The original code was a potential Spectre v1 exploit gadget, and a Chinese developer noticed that in a code review, and submitted a patch to fix it. However, the patch generated a compiler-warning. Linus Torvalds fixed it, but it evaded the detection from maintainers, and finally a broken version created by the stable kernel maintainers got merged into all stable trees.
And let's assess the situation.
1. More than one potential Spectre v1 vulnerability gadgets already exist in the kernel, regardless of anyone's actions. It means all the deployed production systems all across the Internet are already somewhat vulnerable.
2. In the kernel ptrace code, there was a Spectre gadget. Again, it means all the deployed production systems all across the Internet are already vulnerable to it.
3. A Chinese developer submitted a "fix". Linus noticed the fix was broken, but the kernel maintainers picked up the old patch.
4. As a result, (1) no new fix is introduced, no new bug is introduced, (2) all the deployed production systems all across the Internet are still vulnerable to the same Spectre gadget, just like before. And (3) the head of a infosec company noticed it and published an article to criticize the mainline kernel. (4) It created a lot of buzz and the problem is fixed. There is nothing even remotely close to "the entire avalanche in the code repository", the code repository worked exactly like before.
In conclusion, we ask, cui bono - "to whom is it a benefit?".
If I was the security service of the Chinese government, my intention could never be inserting a malicious vulnerability, because the same type of vulnerability already existed, I probably know a lot of different ones as well. Further, even if the gadget in ptrace is the only vulnerability I know, by submitting a false fix, it did not introduce any additional vulnerability, but in fact greatly increased the chance of the fix being noticed and fixed. I gained nothing by going through all of these troubles by first creating a good patch which generates a compiler warning to trick maintainers to ruin the patch while attempting to fix it.
So we can rule out the "Chinese hacker" meme, it doesn't make sense at all. If you still insist, the only possibilities remained are,
1. The intention of the attacker by submitting a false fix is to mislead the kernel developers to believe the fix is good. So that nobody would notice the same problem in the future, so that I can use it as a long-term backdoor, not only a short-term one.
Certainly possible, but unlikely. By submitting the code, the risk of being discovered is much higher than the benefit of keeping the original vulnerability undetected, because it generates a compiler warning. Even worse, regardless of the compiler warning is being noticed or not, in either way, I ended up risking to patch my own backdoor. Not to say the original vulnerability is something of little value.
2. The intention of the attacker by submitting a false fix is to start a propaganda campaign and discredit the competence of kernel developers. Or to examine the abilities of kernel maintainers to avoid making mistake, or some other grand scheme of chess.
If I am the project leader of a Chinese operating system, perhaps I'd like to do this, as it would be beneficial to me for citing it as my evidence later to push my project's agenda. Another scenario would be 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.
Again, certainly possible, but unlikely, and very stupid.
In conclusion, if you cannot understand even the basic facts (such as the nature of Spectre vulnerability), fail to comprehend the basic idea of an simple article like this one, and fail to understand the comments, I suggest you to stop posting uninformed comments.
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."