Hacker News Reader

Comment by nousermane | Hacker News Reader

I'm not a fan of rust either, not by a long mile. But current kernel approach to memory safety is a complete, utter, demonstrated failure. Look at last week's CVE-2022-41674:

https://seclists.org/oss-sec/2022/q4/23

This is a catastrophic bug, that (after some work on developing an actual RCE) lets anybody within wifi range to get root on your laptop (or phone, or access point). And all it took is this one line:

https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wir...

We've all been repeating the "1000 eyes - all bugs are shallow" mantra for far too long. This one was in the mainline for more than 3 years, and nobody noticed. How many more are lurking there?

View on HN

Separately I feel "with enough eyes, all bugs are shallow" fails to apply in codebases with many users but most of whom consume it as a black box without looking inside. Some issues making it harder for me to read the code I use:

- The Linux kernel and issuance .so files lack a "view source" button on compiled binaries. And even checking out the matching source, building a replacement binary, and diffing your local changes from the matching source is an arduous progress to setup per program/library from a tarball/Git tag, wait for the computer to finish, install dependency .so files globally, ensure symbols are present, ensure you can breakpoint static functions...

- Dynamic dispatch and generic code might help maintainers and code extensibility but (in my experience) definitely impede external eyeballs from understanding code.

Yes! That's why I adore the way plan 9 has the whole source tree in /sys/src. Everything is there.

To a smaller extent, netbsd has pkgsrc, and most BSD friends have something like that too.

s/issuance/userspace, s/computer/compiler

Does Rust help solve overflow errors? Don't you need to explicitly used "saturated_add" or "checked_add"?

Rust doesn't directly solve overflow errors. What Rust does do is turn them into logic errors, rather than memory issues. To break it down:

* Overflow with the default operators is not UB in Rust, it will either panic or two's compliment wrap, depending on various things (including things you can set to choose this global behavior). This already prevents various issues.

* You can also explicitly choose to do various operations with whatever overflow semantics you want, as you mention with saturated_add and friends.

* Because indexing is bounds checked, where in some languages an overflowed integer would lead to incorrect indexing and therefore possible memory problems, you'll either get a panic or a logic bug, not a memory bug.

* If your integer isn't being used for indexing, you'll end up with some sort of error, but again, at worst a logical error, not a memory error.

is this really that bad of a bug though? i need to be using that driver, and someone near me has to be actively knowing and trying to inject... and it's a DoS ? i'm sincerely asking btw

Driver in question is cfg80211. Majority of popular wireless cards use it.

DoS is the only publicly known exploit, right now. There is understanding that RCE is possible with additional specialist work.

Local packet injection is an implementation detail of the first public PoC. There are other implementations of the same exploit, that deliver packets over the air instead. Including one that runs on ESP32, and attacks unmodified Linux nearby:

https://github.com/jo-m/linux-wifi-ota-crash

> and someone near me has to be actively knowing and trying to inject

Just leave a small board, a battery, and a solar panel on a roof or tree near your office. If RCE is possible with this (people think it is), this would be very valuable.

On a tree near my office, you'd gain access to our network, Microsoft's and a European central bank. Not bad at all.

Confused by that bug. Adding 2 to a U8 causing overflow makes it smaller, not larger. It will copy too little? not too much.

How would Rust fix this? Here's a Rust program that overflows a u8, with no compile error or warning. https://godbolt.org/z/ME3e9KeMe

You can turn on runtime overflow checking with the Rust compiler, but you can do that with gcc compiling C too.

I guess the answer is that you wouldn't do the memcpy like that in idiomatic Rust - you'd use some higher level construct that gives the compiler more chance to catch your errors. Could anyone comment on how this works in a case like this?

edit: Can't reply to repliers. Defined overflow doesn't help here. A checked_add() function could be used in the Linux kernel in C, just as easily as in Rust. Forcing checked_add() to be used in the Rust compiler would help, but would also have performance and readability impact, which is presumably why it is still being debated.

> How would Rust fix this? [...] I guess the answer is that you wouldn't do the memcpy like that in idiomatic Rust - you'd use some higher level construct that gives the compiler more chance to catch your errors.

Yes, that's the trick: it will be caught not in the u8 overflow, but either in the memcpy equivalent (which is "dst.copy_from_slice(src)"), or in the slice manipulation before it. What happens is that slices in Rust are represented by a "fat pointer", a pair of the starting address and the length, and both the "copy_from_slice" method and the index/index_mut operator check the bounds before doing the operation.

(You could do things using "unsafe" and raw pointers, which have only the starting address without the length, but in idiomatic Rust you'd use slices most of the time.)

In C when you use an inappropriate type (such as u8 here for a size) it's just coerced. In Rust the wrong type doesn't compile. So, I think the programmer is less likely to choose u8 in the first place.

I thought the idea was that Rust’s behavior in cases of overflow was defined, whereas c/c++ is not defined by the language.

The C language says the unsigned integers wrap. That is, C's 8-bit unsigned integer (e.g. uint8_t or in Linux source u8) behaves the same as Rust's Wrapping<u8> type.

In Rust the u8 type wraps in default release builds, but panics on overflow in debug builds, but this is kernel code so it will definitely be built in release mode with wrapping.

That is because you're using a normal add instead of `checked_add`. There are discussions about forcing the compiler to only allow `checked_x` and `wrapping_x` operations for any mathematical operation within your code.

"The subsequent memcpy" is this:

  memcpy(pos, mbssid + cpy_len, ((ie + ielen) - (mbssid + cpy_len)));

> (after some work on developing an actual RCE)

While I agree that bug is serious, that "some" is doing pretty heavy lifting here. Is there an RCE for this bug?

There are almost certainly some private ones. This is an extremely high impact bug.

Reply on news.ycombinator.com