| |||||||||||||
Stable kernel 2.6.27.5Stable kernel 2.6.27.5Posted Nov 8, 2008 21:37 UTC (Sat) by PaXTeam (subscriber, #24616)In reply to: Stable kernel 2.6.27.5 by jmayer Parent article: Stable kernel 2.6.27.5 do improperly fixed problems count? say, commit df211d2ac9df176d9b9b4b0984f9dcb50ece39fc. one wonders what review the rest got if this 2 liner was let through...
(Log in to post comments)
Stable kernel 2.6.27.5 Posted Nov 9, 2008 12:48 UTC (Sun) by mb (subscriber, #50428) [Link]
Can you explain to stupid people, like me, what the problem is with this fix?
Stable kernel 2.6.27.5 Posted Nov 9, 2008 14:20 UTC (Sun) by tialaramex (subscriber, #21167) [Link]
I would guess that PaXTeam has spotted that the i386 code is different to the x86-64 code here. The i386 code is easier to understand, because it essentially marks out boundaries such that any 32-bit access will be inside the kernel stack. On the other hand it's a bit more complicated because x86 calling conventions are a bit weird. In contrast the patched x86-64 code is less explicit, forbidding an access exactly at the edge (outside) of the stack but the convention it is implementing is more straight forward.
If the pointer was somehow not to be 64-bit aligned, then the patch for x86-64 leaves open the possibility of an unaligned pointer access overlapping the edge of the stack, which would be caught by the equivalent code from the i386 kernel. My understanding is that the x86-64 ABI forbids this in general, but perhaps PaXTeam knows better, at least in regard to the kernel stacks.
Stable kernel 2.6.27.5 Posted Nov 9, 2008 14:31 UTC (Sun) by tialaramex (subscriber, #21167) [Link]
Oh, and this trick...
ip = *(u64 *)(fp+8);
...would obviously back-fire if e.g. (fp = stack+THREAD_SIZE - 8) but it's not clear to me that this can happen either. Maybe I need to go read more of the kernel stack code.
Stable kernel 2.6.27.5 Posted Nov 9, 2008 16:27 UTC (Sun) by PaXTeam (subscriber, #24616) [Link]
no, i haven't even looked at the i386 code, it was obviously wrong just by reading the patch. and the problem has nothing to do with alignment, it's an untrusted pointer (that's why it's bounds checked in the first place, else it wouldn't have to be), so no alignment assumptions can be made.
Stable kernel 2.6.27.5 Posted Nov 9, 2008 18:02 UTC (Sun) by tialaramex (subscriber, #21167) [Link]
It is usually helpful to look a bit further than the patch.
If you're right that there's nothing outside this function to protect the state it's inspecting, (ie the target process might start running) since that's the only way for this value to become "untrusted" then the i386 code is exactly the right place to look, since that takes the proper care of ordering e.g.
sp = p->thread.sp;
We have to stash the stack pointer because otherwise the other process can race us, changing it just after we confirm that it's inside bounds and before we dereference it. Similarly with results read from the stack, although the x86-64 code gets the latter right already.
But really I don't have enough information to make a good judgement about how much paranoia is justified. What locks is a caller of get_wchan expected to hold? Can this process structure go away from underneath us while we're executing? What about the associated stack?
Stable kernel 2.6.27.5 Posted Nov 9, 2008 19:30 UTC (Sun) by PaXTeam (subscriber, #24616) [Link]
i actually never said that the task's stack couldn't change while this code is running. in fact, quite the opposite is true, hence all the various checks in the code.
Stable kernel 2.6.27.5 Posted Nov 9, 2008 16:23 UTC (Sun) by PaXTeam (subscriber, #24616) [Link]
i don't think you're stupid just because you don't understand the problem. it's very simple actually: it's about bounds checking (of untrusted pointers), that is, before accessing a piece of memory, we want to ensure that the first byte of the access isn't below the allowed range and the last byte of the access isn't above the allowed range. these two checks ensure that the access stays within bounds (whether the accessed data is good/bad is another question, that's what the rest of the code deals with).
now the code in question got the first check right but not the second one and the 'fix' didn't really improve it either. you can see it from the code that the accessed data is 8 bytes long (sizeof u64) however the upper bound for both lines is wrong and changing > to >= doesn't help it much when the upper bound itself is off by 8/16 bytes already.
then there's a smaller issue of two useless unsigned long casts that anyone who actually looked at the whole code should have spotted.
Patch? Posted Nov 9, 2008 16:34 UTC (Sun) by corbet (editor, #1) [Link] Now that you've looked at the code and spotted this problem, have you submitted a patch to fix it?
Patch? Posted Nov 9, 2008 16:51 UTC (Sun) by PaXTeam (subscriber, #24616) [Link]
it's in PaX otherwise i can't submit it due to the DCO excluding anonymous contributors.
Patch? Posted Nov 9, 2008 20:49 UTC (Sun) by arekm (subscriber, #4846) [Link]
You CAN submit it without Signed-off line. Others can ignore it of course but patch will be there for anyone.
Stable kernel 2.6.27.5 Posted Nov 9, 2008 17:11 UTC (Sun) by mb (subscriber, #50428) [Link]
> code that the accessed data is 8 bytes long (sizeof u64)
Cool, thanks. I didn't notice that by reading the patch only.
> then there's a smaller issue of two useless unsigned long casts that anyone who actually looked at the whole code should have spotted.
Yeah, these casts look rather fishy to me, too.
Stable kernel 2.6.27.5 Posted Nov 9, 2008 17:37 UTC (Sun) by PaXTeam (subscriber, #24616) [Link]
well, the pointer dereferences along with the proper type casts are right in the next line after the bounds checks (prudent reviewers would check the entire code to account for all dereferences, however).
|
|||||||||||||