Bound GGUF tensor data offsets against the file mapping - #4179
Merged
zcbenz merged 1 commit intoAug 12, 2026
Conversation
gguflib computes a tensor's data pointer as ctx->data + ctx->data_off + the tensor's offset field, in unsigned arithmetic and without comparing the result against the mapping. A crafted offset therefore produces a pointer outside the mapped file, and mlx's own memcpy in extract_tensor_data then reads it. If the addition wraps, the pointer lands back inside the mapping and the wrong bytes are read silently. Validate offset and size against the file in load_arrays, which is the single point both the plain and the quantized paths pass through.
zcbenz
approved these changes
Aug 12, 2026
This was referenced Aug 12, 2026
x14ngch3n
added a commit
to x14ngch3n/mlx
that referenced
this pull request
Aug 13, 2026
Move check_metadata_value_in_file() out of set_mx_value_from_gguf and call it once per key in load_metadata(), mirroring check_tensor_in_file() on the tensor path (ml-explore#4179). set_mx_value_from_gguf is back to reading value lengths straight from the file; all STRING/ARRAY bounds checking (fixed scalars, length-prefixed strings, fixed-size arrays, and each element of a string array) now lives in a single validator invoked before the value is consumed. Lengths that would not fit in the int the downstream array() / std::string constructors take are rejected there too. Adds "test gguf metadata value validation" covering valid empty/small strings plus OOB string, far-past-end string, fixed-size array, and string array element cases (ASAN, -O1). Co-Authored-By: Claude <noreply@anthropic.com>
1 task
zcbenz
pushed a commit
to x14ngch3n/mlx
that referenced
this pull request
Aug 18, 2026
The tensor load path validates offset and byte size against the mmap'd file (check_tensor_in_file, ml-explore#4179). The metadata path did not: set_mx_value_from_gguf read val->string.len / val->array.len straight from the file and passed them to std::string / array construction, so a crafted STRING or ARRAY metadata value could claim a length far larger than the file and force a read past the mapping (out-of-bounds read, SEGV / potential memory disclosure). gguf_get_key() performs no bounds checking of its own, and mlx does not use gguflib's bounded gguf_do_with_value walk, so nothing else caught this. Distinct from ml-explore#4136/ml-explore#4179 (tensor data offset), ml-explore#3436 (gguflib asserts), and CVE-2025-62609. Add check_metadata_value_in_file() mirroring check_tensor_in_file, and bound each STRING and ARRAY metadata value (including each element of a string array) against the mapping before any copy. Lengths that would narrow badly to int are rejected before the static_cast. Reproduced under AddressSanitizer on main (4 MB over-read on a ~50-byte file at gguf.cpp:128 STRING and :155 ARRAY); after this change the same PoCs throw cleanly and a normal save/load round trip still succeeds. Co-Authored-By: Claude <noreply@anthropic.com>
zcbenz
pushed a commit
to x14ngch3n/mlx
that referenced
this pull request
Aug 18, 2026
Move check_metadata_value_in_file() out of set_mx_value_from_gguf and call it once per key in load_metadata(), mirroring check_tensor_in_file() on the tensor path (ml-explore#4179). set_mx_value_from_gguf is back to reading value lengths straight from the file; all STRING/ARRAY bounds checking (fixed scalars, length-prefixed strings, fixed-size arrays, and each element of a string array) now lives in a single validator invoked before the value is consumed. Lengths that would not fit in the int the downstream array() / std::string constructors take are rejected there too. Adds "test gguf metadata value validation" covering valid empty/small strings plus OOB string, far-past-end string, fixed-size array, and string array element cases (ASAN, -O1). Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Fixes #4136 (thanks @professor-moody for the report).
gguflibsetstensor->weights_data = ctx->data + ctx->data_off + *offsetfrom afile-controlled offset without bounding it against
ctx->size(gguflib.c:297-298), andmlx's
memcpyinextract_tensor_data(mlx/io/gguf.cpp:71) reads through it. A craftedoffset points outside the mapping; if the addition wraps it points back inside, so the load
silently returns wrong bytes instead of faulting.
This validates offset and extent in
load_arrays, the one point both the plain and thequantized paths pass through, using only
gguf_ctxfields mlx already has. mlx does the samea few lines away at
gguf.cpp:67(CVE-2025-62609) and for safetensors atsafetensors.cpp:196.It is still needed if gguflib is fixed upstream. The pending fix there,
antirez/gguf-tools#33, signals a rejected tensor with
return 0, which in awhile (gguf_get_tensor(...))loop is indistinguishable from "no tensors left". Patchingthe pinned gguflib with #33 locally, a 2-tensor file with one bad offset loads 1 tensor,
left_tensors == 0, no error — silently missing weights rather than a raise.Not addressed by #3436: the only assert on this path is
ndim(gguflib.c:275), so-UNDEBUGhas nothing to keep alive foroffset/bsize.Scope: this bounds the tensor data region against the file. It does not make GGUF loading
safe against arbitrary crafted files -- internally consistent but false metadata is a
separate class, not addressed here.
Without the guard two of the new subcases crash (SIGSEGV, or SIGBUS depending on where the
bad pointer lands) and four read wrong bytes without raising; all pass with it. The helper
follows
write_raw_safetensorsabove it.Checklist
Put an
xin the boxes that apply.pre-commit run --all-filesto format my code / installed pre-commit prior to committing changesAI assistance was used in developing this change (Opus 5); I reviewed every changed line and
ran the commands above myself.