3 ms·

The root cause of the bug isn't the kernel dereferencing a NULL and causing UB. It's the kernel doing error handling and attempting to kill the oopsing task and

by chc4 4y ago

The root cause of the bug isn't the kernel dereferencing a NULL and causing UB. It's the kernel doing error handling and attempting to kill the oopsing task and continue. If the semantics of Rust panics also did a kernel oops, unwrap() would trigger the exact same bug described in the blog post with regards to reference count rollover (if Rust in the kernel doesn't do stack unwinding)

jeffbee 4y ago
Yes, but fundamentally the kernel's inability to handle exceptional cases is all in deference to performance. Non-performance-critical sections, which in a fair analysis would be 99.9% of the kernel at least, should be written in a language and style that provides structured unwinding, not just jumping to the error case and whoops I accidentally jumped beyond all the unlocks and reference count decrements and deallocations. That's the issue.
Dylan16807 4y ago
You wouldn't allow code that aborts without cleanup in these areas of the kernel, or in the kernel at all. You can't make a similar rule against null dereferences, because those happen by accident. (Unless you wrap every single pointer dereference, which is not happening.) If you don't allow aborting, then the compiler makes you write an error-handling path that returns, and the cleanup code will not be skipped.
deathanatos 4y ago
There are two bugs discussed in the article. One is the one this thread started with, which was the kernel deref'ing a NULL. You're right that this is separate from the handling of the oops, which is the main exploitability that TFA is getting at, and certainly fixing one deref leading to an oops (the proc file chasing NULL) doesn't fix the other bug of "any oops can be further exploited". But the context of this subthread is the implication that you must have some null, and some thing must happen when it is chased. That assumption is wrong, that's what the core of the comment I'm making is getting at: you can't follow a null if you don't have the possibility of them in the first place. (Or where you must have an Option<T>, you can build safe interfaces for handling that fact.) > unwrap() would trigger the exact same bug described in the blog post with regards to reference count rollover If we consider this instead as "an unwrap occurring during the oops handling", maybe, but it's not guaranteed that that is the case. Other aspects of Rust could similarly prevent that bug. I haven't fully grokked the latter half of the article, but I didn't think it would be necessary for the comment, as, a. the chain was about "Printing /proc/$pid/smaps is not on any conceivable performance-critical hot path." and b. followed by the question about null. Ref-counting in Rust is often dealt with via RAII, and is safe through that, both in that RAII means the refcount is managed correctly and without input from the coder, but also Rc (and I presume Arc) will abort on overflow. I don't know if that would fully translate to kernel code, given that we might be taking refs due to the actions of userland, and that might be happening near the userland/kernel boundary and be reasonably subject to unsafe code that could very well fall prey to the same problems.

by chc4 4y ago

jeffbee 4y ago

Yes, but fundamentally the kernel's inability to handle exceptional cases is all in deference to performance. Non-performance-critical sections, which in a fair analysis would be 99.9% of the kernel at least, should be written in a language and style that provides structured unwinding, not just jumping to the error case and whoops I accidentally jumped beyond all the unlocks and reference count decrements and deallocations. That's the issue.

Dylan16807 4y ago

You wouldn't allow code that aborts without cleanup in these areas of the kernel, or in the kernel at all. You can't make a similar rule against null dereferences, because those happen by accident. (Unless you wrap every single pointer dereference, which is not happening.) If you don't allow aborting, then the compiler makes you write an error-handling path that returns, and the cleanup code will not be skipped.

deathanatos 4y ago

There are two bugs discussed in the article. One is the one this thread started with, which was the kernel deref'ing a NULL. You're right that this is separate from the handling of the oops, which is the main exploitability that TFA is getting at, and certainly fixing one deref leading to an oops (the proc file chasing NULL) doesn't fix the other bug of "any oops can be further exploited". But the context of this subthread is the implication that you must have some null, and some thing must happen when it is chased. That assumption is wrong, that's what the core of the comment I'm making is getting at: you can't follow a null if you don't have the possibility of them in the first place. (Or where you must have an Option<T>, you can build safe interfaces for handling that fact.) > unwrap() would trigger the exact same bug described in the blog post with regards to reference count rollover If we consider this instead as "an unwrap occurring during the oops handling", maybe, but it's not guaranteed that that is the case. Other aspects of Rust could similarly prevent that bug. I haven't fully grokked the latter half of the article, but I didn't think it would be necessary for the comment, as, a. the chain was about "Printing /proc/$pid/smaps is not on any conceivable performance-critical hot path." and b. followed by the question about null. Ref-counting in Rust is often dealt with via RAII, and is safe through that, both in that RAII means the refcount is managed correctly and without input from the coder, but also Rc (and I presume Arc) will abort on overflow. I don't know if that would fully translate to kernel code, given that we might be taking refs due to the actions of userland, and that might be happening near the userland/kernel boundary and be reasonably subject to unsafe code that could very well fall prey to the same problems.