16 ms·

Teardown of a Failed Linux LTS Spectre Fix

171 points by pentestercrab 7y ago

sambe 7y ago
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 7y ago
  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 7y ago
    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
jandeboevrie 7y ago
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.
- viraptor 7y ago
  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.
- ncmncm 7y ago
  But they are correct. Correctness should forgive any amount of "tone", if indeed we care about correctness.
  - unixhero 7y ago
    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.
    - ncmncm 7y ago
      In this case the author is right, so your guy's faults do not figure. Stern, opinionated, and wrong are obviously worse than just wrong.
  - mannykannot 7y ago
    It is no good being correct if you cannot communicate it, and the most effective communication drops the tone and sticks to the facts. Having said that, I do not think, in this case, there is significant tone/attitude. Naming names was not necessary, however.
  - rndgermandude 7y ago
    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".
    - pg_is_a_butt 7y ago
      oh, aren't you so smart and everyone else so uncontrollably rude. just monsters. not like you... you're way better. an ignorant hypocrite, perhaps, but not rude.
- AdmiralAsshat 7y ago
  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.
- pferde 7y ago
  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.
- vardump 7y ago
  Thank you grsecurity for the patch. They provided a valuable fix. If they consider some chest bumping helps them to get more business to support their future freebie bug hunts, I'd call it fair. Also, I think bragging rights motivate a lot of white hats doing valuable work for free. Would we be better off without them? That said, I'm not saying being impolite is ok.
- presidentscroob 7y ago
  grsec has a history of not playing well with others, especially when they took away the source and went $$$ payware only. Took away public stable patches: https://news.ycombinator.com/item?id=10128925 https://news.ycombinator.com/item?id=10128925 Took away public test patches: https://news.ycombinator.com/item?id=14205145 https://news.ycombinator.com/item?id=14205145 When they sued a blogger for defamation: https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecurity_sues_bruce_perens_for_defamation/ https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecu... Classic Coke Linus on grsec patches: https://www.spinics.net/lists/kernel/msg2540934.html https://www.spinics.net/lists/kernel/msg2540934.html Being a dick doesn't affect the quality and/or merits of code, it just makes some readers fall for their opinions more readily while turning off others.
- deleted 7y ago
  [deleted]
justinjlynn 7y ago
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-contributory-infringement-risk-for-customers/ https://perens.com/2017/06/28/warning-grsecurity-potential-c... [2] https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecurity_sues_bruce_perens_for_defamation/ https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecu... [3] https://perens.com/wp-content/uploads/sites/4/2017/12/file0.43502131597246.pdf https://perens.com/wp-content/uploads/sites/4/2017/12/file0....
- progval 7y ago
  > 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.
archi42 7y ago
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.
phil9987 7y ago
So Linus fucked it up?!
- unnouinceput 7y ago
  did you read the article or only one phrase of it, the one about Linus? He did a correct fix but was ignored by the other developer.
  - phil9987 7y ago
    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.
- segfaultbuserr 7y ago
  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 7y ago
    > 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 7y ago
      So one could argue that Linus kinda failed in fixing the problem he has identified correctly.
  - phil9987 7y ago
    Makes sense, thanks for the clarification.
unnouinceput 7y ago
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 7y ago
  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 7y ago
    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 7y ago
      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.
  - ncmncm 7y ago
    It conformed to a Standard, just not the archaic one Linus uses.
    - microtherion 7y ago
      Ah yes, you're quite right, this is legal C99 and C11, but not legal C90. I'm more of a C++ programmer, and the C code I deal with tends to be ancient, so I had forgotten that this became legal at some point.
sdrothrock 7y ago
> 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."
- Dylan16807 7y ago
  > the approach employed in the LTS kernels seems to have been to naively silence the warning I wouldn't even give it that much credit. Putting "line that assigns to var" after "line that uses var" is worse than 'naive'.
- richardwhiuk 7y ago
  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.
  - sdrothrock 7y ago
    > 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.
pornel 7y ago
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 7y ago
  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 7y ago
    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 7y ago
atheowaway4z 7y ago
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 ?
- temac 7y ago
  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.
- wyldfire 7y ago
  The article describes grsecurity's 'respectre' compiler plugin [1]. [1] https://grsecurity.net/respectre_announce.php https://grsecurity.net/respectre_announce.php
  - mannykannot 7y ago
    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)."
Jach 7y ago
> 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...
mehrdadn 7y ago
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...
misc228 7y ago
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-perens-warns-grsecurity-breaches-the-linux-kernels-gpl-license https://linux.slashdot.org/story/17/07/09/188246/bruce-peren... http://perens.com/blog/2017/06/28/warning-grsecurity-potential-contributory-infringement-risk-for-customers/ http://perens.com/blog/2017/06/28/warning-grsecurity-potenti... 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).

