Bound tensor data offset/size against the mapping in gguf_get_tensor (follow-up to #28)

[gigioneggiando]

Follow-up to #28, as suggested there.

#28 bounds the metadata / tensor-info walk, but the tensor data region is still unchecked. In gguf_get_tensor():

tensor->offset = ctx->data_off + *offset;          // *offset is raw, file-controlled
tensor->weights_data = ctx->data + tensor->offset; // pointer formed with no bound vs ctx->size

*offset and the resulting tensor->bsize are never checked against ctx->size, so a crafted tensor offset/size places weights_data (or its bsize-byte extent) outside the mapping. Any later read of the weights (gguf_tensor_to_float, inspect-tensor, compare) is then an out-of-bounds read.

Reproduction

A one-tensor GGUF whose data offset is 1 MiB (well past the file), driven through the real gguf_get_tensor:

[poc] file size=57, crafted tensor data offset=0x100000
before: gguf_get_tensor ACCEPTED it (return 1); weights_data is OUTSIDE the mapping -> OOB read primitive
after : gguf_get_tensor REJECTED the out-of-range offset (return 0)

Fix

Compute bsize first, then verify (overflow-safe) that *offset and the resulting absolute offset are within ctx->size and that the bsize-byte extent fits, before weights_data is formed; return 0 otherwise.

This touches only the offset/bsize tail of gguf_get_tensor, not the dimension/ndim area #28 changes, so it should apply cleanly alongside #28 (happy to rebase on top of it if you merge #28 first).

[professor-moody]

This bound matters beyond gguf-tools itself: ml-explore/mlx (Apple's MLX array framework) vendors this exact gguflib.c at a pinned commit and exposes it through mx.load(path) on any .gguf file. Because gguf_get_tensor sets tensor->weights_data = ctx->data + ctx->data_off + *offset from an unchecked file-controlled *offset, a crafted .gguf whose tensor offset points past the mapping yields a non-NULL out-of-range pointer; MLX's consumer null-checks it (the fix for CVE-2025-62609) but not the bound, then memcpys from it.

I re-verified on mlx HEAD (2026-07-17) that the offset is still unguarded: mlx's -UNDEBUG on the gguflib target keeps only the ndim assert active, not any offset bound. This PR (bounding tensor->offset/bsize against ctx->size) is the fix; landing it lets mlx and the other gguflib consumers bump their pin. Happy to help with the mlx-side bump.

[gigioneggiando]

Thanks for digging into the downstream angle — that's a really useful data point. Good to know MLX vendors gguflib.c at a pinned commit and that mx.load() exposes gguf_get_tensor's offset/bsize path directly; confirms this isn't just a theoretical hardening fix for gguf-tools itself.

Your re-verification on mlx HEAD (2026-07-17) matches what I'd expect: CVE-2025-62609's fix null-checks the resulting pointer but doesn't touch the bound on *offset/bsize against the mapping size, so a crafted .gguf can still push weights_data out of range before that null-check ever gets a chance to matter — same root cause, still open on mlx's side.

Happy to help with the mlx-side pin bump once this lands here — let me know if/when you loop them in, or if you'd rather I reach out given I filed this one.

[gigioneggiando]

Quick follow-up: have you already reported this to the MLX team (e.g. via their private vulnerability reporting), or would you like me to file it? I checked ml-explore/mlx's current gguf.cpp directly — the null-check from CVE-2025-62609's fix is there, but nothing bounds tensor->weights_data / bsize against the actual mapping, so this is a distinct, currently-unpatched gap, not covered by that CVE or CVE-2025-62608. Happy to send it their way (their PVR is enabled) if you haven't already, crediting your write-up here either way.

[professor-moody]

@gigioneggiando

Thanks, this is a really useful independent confirmation, appreciate you rebuilding HEAD to check it.

So, worth consideration... the mlx maintainers treat model-loading memory-safety as out of their threat model (they've said loading a model file is not much different from loading a script, and closed the earlier GGUF reports on that basis). I have reported this to them, but it may get closed and fixed on the public side. The durable fix really is here in gguf-tools, once #33 lands, mlx and the other consumers can just bump their pin off 8fa6eb6.

So the most useful thing is getting #33 merged. Genuinely appreciate you digging into the downstream reachability, that's exactly the data point that shows why the bound matters beyond gguf-tools itself.

So, we'll see what they say.

[gigioneggiando]

Thanks for the update, and for reporting upstream. That threat-model stance from mlx (model loading != safe-by-default) is useful context. Agreed the durable fix is here — hoping this lands soon so mlx and the other gguflib consumers have a commit to pin against. Will follow up if there's any movement on merging it.