Validate Col2Im inputs to prevent heap over-read#28706
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the CPU Col2Im kernel by validating runtime shape inputs before dispatching to the underlying math routines, preventing a column-buffer over-read when image_shape implies more sliding blocks than the input tensor contains.
Changes:
- Adds rank, dimensionality, positivity, padded extent, channel/block, and sliding-block count validation in
col2im.cc. - Corrects the N-D
Col2imNdoutput-shape argument to use computed sliding-block extents. - Adds regression and stride-based 5D coverage for the CPU
Col2Imoperator.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
onnxruntime/core/providers/cpu/tensor/col2im.cc |
Adds input validation and passes correct sliding-block dimensions to N-D col2im. |
onnxruntime/test/providers/cpu/tensor/col2im_test.cc |
Adds tests for 5D strided behavior and the over-read regression case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tianleiwu
left a comment
There was a problem hiding this comment.
Thanks for the well-targeted hardening fix. The core change correctly closes the heap over-read by validating the column tensor against the sliding-block count derived from image_shape/block_shape/pads/strides/dilations, and it also fixes the output_shape argument passed to Col2imNd (the old adjusted_image_shape_dims ignored pads and strides). Converting the attribute-size ORT_ENFORCE checks to ORT_RETURN_IF_NOT is the right move for untrusted models, and the col_shape[2] check guards both the 2-D and N-D dispatch paths.
One remaining crash vector (inline): the strides and dilations attribute values are never validated as positive, so a crafted model with strides = [0, 0] causes an integer division by zero at the new sliding-block computation. Adding positivity checks would close the last crafted-model DoS in this path.
328a9b7 to
ce3f24a
Compare
tianleiwu
left a comment
There was a problem hiding this comment.
Thanks for the fix — bounding col_shape[2] against the derived sliding-block count closes the heap over-read cleanly, and the per-dimension strides/dilations positivity checks are now placed before they are used as divisors.
One concrete issue: two of the new tests assert error substrings that do not match the messages the kernel actually emits, so they will fail in CI (details inline).
Nit: the PR description mentions a single new test and "7 tests (6 existing + 1 new)", but the diff adds four tests (WithStrides5dNCHWD, ImageShapeLargerThanColumnTensor, StridesMustBePositive, DilationsMustBePositive). Please update the description and the local count.
tianleiwu
left a comment
There was a problem hiding this comment.
Re-reviewed at head 308d8dd. All previously raised concerns are addressed and the corresponding threads are resolved:
strides[i]/dilations[i]are now validated as positive in the per-dimension loop before being used as a divisor / kernel-extent factor, closing the SIGFPE-on-crafted-model gap.- The expected substrings in
StridesMustBePositiveandDilationsMustBePositivenow match the kernel's emitted messages.
The core hardening is sound: the kernel now rejects inputs whose col_shape[2] does not equal the sliding-block count derived from image_shape/block_shape/pads/strides/dilations, plus rank/positivity/multiple-of-block checks, with overflow-safe SafeInt arithmetic. Switching the ND path from adjusted_image_shape_dims (stride=1/pad=0 assumption) to the correctly computed sliding_block_shape_dims is also a genuine correctness fix for strided/padded inputs, and is covered by the new WithStrides5dNCHWD test. The added negative tests give good coverage of each new validation path. Converting the attribute-size ORT_ENFORCE checks to ORT_RETURN_IF_NOT is the right call for surfacing a clean Status. LGTM.
The CPU Col2Im kernel derived the output spatial extent from the 'image_shape' and 'block_shape' inputs and dispatched to math::Col2im / math::Col2imNd without checking that the column tensor actually contained that many sliding-block positions. A crafted model whose 'image_shape' implied more positions than the column allocation held could cause the loop to read past the column buffer and copy adjacent heap contents into the output tensor, leaking process memory to the caller. Validate that 'image_shape' and 'block_shape' are 1-D and equal length, that all image / block / padded extents are positive and large enough for the dilated kernel, that the column tensor is rank 3, and that col_shape[1] is a positive multiple of prod(block_shape) and col_shape[2] equals the expected number of sliding blocks. Add a regression test that mirrors the reported PoC and expects a runtime failure.
Keep the N-D Col2Im path consistent with the input validation by passing the same per-dimension sliding-block counts to math::Col2imNd. Add a strided 5-D regression test that would previously validate the shorter column tensor but iterate too many column positions.
The DML execution provider rejects the inconsistent shapes during graph partitioning with a different error message, causing the ImageShapeLargerThanColumnTensor negative test to fail in the windows_dml CI job. Exclude the DML EP so the expected error-text assertion only runs against the CPU kernel.
Add per-dimension positivity checks before computing Col2Im sliding block counts, and cover zero stride/dilation attributes with negative tests.
7292a9d to
62a51b2
Compare
tianleiwu
left a comment
There was a problem hiding this comment.
Re-reviewed at head 62a51b28. The final commit aligns the Compute() strides/dilations error messages (All stride values must be positive / All dilation values must be positive) with both the constructor guard and the expected substrings in the new tests, closing the last open thread.
Verification of the core fix:
- N-D path — Swapping
adjusted_image_shape_dims(which assumed stride 1 / no pad) for the stride/pad/dilation-awaresliding_block_shape_dimsis correct.Col2imNdforwards this asoutput_shapetoIm2col, where it drives the column index (index_col *= output_shape[d_i]andNextPosition(rank, output_shape, ...)). The previous value produced an incorrect column index for strided/padded N-D inputs, so this is a genuine correctness fix in addition to the bounds hardening. - 2-D path —
col_shape[2] == expected_col_blocksexactly matches theoutput_h * output_wextent thatmath::Col2imreads internally, so the validatedcoltensor can no longer be over-read. - Divisor / overflow safety —
strides[i] > 0,dilations[i] > 0, andkernel_shape_size > 0are all checked before being used as divisors, andpadded_extent >= adjusted_kernelprevents a negative sliding-block count. Size arithmetic usesSafeInt. - Converting the attribute-size
ORT_ENFORCEchecks toORT_RETURN_IF_NOTgives cleanStatusfailures consistent with the new validation.
The added regression tests cover the original reproducer plus the strides/dilations/padded-kernel/channel-multiple guards. All previously raised concerns are addressed and resolved. LGTM.
|
@GopalakrishnanN, you do not need the last commit since the fix is in main branch. Simple fix is to revert it, and the PR is ready to merge (the failed CI is not required right now). |
This reverts commit 0061d8d.
Description
The CPU
Col2Imkernel derived the output spatial extent from theimage_shapeandblock_shaperuntime inputs and dispatched tomath::Col2im/math::Col2imNdwithout checking that the column tensor actually contained the implied number of sliding-block positions.A crafted model whose
image_shapeimplies more sliding-block positions than the column allocation holds caused the inner loop inmath::Col2imto advance past the end ofdata_coland copy adjacent heap contents into the output tensor, leaking process memory to the caller.Motivation and Context
Heap buffer over-read in
Col2Imreported as an information-disclosure issue. Reproducer:colshape[1, 1, 4]withimage_shape = [4, 4]andblock_shape = [1, 1]produces a[1, 1, 4, 4]output whose first 4 elements are the input data and whose remaining 12 elements are uninitialized heap values that vary across runs.Fix
In
onnxruntime/core/providers/cpu/tensor/col2im.cc, validate inputs before dispatch:image_shapeandblock_shapeare 1-D and have the same length, with at least one spatial dimension.image_shapeandblock_shapevalues are positive.col(input 0) is rank 3.col_shape[1]is a positive multiple ofprod(block_shape).col_shape[2]equals the expected sliding-block count derived fromimage_shape,block_shape,pads,strides, anddilations.Existing
ORT_ENFORCEattribute-size checks are converted toORT_RETURN_IF_NOTso all validation surfaces as a cleanStatusfailure rather than an exception.Test
Added regression test
Col2ImOpTest.ImageShapeLargerThanColumnTensorinonnxruntime/test/providers/cpu/tensor/col2im_test.ccthat mirrors the reproducer and expects the kernel to fail with a message identifying the sliding-block mismatch.Local run of
onnxruntime_provider_test --gtest_filter=Col2ImOpTest.*on Windows / Release passes all 7 tests (6 existing + 1 new).