Unsound usages of unsafe function `slice::from_raw_parts`

[shinmao]

trafficstars

Hi, we are researchers from Sunlab. When we tried to scan Rust-based repositories with our own implemented bug detectors, we found that there are some potentially unsound usages of slice::from_raw_parts in the package candle-core at version 0.4.1.

quantized::ggml_file::from_raw_data

https://github.com/huggingface/candle/blob/a4d5a414e3ae79642ecfd6b7bb410c26a8a62a06/candle-core/src/quantized/ggml_file.rs#L120-L134 At line 128, the reference to u8 slice was casted to raw pointers of generic types that implemented traits such as GgmlType e.g., BlockQ4_0 or f32. However, the pointer type conversion in such way would break the alignment requirement of slice::from_raw_parts, and it might lead to unsound behaviors.

Even though this API is private, but it is accessible from other public API such as qtensor_from_ggml.

PoC

use candle_core::quantized::{ggml_file::qtensor_from_ggml, GgmlDType};
use candle_core::Device;

fn main() {
    let raw_data: [u8; 1] = [0x1; 1];
    let dim: Vec<usize> = Vec::from([1usize, 2, 3, 4]);
    let res = qtensor_from_ggml(
        GgmlDType::F32,
        &raw_data,
        dim,
        &Device::Cpu
    );
    println!("{:?}", res.unwrap().data());
}

Run with miri and get the result,

error: Undefined Behavior: dereferencing pointer failed: alloc828 has size 1, so pointer to 96 bytes starting at offset 0 is out-of-bounds
   --> /home/rafael/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:101:9
    |
101 |         &*ptr::slice_from_raw_parts(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ dereferencing pointer failed: alloc828 has size 1, so pointer to 96 bytes starting at offset 0 is out-of-bounds
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc828 was allocated here:
   --> src/main.rs:5:9
    |
5   |     let raw_data: [u8; 1] = [0x1; 1];
    |         ^^^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `std::slice::from_raw_parts::<'_, f32>` at /home/rafael/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:101:9: 101:47
    = note: inside `candle_core::quantized::ggml_file::from_raw_data::<f32>` at /home/rafael/.cargo/registry/src/index.crates.io-6f17d22bba15001f/candle-core-0.4.1/src/quantized/ggml_file.rs:128:25: 128:87
    = note: inside `candle_core::quantized::ggml_file::qtensor_from_ggml` at /home/rafael/.cargo/registry/src/index.crates.io-6f17d22bba15001f/candle-core-0.4.1/src/quantized/ggml_file.rs:154:27: 154:86
note: inside `main`
   --> src/main.rs:7:15
    |
7   |       let res = qtensor_from_ggml(
    |  _______________^
8   | |         GgmlDType::F32,
9   | |         &raw_data,
10  | |         dim,
11  | |         &Device::Cpu
12  | |     );
    | |_____^

In addition to OOB, the potential consequence of this undefined behavior is inconsistency in different environment. For example, if we run this program with x86 targets:

Ok([1, 192, 51, 207, 86, 4, 0, 0, 0, 4, 0, 0, 0, 194, 48, 31, 10, 1, 0, 0, 0, 2, 0, 0, 0, 3, 0, 0, 0, 4, 0, 0, 0, 3, 0, 0, 0, 32, 1, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 49, 0, 0, 0, 7, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0, 0, 20, 48, 207, 86, 91, 0, 0, 0, 192, 51, 207, 86, 4, 0, 0, 0, 4, 0, 0, 0, 124, 255, 255])

However, if we run this program with x86_64 targets:

Ok([1, 32, 185, 148, 192, 200, 85, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0])

This might be one impact when users rely on the consistency of API.

Some other usages of slice::from_raw_parts are also unsound:

• safetensors::convert_slice: https://github.com/huggingface/candle/blob/a0460cd2b13a396ff8545dc1bbffa741f2ec3d79/candle-core/src/safetensors.rs#L94
• safetensors::convert_slice_with_cast: https://github.com/huggingface/candle/blob/a0460cd2b13a396ff8545dc1bbffa741f2ec3d79/candle-core/src/safetensors.rs#L124

Would love to have more discussion about consequence and how to fix this issue:)

[tomsanbear]

Thanks for the find! I was able to reproduce your example, thanks for raising this, do you mind taking a look at the linked draft PR and providing some feedback @shinmao , admittedly I'm still fairly new to rust, and especially unsafe mechanics so I could be off on my solution

[shinmao]

hi @tomsanbear , thanks for your response. I just saw your patch with bytemuck::cast_slice that could throw error when cast to a larger-aligned slice. Also, I saw you mentioned that webgpu project heavily relied on the usages of slice::from_raw_parts. Do you think it could cause some serious security impacts in the projects? Would you like to share the case with us? :)