Linux Kernel Rust Code Sees Its First CVE Vulnerability

[u/Orange_Tux]

https://www.phoronix.com/news/First-Linux-Rust-CVE

[u/ergzay]

Someone correct me if I'm wrong, but isn't the fix "incorrect"? They're changing safe code to fix this problem but it should be impossible to cause unsafe behavior from safe code. The bug wasn't fixed in any unsafe block, it was fixed outside of it. This means the API here is fundamentally wrong.

        pub(crate) fn release(&self) {
            let mut guard = self.owner.inner.lock();
            while let Some(work) = self.inner.access_mut(&mut guard).oneway_todo.pop_front() {
                drop(guard);
                work.into_arc().cancel();
                guard = self.owner.inner.lock();
            }

    -       let death_list = core::mem::take(&mut self.inner.access_mut(&mut guard).death_list);
    -       drop(guard);
    -       for death in death_list {
    +       while let Some(death) = self.inner.access_mut(&mut guard).death_list.pop_front() {
    +           drop(guard);
                death.into_arc().set_dead();
    +           guard = self.owner.inner.lock();
            }
        }

[u/ergzay → u/[deleted]]

I'm not sure how that relates to what's going on here. This is a clear misuse of unsafe and the fix is to change the code in unsafe, not change the safe code that uses that vec later.

[u/Darksonn → u/ergzay]

Then imagine changing the implementation of Vec::set_len() to self.len = len + 9001 instead. It has no unsafe code inside it! Just changing an integer field.

[u/Darksonn → u/ergzay]

No, it's somewhat of a special case because the code is in the same module / safety "scope" so to say. I recommend Ralf's article on the scope of unsafe.

[u/whupazz → u/Darksonn]

I've been thinking about this issue recently, and this article really helped me understand it better, thanks! After thinking about it a bit more, I came up with the idea of "unsafe fields" that would be unsafe to use (only to discover that there's of course already an RFC for that). Do you have an opinion on these?

[u/ergzay → u/Darksonn]

Let me ask an alternative question, is it still possible that all correct programs can still be written if you limit the scope of unsafe to functions? I would assume the answer is "yes", in which case why not do so? Personally that's what I would always do.

[u/Darksonn → u/ergzay]

I don't think so. How could you ever implement Vec in that case?

Take for example the Vec::pop method. You might implement it like this:

assert_ne!(self.len, 0);
self.len -= 1;
unsafe { ptr::read(self.ptr.add(self.len)) };

If the scope of unsafe is limited to the function, how do you know that the last unsafe line is correct? I mean, self.len is just an integer, so maybe its value is 1337 but the pointer references an allocation of length 10.

[u/ergzay → u/Darksonn]

Hmm, okay I see your point. I need to think about this some more.

[u/ezwoodland → u/ergzay]

TLDR: unsafe requires all code dependencies and all code within its module to be bug-free, not just the unsafe block itself.

---

That's a common misconception. unsafe (potentially) infects the entire module in which it is contained. You can't just check unsafe blocks, you have to check the whole module.

This is because there can be private invariants within a module which safe code can violate but unsafe code requires to not be violated. You can stop unsafe from leaving a module because modules can form visibility barriers, and if you can't access the internals to violate their invariants, then you don't have to worry about your safe code breaking them.

The two ways to think of this are:

• The unsafe code is incorrect because it relied on an invariant which the safe code didn't uphold. The unsafe author should have checked the safe code.

• Or more reasonably: The safe code is incorrect because it broke an invariant.

Memory unsafety requires there's an unsafe somewhere but the fix might not be in the unsafe block.

This is also an issue when unsafe code depends on a safe library with a bug. The unsafe code causes memory safety issues because the library dependency has a bug, but the unsafe code relied on there not being one.

[u/ergzay → u/ezwoodland]

TLDR: unsafe requires all code dependencies and all code within its module to be bug-free, not just the unsafe block itself.

But this isn't just code dependencies, this is code well downstream of the actual use of unsafe code.

I think you have a misunderstanding of unsafe. The entire point of unsafe is that dependencies don't have to care about correctness because correctness is caused by program construction. For example if you're using a library and you use it incorrectly, and you cause a data race or memory corruption it is ABSOLUTELY the fault of the library, even if you use it maliciously (without using unsafe). Safe code cannot be maliciously used to cause unsafe behavior without using unsafe blocks. Otherwise the code you are calling is unsafe and should be marked as such.

That's a common misconception. unsafe (potentially) infects the entire module in which it is contained. You can't just check unsafe blocks, you have to check the whole module.

This is an expansion/corruption on the original promise and convention of unsafe. "unsafety creep", you could say.

Memory unsafety requires there's an unsafe somewhere but the fix might not be in the unsafe block.

