Skip to content

fix: guard TensorProto data size against dims in createElmAttrFromArray - #3616

Draft
jonathanhuang-bot wants to merge 1 commit into
onnx:mainfrom
jonathanhuang-bot:fix/f005-tensorproto-heap-overflow
Draft

fix: guard TensorProto data size against dims in createElmAttrFromArray#3616
jonathanhuang-bot wants to merge 1 commit into
onnx:mainfrom
jonathanhuang-bot:fix/f005-tensorproto-heap-overflow

Conversation

@jonathanhuang-bot

Copy link
Copy Markdown
Collaborator

The destination buffer in fromRawBytes() is sized from the tensor's declared dims (getNumElements()*bytewidth), but std::transform in createElmAttrFromArray wrote array.size() elements without checking that the two match. A crafted .onnx with more data bytes than dims implied caused a heap buffer overflow with fully attacker-controlled length and bytes.

Hunk 1 (FrontendDialectHelper.cpp): add a size check in createElmAttrFromArray before calling fromArray. This closes the raw_data and typed proto field (int32_data/float_data/etc.) paths, which both funnel through this chokepoint unconditionally.

Hunk 2 (ElementsAttrBuilder.cpp): add a size check in fromMemoryBuffer. On little-endian hosts (and for single-byte dtypes on any host), the EXTERNAL data-location path calls fromMemoryBuffer directly, bypassing createElmAttrFromArray entirely. This second guard closes that remaining path.

Both checks use llvm::report_fatal_error so they fire in Release builds (unlike assert which is a no-op under NDEBUG).

Analysis file for this bug: disposition-f005-analysis.md

The destination buffer in fromRawBytes() is sized from the tensor's
declared dims (getNumElements()*bytewidth), but std::transform in
createElmAttrFromArray wrote array.size() elements without checking
that the two match. A crafted .onnx with more data bytes than dims
implied caused a heap buffer overflow with fully attacker-controlled
length and bytes.

Hunk 1 (FrontendDialectHelper.cpp): add a size check in
createElmAttrFromArray before calling fromArray. This closes the
raw_data and typed proto field (int32_data/float_data/etc.) paths,
which both funnel through this chokepoint unconditionally.

Hunk 2 (ElementsAttrBuilder.cpp): add a size check in
fromMemoryBuffer. On little-endian hosts (and for single-byte dtypes
on any host), the EXTERNAL data-location path calls fromMemoryBuffer
directly, bypassing createElmAttrFromArray entirely. This second guard
closes that remaining path.

Both checks use llvm::report_fatal_error so they fire in Release
builds (unlike assert which is a no-op under NDEBUG).

Signed-off-by: Jonathan Huang <jonathanhuang@ibm.com>
@jenkins-droid

Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@jonathanhuang-bot

Copy link
Copy Markdown
Collaborator Author

Note on deviation from the fable candidate patch (bug_06)

The fable candidate patch bug_06/patch.diff adds a size guard to createElmAttrFromArray in FrontendDialectHelper.cpp. That guard is included here unchanged (Hunk 1). However, bug_06 alone is not a complete fix — a third attacker-reachable code path bypasses createElmAttrFromArray entirely and was left unguarded. This PR adds a second guard (Hunk 2) to close it.

Why bug_06 is incomplete:

A TensorProto can carry its element data in three ways. Two of them — raw_data and typed proto fields (float_data, int32_data, etc.) — always funnel through createElmAttrFromArray, so bug_06's guard covers them. The third — data_location == EXTERNAL — routes through createElementsAttrFromMemoryBuffer_LE, which contains a compile-time branch:

// FrontendDialectHelper.cpp
template <typename T>
ElementsAttr createElementsAttrFromMemoryBuffer_LE(...) {
  if constexpr (shouldSwapLEBytes<T>) {
    // multi-byte dtype on big-endian host → goes through createElmAttrFromArray ✅
    return createElmAttrFromArray<T>(...);
  } else {
    // BYPASSES createElmAttrFromArray entirely ❌
    return OnnxElementsAttrBuilder(ctx).fromMemoryBuffer(...);
  }
}

shouldSwapLEBytes is sizeof(T) > 1 && native_endianness != little. It is false — sending EXTERNAL data straight to fromMemoryBuffer with no size check — in two cases:

  1. Single-byte dtypes (INT8, UINT8, BOOL) on any host — sizeof(T) == 1, so the condition is always false regardless of host endianness.
  2. Any dtype on a little-endian host (x86, ARM) — the common case for local development and CI.
    The only scenario where bug_06 alone would catch an EXTERNAL-path overflow is a multi-byte dtype compiled on a big-endian host (s390x) — a narrow subset of real deployments.

What Hunk 2 adds:

A byte-level size check in ElementsAttrBuilder::fromMemoryBuffer — comparing membuf->getBufferSize() against type.getNumElements() * bytewidthOfBType(btype) — that fires before the buffer is wrapped in a DisposableElementsAttr and handed to downstream compilation passes. This closes the EXTERNAL path for all dtypes on all hosts. The check is compile-time-only (model import), with zero inference-path cost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants