![](/i/fill.gif) |
![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
Darren New <dne### [at] san rr com> wrote:
> The exploit was a bug in the kernel that dereferenced a pointer before
> checking for null, and the compiler silently optimized out the later check
> for null. If you can get the first dereference to work (by mapping some
> valid memory to the address associated with the null pointer value) then you
> skip over code people thought they wrote into their program and which the
> compiler removed.
I'd rather say it was a flaw in the kernel, forgetting to already check for null
*before* dereferencing.
In such a "mission-critical" piece of software such as a kernel, *all* that
comes in from untrusted sources should *always* be checked thoroughly before
doing *anything* with it. "Oh, if this pointer happens to be NULL, the software
will core dump anyway" is a *very* questionable mode of operation.
Code dereferencing a possibly-NULL pointer *is* bogus - it's nothing you can
simply blame on the compiler. And mission-critical code should not *never* be
bogus.
There's even some reason to argue that if the programmer happily dereferences a
pointer without checking it for NULL, why shouldn't the compiler assume that
other provisions have been made that it cannot be NULL in the first place?
I'm also surprised that this flaw wasn't found and fixed earlier. There's tools
like PC-lint (taken as an example here because I'm a bit familiar with it) that
do a pretty good job at complaining when you're trying to dereference a
potentially-NULL pointer. I mean, you'd expect that *someone* runs *something*
like PC-lint on code as widely-used as a Linux kernel?!
Well, maybe that's a problem with free-software projects: PC-lint costs money.
Companies developing commercial software can afford to buy a few licenses. The
individual parties in a free-software project possibly can't, or refuse to pay
anything for their tools, or refuse to be the only ones paying for something
that benefits all.
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
clipka <nomail@nomail> wrote:
> Code dereferencing a possibly-NULL pointer *is* bogus - it's nothing you can
> simply blame on the compiler. And mission-critical code should not *never* be
> bogus.
I didn't get the impression that they were dereferencing a null pointer
deliberately in order to get an exception, or that it was a mistake. (The
only "mistake" was not realizing what kind of optimization gcc would do.)
--
- Warp
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
clipka wrote:
> I'd rather say it was a flaw in the kernel, forgetting to already check for null
> *before* dereferencing.
Yes, exactly. But I'm trying to figure out why you would add optimizations
specifically to improve the performance of code you are optimizing *because*
you know it is flawed.
Note that this isn't just a general dead-code-elimination. This is
specifically checking for "test for null after already dereferencing", as
there's an explicit compiler chip to turn that off.
> In such a "mission-critical" piece of software such as a kernel, *all* that
> comes in from untrusted sources should *always* be checked thoroughly before
> doing *anything* with it.
Right. I think the comments on the report of it that I read implied that
the assignment was added later, and the problem is that in C, the
declarations all had to be at the top.
So someone wrote
blah * xyz;
if (!xyz) return failure;
... use xyz ...
Someone else came along and added a declaration
blah * xyz;
another pdq = xyz->something;
if (!xyz) ....
Instead, they should have said
blah * xyz;
another pdq;
if (!xyz) ...
pdq = xyz->something;
> "Oh, if this pointer happens to be NULL, the software
> will core dump anyway" is a *very* questionable mode of operation.
Exactly.
> Code dereferencing a possibly-NULL pointer *is* bogus - it's nothing you can
> simply blame on the compiler. And mission-critical code should not *never* be
> bogus.
Agreed. The problem is that the compiler optimized out good code based on
bogus code. My only question is whether I'm missing something here, because
such an optimization seems really a bad idea.
> There's even some reason to argue that if the programmer happily dereferences a
> pointer without checking it for NULL, why shouldn't the compiler assume that
> other provisions have been made that it cannot be NULL in the first place?
That's exactly what the compiler did. It looked, saw the dereference of the
pointer, then the check that the pointer isn't null, and optimized out the
check for the pointer being null. But the fact that the programmer said to
check for NULL at a point where all code paths have already dereferenced the
pointer would seem to be at least a warning, not an "oh good, here's an
optimization I can apply."
The only reason I can think of for that would be code generated from some
other language, at which point I'd think you'd want a compiler flag to turn
*on* that optimization, with it being off by default.
> I'm also surprised that this flaw wasn't found and fixed earlier.
I'm surprised and dismayed when the kernel gets code checked in that's
syntactically invalid, then released, then when people complain about it
they say "Oh, I didn't know how that code builds" and someone else says "try
changing the ownership of the source file to be root", and then a few weeks
later they have a patch on some website somewhere unassociated with the
actual original project that makes it compile.
I'm beyond being surprised at how bad the free stuff can be if you're
actually a programmer. </rant>
--
Darren New, San Diego CA, USA (PST)
"We'd like you to back-port all the changes in 2.0
back to version 1.0."
"We've done that already. We call it 2.0."
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
Slime wrote:
> Of course this is a contrived example, but it's probably not too far from a
> real use case.
Ah, that makes sense. Thank you for clarifying that.
It *is* code generation. Just from the same language you're compiling. That
didn't occur to me. :-)
--
Darren New, San Diego CA, USA (PST)
"We'd like you to back-port all the changes in 2.0
back to version 1.0."
"We've done that already. We call it 2.0."
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
Darren New wrote:
> Doctor John wrote:
>> At a guess that would be:
>> http://www.theregister.co.uk/2009/07/17/linux_kernel_exploit/
>
> Yes, but since that wasn't the point of my question and people might
> reasonably assume I'm bashing on Linux if I posted a link to it, I
> thought I'd omit it. The question was why the compiler felt the need
> for *that* optimization in the first place. When would it ever be a good
> idea?
>
Actually, I was going to ask the group the same question but you just
beat me to it :-) and you could hardly assume that I was in any way
anti-Linux.
What bothers me (apart from the unwanted optimisation) is why Torvalds
et al have chosen to remain silent on this.
Another thing to remember is that it is not only the kernel code that is
the problem here, it's also gcc - so comments from the Stallman camp
would also be appropriate.
John
--
"Eppur si muove" - Galileo Galilei
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
Darren New <dne### [at] san rr com> wrote:
> But I'm trying to figure out why you would add optimizations
> specifically to improve the performance of code you are optimizing *because*
> you know it is flawed.
No, optimizers don't optimize code *because* it's flawed, just maybe *although*
it's flawed.
Or, possibly more particularly fitting the intention in this case: Although it
is *redundant*.
After all, that's a crucual part of optimization. It allows programmers to write
source code that is more self-descriptive and more self-defensive, yet at the
speed of hand-optimized code.
For instance, one might write:
if (i <= LOWER_BOUND) {
return ERR_TOO_LOW;
}
// very long code here
if (i > LOWER_BOUND && i <= UPPER_BOUND) {
// (1)
return OK;
}
The test for i>LOWER_BOUND is, of course, perfectly redundant. But explicitly
stating it may help...
(a) to make it easier to see for the casual reader that at (1), i>LOWER_BOUND is
always true; after all, they may not have read the part before the very long
code, or may have forgotten about the i<=LOWER_BOUND test by then; and
(b) to make sure that at (1), i>LOWER_BOUND is true even when someone tampers
with the remainder of the code and happens to remove the first check.
> Right. I think the comments on the report of it that I read implied that
> the assignment was added later, and the problem is that in C, the
> declarations all had to be at the top.
>
> So someone wrote
> blah * xyz;
> if (!xyz) return failure;
> ... use xyz ...
>
> Someone else came along and added a declaration
> blah * xyz;
> another pdq = xyz->something;
> if (!xyz) ....
That may indeed explain how the code came to be there in the first place.
> Instead, they should have said
> blah * xyz;
> another pdq;
> if (!xyz) ...
> pdq = xyz->something;
Yesss. Absolutely.
I must confess that I used to do it otherwise, too. It was PC-lint that taught
me not to, by constantly pestering me about it. And developing embedded
software for the automotive industry, I had to obey it: MISRA rules forbid to
use *any* language constructs that officially lead to unspecified behavior.
> Agreed. The problem is that the compiler optimized out good code based on
> bogus code. My only question is whether I'm missing something here, because
> such an optimization seems really a bad idea.
Optimizers aren't designed to detect bogus code - they're designed to speed up
things.
Even most compilers do a pretty poor job at detecting bogus code.
That's why you need static code-analysis tools, specifically designed to
identify such bogosities.
> > There's even some reason to argue that if the programmer happily dereferences a
> > pointer without checking it for NULL, why shouldn't the compiler assume that
> > other provisions have been made that it cannot be NULL in the first place?
>
> That's exactly what the compiler did. It looked, saw the dereference of the
> pointer, then the check that the pointer isn't null, and optimized out the
> check for the pointer being null. But the fact that the programmer said to
> check for NULL at a point where all code paths have already dereferenced the
> pointer would seem to be at least a warning, not an "oh good, here's an
> optimization I can apply."
As I already pointed out above, it may also have been code that the developer
left in there just for clarity, to be removed later, or whatever, *expecting*
the compiler to optimize it away.
> I'm surprised and dismayed when the kernel gets code checked in that's
> syntactically invalid,
At this point, in a good commercial project the developer would already get his
head chopped off - by colleagues wo *do* their homework and therefore hit this
stumbling block when trying to compile their own changes to do their own module
testing (and as good developers had suspected themselves to have done something
wrong first, and spent hours to track down the problem, until ultimately
identifying their colleague as the culprit and being *not* amused).
Checking in code you never actually compiled yourself? Hey, haven't done our
homework, have we?!?
> then released,
.... and at this point it would be the build manager's head to roll if it's a
single-platform project or the particular code applies to all projects - or the
test team leader's head if it's intended to be a portable thing and the code is
activated only on certain target platforms (unless of course the code is a fix
for an exotic platform that isn't available in-house).
In free-software projects with all contributors being volunteers, unfortunately
there's no authority to chop off some heads. After all, if you treat the
volunteers too harshly, off they go to someplace more relaxed.
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
Doctor John <joh### [at] home com> wrote:
> What bothers me (apart from the unwanted optimisation) is why Torvalds
> et al have chosen to remain silent on this.
What? And lose ground on their "safest operating system on earth" territory?
I did believe that, too, but hearing these news and thinking about the
background and how the "top hats" went to deal with the issue, I guess it's
time to start thinking out of the box once again: The Linux developer
community, too, want to "sell" their products in a way. Both their kernel, and
their FSF ideals.
Now, after always claming that free software is the superior development
approach, they prove that some of its drawbacks are potentially more serious
than they would like them to be.
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
clipka <nomail@nomail> wrote:
> [-- text/plain, encoding 8bit, charset: iso-8859-1, 18 lines --]
> Doctor John <joh### [at] home com> wrote:
> > What bothers me (apart from the unwanted optimisation) is why Torvalds
> > et al have chosen to remain silent on this.
> What? And lose ground on their "safest operating system on earth" territory?
> I did believe that, too, but hearing these news and thinking about the
> background and how the "top hats" went to deal with the issue, I guess it's
> time to start thinking out of the box once again: The Linux developer
> community, too, want to "sell" their products in a way. Both their kernel, and
> their FSF ideals.
> Now, after always claming that free software is the superior development
> approach, they prove that some of its drawbacks are potentially more serious
> than they would like them to be.
Aren't you exaggerating a bit?
Do you know how many serious security holes are found each year in Linux?
I think the number is in the *hundreds*. A few of these bugs each year provide
a way to get root privileges by running a program exploiting the bug. It has
happened dozens of times in the past, and it will happen in the future. This
is just another one of those.
(And before anyone says anything, no, Windows is not better. Windows is
year after year always at the top of the list of most security flaws found
during the year.)
You *can't* expect the OS to be completely safe.
--
- Warp
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
Warp wrote:
> Do you know how many serious security holes are found each year in Linux?
And I don't think this one is even in a kernel that was released into major
distributions, for that matter. :-)
> (And before anyone says anything, no, Windows is not better. Windows is
> year after year always at the top of the list of most security flaws found
> during the year.)
Is this still true? I think there's more 3rd-party flakey code, but not
stuff that comes with Windows necessarily. Certainly not as much as it used
to be.
--
Darren New, San Diego CA, USA (PST)
"We'd like you to back-port all the changes in 2.0
back to version 1.0."
"We've done that already. We call it 2.0."
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
clipka wrote:
> Optimizers aren't designed to detect bogus code - they're designed to speed up
> things.
Sure, but it sounded like an optimization that only worked on bogus code,
which seemed like a waste of time of the person writing the optimizer.
Macros make a good explanation why, tho.
> As I already pointed out above, it may also have been code that the developer
> left in there just for clarity, to be removed later, or whatever, *expecting*
> the compiler to optimize it away.
I think this is different. Your cases, sure.
> At this point, in a good commercial project the developer would already get his
> head chopped off
Well, to be fair, they all know it's bogus. It's been at 0.9.x for like five
years. :-) I'm always amused at open source folks who won't recognise that
the first thing they give to the public is 1.0 regardless of how you number it.
> Checking in code you never actually compiled yourself? Hey, haven't done our
> homework, have we?!?
Crap like this would be completely untenable before Google, really.
> test team leader's head if it's intended to be a portable thing and the code is
> activated only on certain target platforms (unless of course the code is a fix
> for an exotic platform that isn't available in-house).
This happened to be some mips-specific assembly. Not exactly exotic, but
then why are you changing that file if you don't have a mips chip to test it
on in the first place?
--
Darren New, San Diego CA, USA (PST)
"We'd like you to back-port all the changes in 2.0
back to version 1.0."
"We've done that already. We call it 2.0."
Post a reply to this message
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |