Fix integer overflow in RKNPU implicit bias allocation

[Gopalakrishnan Nallasamy (GopalakrishnanN)]

Description

The RKNPU execution provider's ONNX converter creates implicit (all-zero) bias
buffers when a Conv / Gemm / QLinearConv node omits its bias input. The
buffer size was computed as sizeof(T) * dim (where dim derives from a model
weight's shape) with no overflow check, and the raw allocation was tracked in a
manually-freed void* list.

This PR hardens the converter:

• Dimension validation: ONNX int64_t dimensions are validated (via
  ORT_ENFORCE in a new ToRknpuDim helper) before being narrowed to the RKNPU
  uint32_t shape representation, rejecting negative or out-of-range values.
  This covers all four ingestion points (HandleInitializer,
  GetInputOfOnnxModel, GetShape, GetSupportedNodes).
• Overflow-checked allocation: all four implicit-bias sites
  (AddLayerConvImpl, AddLayerQLinearConvImpl, AddLayerDepthwiseConvImpl,
  AddLayerFC) go through a shared AllocZeroedBias helper that computes the
  byte count with SafeInt<size_t> (throws on overflow) and returns a
  zero-initialized std::make_unique<uint8_t[]> buffer.
• RAII ownership: free_list_ is now
  std::vector<std::unique_ptr<uint8_t[]>>, so the bias buffers are freed
  automatically and Clear() no longer walks/free()s raw pointers.

Motivation and Context

A malicious ONNX model can provide dimensions that are unsafe for the RKNPU
converter's 32-bit shape representation or for byte-size allocation arithmetic:

• ONNX stores dimensions as int64_t, while the RKNPU converter/DDK uses
  uint32_t shape values. Silently narrowing a large or negative int64_t
  value can produce a misleading uint32_t dimension.
• Even after a dimension is represented as uint32_t, the original
  sizeof(T) * dim could overflow size_t on 32-bit RKNPU targets. For example,
  dim = 0x40000400 makes sizeof(float) * dim wrap to 4096 bytes while the
  created tensor still advertises dim elements, which would corrupt the heap
  when the bias is consumed by the driver.
• The original code also passed the malloc result to memset without a null
  check.

The fix uses SafeInt<size_t> (the ORT-standard idiom for memory-size
arithmetic), validates ONNX dimensions before they enter the RKNPU uint32_t
shape model, and replaces the manual malloc/free list with zero-initialized,
RAII-owned std::make_unique<uint8_t[]> buffers.

Validation: the RKNPU EP requires the Rockchip DDK (RKNPU_DDK_PATH) and an
ARM target, so it does not build on a typical x64 dev box and has no GPU-free CI
leg. The change was validated with clang-format, git diff --check, and a
standalone test against ORT's SafeInt.hpp, confirming the guard throws on the
overflowing dimensions (0x40000400, 0xFFFFFFFF) while preserving normal and
zero-dimension behavior.

Testing: No unit test is included because the RKNPU EP is not compiled in
any CI leg (it requires the proprietary Rockchip DDK and an ARM target), and the
ToRknpuDim / AllocZeroedBias helpers have internal (file-static) linkage,
so they are not reachable from the gtest suites. The overflow/validation logic
was instead exercised with a standalone test against ORT's SafeInt.hpp as
noted above.

[Copilot]

Pull request overview

This PR hardens the RKNPU execution provider’s ONNX-to-RKNN converter against attacker-controlled shape dimensions by preventing sizeof(T) * dim wraparound and by avoiding memset on a null allocation when creating implicit (all-zero) bias buffers.

Changes:

• Add a shared AllocZeroedBias helper that computes allocation sizes with SafeInt<size_t> and checks malloc results before zeroing.
• Route implicit-bias allocations in Conv/QLinearConv/DepthwiseConv/FC layer builders through the helper.
• Include ORT’s safeint.h to use the standard overflow-checked arithmetic idiom.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Dmitri Smirnov (yuslepukhin)]

1. malloc/free vs C++ allocation (yuslepukhin's comment, unaddressed)
   AllocZeroedBias uses malloc + memset, but ORT convention is new (std::nothrow) char[num_bytes] + zero-initialization. The free_list_ presumably calls free() in the destructor — if only this helper is switched to new[], you get undefined behavior (mixing free with new[]). The fix must be coordinated: change both allocation and deallocation together.

2. Raw pointer free_list_ — exception-safety gap
   The pattern at each call site is:

void* ptr = AllocZeroedBias(sizeof(float), dim);
free_list_.push_back(ptr);  // <-- can throw (vector reallocation)

If push_back throws, the allocated ptr is leaked. This is a pre-existing issue not introduced by this PR, but it's a latent leak. A safer pattern would use a std::unique_ptr.

3. Unnecessary ORT_TRY/ORT_CATCH wrapper (yuslepukhin's comment, unaddressed)
   SafeInt<size_t> already throws OnnxRuntimeException on overflow. The try/catch that re-throws with a different message adds code complexity for no functional benefit. In no-exceptions builds, ORT_TRY/ORT_CATCH compiles to nothing anyway, potentially leaving num_bytes = 0 undetected (though ORT_ENFORCE on the null check would catch it downstream).

4. Zero-dimension edge case
   If count == 0, malloc(0) is implementation-defined (may return NULL or a unique pointer). The code handles this via ORT_ENFORCE(ptr != nullptr || num_bytes == 0), and skips memset if ptr is NULL. But downstream consumers receiving a NULL bias pointer could dereference it if they don't also check for zero elements. Unlikely in practice (zero-dim bias is nonsensical), but undocumented.

5. Security-descriptive comment
   Lines 132–138 explicitly describe the attack vector ("malicious model", "heap buffer overflow").
   This is specifically prohibited
   For security fixes, ORT convention is neutral language. Should be trimmed to something like:

// Allocates a zero-initialized bias buffer with overflow-checked size arithmetic.

6. free_list_ ownership model
   The free_list_ is a std::vector<void*> with manual cleanup — a classic C pattern that's fragile. Do not write C code.
   If any code path between allocation and destructor throws (or if the object is moved/copied), pointers leak or double-free. This PR doesn't worsen it, but it perpetuates an unsafe idiom. Ideally these would be std::vector<std::unique_ptr<char[]>> or similar RAII wrapper.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.