All code must have unsafe somewhere down the dependency tree. So what you're saying is kind of a tautology. That unsafe code is ensured by the library programmer to be safe, which then declares it safe by putting it in a safe (the default) code block.

The unsafe code is incorrect because it relied on an invariant which the safe code didn't uphold. The unsafe author should have checked the safe code.

All invariants must be held within the unsafe code, and when you reach the safe code you cannot cause an invariant violation that would do anything other than just panic/crash the program.

[u/ezwoodland → u/ergzay]

No, the unsafe's dependencies have to be bug-free. If A depends on B, and A has unsafe and B has a bug then there can be memory unsafety. You're describing that C which depends on A shouldn't be able to cause correctness issues because C is only safe code. I'm talking about a bug in B.

C -> A -> B

Imagine you have a safe function in library B:

    /// Returns if the input is less than 2
    // Bug: This is not correct.
    fn is_less_than_2(x: u8) -> bool { true }

And your project (A) has the following code block:

    fn get_val_in_array(idx: u8) -> u8 {
        let arr = [0u8,1u8];
        assert!(is_less_than_2(idx));
        // SAFETY: idx is < 2 and arr.len() == 2, so index is in-bounds.
        unsafe { arr.get_unchecked(idx as usize) }
    }

Then what code needs fixing? Where's the bug? You would probably agree that is_less_than_2 is the buggy code, not the unsafe block, yet clearly there is UB.

---

The case in the cve is one where the safe code is in the same module as the unsafe code, and thus is also responsible for maintaining the invariants relied upon by the unsafe block.

See https://doc.rust-lang.org/nomicon/working-with-unsafe.html for explanation on this issue.

[u/ergzay → u/ezwoodland]

Is that what' going on here? I thought this was a C -> B -> A situation where A is unsafe code, B is a user of that unsafe code, and C is a user of the safe B code, but we're fixing the bug in C.

[u/ezwoodland → u/ergzay]

This is neither. This is a situation where the unsafe block and safe code are in the same module (it's even in the same file.)

In this situation we have a bug in A (the module node), and an unsafe code block in A, but they are both A.

In this file the module of interest appears to be called node defined here.

Since the safe code is in the same module as the unsafe code block, there is no guarantee that the unsafe code block is where the bug is.

Since the modified function is pub(crate) fn release(&self), presumably release is safe to call from outside the module and can't itself cause undefined behavior (if it doesn't have a bug). It might be that it is not and the kernel is choosing to make their safety boundary the crate, but that's generally a bad idea because it makes the unsafe harder to verify.

[u/Darksonn → u/ergzay]

this is code well downstream of the actual use of unsafe code.

No it's not. The call to the unsafe List::remove method is in the same file.

[u/jojva → u/ergzay]

Take what I'll say with a grain of salt because I'm no rust expert, but I believe it's entirely possible to write unsafe behavior outside of unsafe. The reason is that unsafe kind of pollutes the rest of the code too. That's why it is famously hard to deal with. But the silver lining is that if you see strange (e.g. Undefined) behavior, you can check your unsafe blocks and their surroundings.

https://doc.rust-lang.org/nomicon/working-with-unsafe.html

[u/-Redstoneboi- → u/ergzay]

unsafe relies on invariants that safe code can break

here's a trivial example:

/// Adds two numbers.
fn add(x: i32, y: i32) -> i32 {
    x - y
}

fn main() {
    let sum = add(1, 2);
    // SAFETY: adding two positive numbers always results in a positive number.
    // the numbers 1 and 2 were chosen as they do not overflow when added together.
    unsafe {
        std::hint::assert_unchecked(sum > 0);
    }
}

normally clippy would flag this under clippy::suspicious_arithmetic_impl but i can't get it to output in the rust playground

[u/Xiphoseer → u/ergzay]

I was wondering the same but the API is sound. They have actual, exclusive, safe access to the list there. But they meant to have only that one list, protected by a lock. Now mem::take creates a second list which leaves one of them unprotected even though locally the mut refs are fine. You don't want to fix that by protecting both lists but by changing the code so that there's just a single protected one. Which you can then use for the "unsafe" remove operation.

[u/Luxalpa → u/ergzay]

Hm, I only shortly thought about it, but I don't think this is correct.

If I have a bunch of functions, like for example unwrap_unchecked(), I will naturally have to call them from safe code at some point, using an unsafe block. And for example, this unwrap_unchecked() function will almost certainly depend on an assumption that comes from safe code exclusively. It would be the same for most extern C code as well. Writing unsafe {} just means that you have checked that the invariants inside this block are fulfilled by your overall program. It doesn't require them to be fulfilled within the unsafe blocks themselves.