171 points by pentestercrab 7y ago

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 7y ago
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 7y ago
  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

jandeboevrie 7y ago

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.

viraptor 7y ago
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.
ncmncm 7y ago
But they are correct. Correctness should forgive any amount of "tone", if indeed we care about correctness.
- unixhero 7y ago
  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.
  - ncmncm 7y ago
    In this case the author is right, so your guy's faults do not figure. Stern, opinionated, and wrong are obviously worse than just wrong.
- mannykannot 7y ago
  It is no good being correct if you cannot communicate it, and the most effective communication drops the tone and sticks to the facts. Having said that, I do not think, in this case, there is significant tone/attitude. Naming names was not necessary, however.
- rndgermandude 7y ago
  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".
  - pg_is_a_butt 7y ago
    oh, aren't you so smart and everyone else so uncontrollably rude. just monsters. not like you... you're way better. an ignorant hypocrite, perhaps, but not rude.
AdmiralAsshat 7y ago
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.
pferde 7y ago
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.
vardump 7y ago
Thank you grsecurity for the patch. They provided a valuable fix. If they consider some chest bumping helps them to get more business to support their future freebie bug hunts, I'd call it fair. Also, I think bragging rights motivate a lot of white hats doing valuable work for free. Would we be better off without them? That said, I'm not saying being impolite is ok.
presidentscroob 7y ago
grsec has a history of not playing well with others, especially when they took away the source and went $$$ payware only. Took away public stable patches: https://news.ycombinator.com/item?id=10128925 https://news.ycombinator.com/item?id=10128925 Took away public test patches: https://news.ycombinator.com/item?id=14205145 https://news.ycombinator.com/item?id=14205145 When they sued a blogger for defamation: https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecurity_sues_bruce_perens_for_defamation/ https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecu... Classic Coke Linus on grsec patches: https://www.spinics.net/lists/kernel/msg2540934.html https://www.spinics.net/lists/kernel/msg2540934.html Being a dick doesn't affect the quality and/or merits of code, it just makes some readers fall for their opinions more readily while turning off others.
deleted 7y ago
[deleted]

justinjlynn 7y ago

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-contributory-infringement-risk-for-customers/ https://perens.com/2017/06/28/warning-grsecurity-potential-c... [2] https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecurity_sues_bruce_perens_for_defamation/ https://www.theregister.co.uk/2017/08/03/linux_kernel_grsecu... [3] https://perens.com/wp-content/uploads/sites/4/2017/12/file0.43502131597246.pdf https://perens.com/wp-content/uploads/sites/4/2017/12/file0....

progval 7y ago
> 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.

archi42 7y ago

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.

phil9987 7y ago

So Linus fucked it up?!

unnouinceput 7y ago
did you read the article or only one phrase of it, the one about Linus? He did a correct fix but was ignored by the other developer.
- phil9987 7y ago
  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.
segfaultbuserr 7y ago
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 7y ago
  > 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 7y ago
    So one could argue that Linus kinda failed in fixing the problem he has identified correctly.
- phil9987 7y ago
  Makes sense, thanks for the clarification.

unnouinceput 7y ago

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 7y ago
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 7y ago
  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 7y ago
    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.
- ncmncm 7y ago
  It conformed to a Standard, just not the archaic one Linus uses.
  - microtherion 7y ago
    Ah yes, you're quite right, this is legal C99 and C11, but not legal C90. I'm more of a C++ programmer, and the C code I deal with tends to be ancient, so I had forgotten that this became legal at some point.

sdrothrock 7y ago

> 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."

Dylan16807 7y ago
> the approach employed in the LTS kernels seems to have been to naively silence the warning I wouldn't even give it that much credit. Putting "line that assigns to var" after "line that uses var" is worse than 'naive'.
richardwhiuk 7y ago
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.
- sdrothrock 7y ago
  > 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.

pornel 7y ago

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 7y ago
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 7y ago
  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 7y ago

atheowaway4z 7y ago

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 ?

temac 7y ago
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.
wyldfire 7y ago
The article describes grsecurity's 'respectre' compiler plugin [1]. [1] https://grsecurity.net/respectre_announce.php https://grsecurity.net/respectre_announce.php
- mannykannot 7y ago
  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)."

Jach 7y ago

> 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...

mehrdadn 7y ago

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...

misc228 7y ago

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-perens-warns-grsecurity-breaches-the-linux-kernels-gpl-license https://linux.slashdot.org/story/17/07/09/188246/bruce-peren... http://perens.com/blog/2017/06/28/warning-grsecurity-potential-contributory-infringement-risk-for-customers/ http://perens.com/blog/2017/06/28/warning-grsecurity-potenti... 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).