Skip to content

Out-of-bounds read in GGUF metadata loader (unbounded string/array length) #4213

Description

@x14ngch3n

Summary

mlx::core::load_gguf performs an out-of-bounds read past the mmap'd file when loading a crafted GGUF file. The metadata KV value lengths (STRING and ARRAY) are read straight from the file and fed to std::string / array copy constructors with no check against the file mapping size. A fix is open in #4212.

Impact

Loading an untrusted .gguf triggers an OOB read. Confirmed with AddressSanitizer: a ~50-byte crafted file forces a 4 MB read past the mapping (both the STRING and ARRAY sinks). In a non-ASAN process the read crosses into an unmapped page → SEGV (denial of service). No write primitive — the allocation and the copy both use the attacker-controlled length, so it is a source over-read, not an over-write.

Reachable from the default-config public API:

import mlx.core as mx
mx.load("evil.gguf", format="gguf")

MLX_BUILD_GGUF is ON by default. All platforms mlx ships (macOS/Linux).

Root cause

gguf_get_key() returns a pointer into the mmap'd file but performs no bounds checking. In set_mx_value_from_gguf (mlx/io/gguf.cpp):

// STRING sink
value = std::string(val->string.string, static_cast<int>(val->string.len));

// ARRAY sink
auto size = static_cast<int>(val->array.len);
value = array(reinterpret_cast<uint32_t*>(data), {size}, uint32);

val->string.len / val->array.len are uint64_ts taken directly from the file. The tensor data path is already bounded by check_tensor_in_file() (from #4179), but the metadata path has no equivalent guard.

Reproduce

Craft a GGUF whose header declares a STRING KV with len = 0x400000 but only a few real payload bytes, then load it:

import mlx.core as mx
mx.load("evil.gguf", format="gguf")   # ASAN: READ of size 4194304 past mapping

ASAN trace (STRING sink):

==95370==ERROR: AddressSanitizer: unknown-crash on address 0x000103ab0000
READ of size 4194304 at 0x000103ab0000 thread T0
    #6 mlx::core::set_mx_value_from_gguf(...) gguf.cpp:128
    #7 mlx::core::load_metadata(gguf_ctx*) gguf.cpp:209
    #8 mlx::core::load_gguf(...) gguf.cpp:279

Fix

PR #4212 bounds the metadata value length against ctx->size (mirroring check_tensor_in_file) before any copy/string construction, and guards the static_cast<int> narrowing. Verified: all crafted files are rejected cleanly and a legitimate GGUF roundtrip still loads.

Related

Found during an independent source audit. Happy to provide the PoC generator / ASAN harness if useful.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions