Reading on uninitialized memory may cause UB ( util::read_spv() )

[JOE1994]

Hello 🦀 ,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

https://github.com/MaikKlein/ash/blob/7fa182cc4330da8da98a7c023049162a4979d0bf/ash/src/util.rs#L116-L124
util::read_spv() method creates an uninitialized buffer and passes it to user-provided Read implementation. This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).

This part from the Read trait documentation explains the issue:

It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit<T>) is not safe, and can lead to undefined behavior.

Suggested Fix

It is safe to zero-initialize the newly allocated u32 buffer before read(), in order to prevent user-provided Read from accessing old contents of the newly allocated heap memory.

Also, there are two nightly features for handling such cases.

• https://doc.rust-lang.org/std/io/struct.Initializer.html
• https://rust-lang.github.io/rfcs/2930-read-buf.html

Thank you for checking out this issue 👍

[JOE1994]

Unfortunately the safe yet naive fix of zero-initializing the newly allocated buffer incurs runtime overhead 😢
As of Jan 2021, there doesn't seem to be an ideal fix that works in stable Rust with no performance overhead.
Below are links to relevant discussions & suggestions for the fix.

• Well-written document regarding the issue
• Rust RFC 2930
• nightly feature std::io::Initializer
• Discussion in Rust Internals Forum

[MarijnS95]

Yikes, it looks like rustsec/advisory-db#680 managed to get merged just yesterday (after sitting for half a year...).

I'm inclined to just remove this function altogether or ban it away to a separately-published util crate, it has nothing to do with Ash's immediate goal of wrapping Vulkan. For parsing SPIR-V one can always resort to rspirv.

[Ralith]

I'm inclined to just remove this function altogether or ban it away to a separately-published util crate, it has nothing to do with Ash's immediate goal of wrapping Vulkan. For parsing SPIR-V one can always resort to rspirv.

read_spv is for loading SPIR-V, not parsing it. We should keep it because ~every single Vulkan application needs to do this, and it's surprisingly tricky to get right (as illustrated by this issue).

[MarijnS95]

I've misread the function initially; we always use a compiler that emits SPIR-V in host-endianness (what vkCreateShaderModule expects) and have never had to use this function.

[Ralith]

Handling endianness isn't actually the main benefit, though it's nice. Vulkan requires that SPIR-V be passed in as a valid u32 pointer, which means that it must be properly aligned. The naive approach of reading into a Vec<u8> or employing include_bytes does not guarantee this, and is hence unsound.

we always use a compiler that emits SPIR-V in host-endianness

Note that what you actually want here is target endianness, though this only differs when cross compiling.

[MarijnS95]

Handling endianness isn't actually the main benefit, though it's nice. Vulkan requires that SPIR-V be passed in as a valid u32 pointer, which means that it must be properly aligned.

This is fairly trivial too, not "surprisingly tricky to get right" - this function could have been safe if it didn't juggle endianness around.

Note that what you actually want here is target endianness, though this only differs when cross compiling.

I was referring to the "host" that runs our Vulkan program, fair to call that a target. We compile the few shaders that we have on that target machine at runtime, so it's always the same on our end. Whatever, we're bikeshedding now 😬

[Ralith]

This is fairly trivial too, not "surprisingly tricky to get right"

The number of times I've had to explain to people that it's necessary at all (see e.g. #245) begs to differ, at least :P

this function could have been safe if it didn't juggle endianness around.

The endianness flip is already 100% safe. The unsafety is required to treat an array of u32s as an array of u8s for I/O purposes. This could be made safe instead at the cost of additional copies, which might discourage its use in favor of unsound pointer casting.

[MarijnS95]

Implicitly saying that, if we took the penalty for copies, we can still change endianness (in a 100% safe way) but we wouldn't be here discussing this in the first place. The flip itself isn't the problem, the way it is implemented is (or was).