Bound GGUF metadata string/array values against the file mapping

[x14ngch3n]

Summary

Bounds the length of STRING and ARRAY metadata KV values against the mmap'd file mapping in load_gguf, mirroring the existing check_tensor_in_file() guard that protects the tensor data path. A crafted GGUF file could otherwise force an out-of-bounds read past the file mapping.

Problem

gguf_get_key() returns a pointer (val) into the mmap'd file but performs no bounds checking, and the metadata value lengths are read straight from the file. In set_mx_value_from_gguf, these attacker-controlled lengths were fed directly to a copy/string constructor with no check against the mapping size:

STRING KV:

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

val->string.len is a uint64_t from the file, narrowed to int, then std::string(ptr, n) memmoves n bytes out of the mmap.

ARRAY KV:

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

array(T*, Shape, Dtype) allocates size*itemsize and std::copys size elements from the mmap — a pure source over-read.

The tensor path was already bounded by check_tensor_in_file() (added in #4179 / fixes #4136), but the metadata path had no equivalent guard.

Impact

Out-of-bounds read past the mmap'd GGUF file when loading an untrusted .gguf. Confirmed with AddressSanitizer (4 MB read on a ~50-byte file, both sinks). In a non-ASAN process the read crosses into an unmapped page → SEGV (denial of service). No write primitive (the allocation is derived from the same len, so it is a source over-read, not an over-write).

Reachable from the default-config public Python API on every platform mlx ships:

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

MLX_BUILD_GGUF is ON by default.

Fix

Adds check_metadata_value_in_file(ctx, val, value_bytes), which verifies the value region lies within [0, ctx->size). Each metadata branch now bounds its value length against the mapping before any copy/string construction:

• STRING: rejects len that doesn't fit in an int or exceeds ctx->size, then bounds sizeof(gguf_string) + len.
• ARRAY: computes elt_size from the declared element type and checks arr_len * elt_size <= remaining (overflow-safe division form) before the element loop.
• STRING array elements: each inner str_val->len is bounded individually.

The static_cast<int> narrowing is also guarded so a length above INT_MAX is rejected explicitly rather than wrapping to a negative int.

Testing

Built with clang -O1 -g -fsanitize=address -fno-omit-frame-pointer (LLVM clang, since libmlx is built with it). Three crafted GGUFs (over-long STRING, over-long UINT32 ARRAY, and STRING with len > INT_MAX):

Before (on main), 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

After (this branch): all three crafted files are rejected cleanly:

exception: [load_gguf] String metadata value length exceeds file size.

no ASAN violation, process exits 0/1 via exception.

No regression: a legitimate roundtrip (save_gguf of a small float tensor + a STRING and an INT32 ARRAY metadata value, then load_gguf) still loads correctly with the new bounds in place.

Prior art

Distinct from #4136/#4179 (tensor data offset/bsize — tensor only), #3436 (gguflib -UNDEBUG asserts — mlx bypasses gguf_do_with_value), and CVE-2025-62609 (different sink). None of these bound the metadata KV value length.

[zcbenz]

Can you do simple check in load_metadata like what #4179 did?

[x14ngch3n]

Thanks @zcbenz. Done in the latest push: the bounds check now lives in a single check_metadata_value_in_file(ctx, type, val) call inside load_metadata() (one call per key, before set_mx_value_from_gguf consumes it), mirroring check_tensor_in_file() on the tensor path. set_mx_value_from_gguf is back to reading lengths straight from the file — the STRING/ARRAY validation (fixed scalars, length-prefixed strings, fixed-size arrays, and each element of a string array) is centralized in the validator. Added "test gguf metadata value validation" next to the tensor-offset test. All four gguf cases pass under ASAN (-O1).

[zcbenz]

feels meaningless to test int_max?

[zcbenz]

can you deduplicate the code

[x14ngch3n]

Addressed both comments in e9b3ac1:

• Extracted gguf_value_type_size() shared by the scalar and array-element paths, and folded the string validation (single + array walk) into one check_string lambda.
• The per-string int max checks were indeed redundant once the file-mapping bound is checked, so instead of defending the narrowing I removed it: std::string now takes the uint64_t length directly. The array-length int max check stays because the downstream array() shape is int-valued.