gguf : harden loader against malformed tensor dims and metadata types - #25596
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
Disclosure: this issue was found via libFuzzer + AddressSanitizer fuzzing of |
JohannesGaessler
left a comment
There was a problem hiding this comment.
Please add corresponding test cases to tests/test-gguf.cpp.
- report the expected vs. actual type when general.alignment is not u32 - use ggml_nelements() > 0 for the zero-element guard and keep the representability checks visually aligned - add test-gguf cases for a wrong-typed alignment key and a zero-dim tensor (both used to crash: assert-abort and SIGFPE respectively) Ran tests/test-gguf: 164/164 pass. Used an AI assistant to help draft these edits; reviewed and verified by me.
Added test cases in 3cd5206: two handcrafted files in tests/test-gguf.cpp — a general.alignment key with a wrong (non-u32) type, and a tensor with a zero-size dimension. Both used to crash before (assert-abort and SIGFPE); with the fix they load cleanly. Full suite passes (164/164). |
|
I ran an unrelated fuzzing campaign against Also did a manual pass on the surrounding overflow check — the ne[0]/ne[2]/ne[3] found via an AI-assisted fuzzing/static-analysis pipeline I built and ran myself, No new PoC needed since this PR's reproducer already covers it — just flagging that it's a |
|
LGTM @ggerganov? |
ggerganov
left a comment
There was a problem hiding this comment.
Seems OK.
My memory is a bit fuzzy on the topic of zero-sized tensors (i.e. ne[k] == 0). There is explicit logic there to allow them, but at the same time, I don't think there is any purpose to do so - these are basically empty tensors.
|
ggml-org/ggml#1517 (comment) They are to be allowed. safetensors allows them and they are used in some models (diffusion stuff) to tag it for a specific feature, instead of kv. |
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> * origin/master: (383 commits) cmake : introduce semantic versioning (ggml-org#26839) gguf : harden loader against malformed tensor dims and metadata types (ggml-org#25596) kleidiai: Add runtime feature detection mechanism for aarch64/kleidiai (ggml-org#26076) model : disallow integer dflash sliding_window_pattern (ggml-org#26900) sync : ggml cmake : add config version support (ggml/1582) server : support slot save/restore with media inputs (ggml-org#26640) ui: add read_media tool (ggml-org#25877) opencl: default FA c8 cluster width to 16 on X1E (ggml-org#26433) tests : update speculative params (ggml-org#26925) vulkan: add TQ2_0 (ternary) support (ggml-org#25850) wavtokenizer-dec : bound posnet/convnext block_count against n_layer_all (ggml-org#26892) convert : handle per_layer_config in Gemma4 (transformers 5.15) (ggml-org#26882) opencl: use flat mv q5_k when weight exceeds image1d_buffer_t limit (ggml-org#26880) chat : fix muse-glimmer detection of tool calls after EOM (ggml-org#26879) ci : add missing release check (ggml-org#26923) CUDA: only disable CUDA graphs when mul_mat_id actually needs a stream sync (ggml-org#26802) cuda : add warp-per-row wkv7 kernel for single-token decode (ggml-org#26111) spec : update speculative-simple (ggml-org#26904) chat : tighten bare function parsing for Qwen models (ggml-org#26793) ...
Overview
Hardens the GGUF loader (
gguf_init_from_buffer/gguf_init_from_reader) against two crashes reachable from a crafted, untrusted.gguffile. The loader is the trust boundary for any app that loads user-supplied GGUF (e.g.llama-serverand downstream tools), so a malformed model file should fail cleanly rather than crash the process.Additional information
Both issues were found while fuzzing
gguf_init_from_bufferwith libFuzzer + AddressSanitizer on currentmaster.1. SIGFPE (division by zero) on a zero tensor dimension
Dimensions are validated as
ne[j] >= 0, but the representability check then divides byne[1..3]:A tensor with
n_dims >= 2andne[1] == 0(allowed by the>= 0check) triggersINT64_MAX / 0→SIGFPE.Minimal PoC (65 bytes, base64):
R0dVRgMAAAABAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAB0AgAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=2. Assertion abort on wrong-typed
general.alignmentThe alignment key is read as
uint32without checking its declared type; a file declaringgeneral.alignmentwith any non-UINT32type hitsGGML_ASSERT(type_to_gguf_type<T>::value == type)→ggml_abort().Minimal PoC (62 bytes, base64):
R0dVRgMAAAAAAAAAAAAAAAEAAAAAAAAAEQAAAAAAAABnZW5lcmFsLmFsaWdubWVudAgAAAABAAAAAAAAAHg=Fix
ne[j] == 0(element count is 0, trivially representable).general.alignmentisGGUF_TYPE_UINT32before reading it; otherwise fail cleanly.Reproduce:
printf '<base64>' | base64 -d > poc.gguf, then load it through anygguf_init_from_*path.Requirements
gguf_init_from_bufferonmaster. The fix and the minimal PoCs were drafted with the help of an AI coding assistant and reviewed/verified by me. I am responsible for all submitted changes. Happy to adjust the approach if maintainers prefer different checks.