ICM fixes (6/n)#28255
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses reported ICM issues by tightening validation for CPU ML SVMClassifier and LinearClassifier kernels to prevent inconsistent attributes/input shapes from leading to incorrect reads, and adds regression coverage for the new failure cases.
Changes:
- Add stricter
SVMClassifier(SVC mode) attribute validation: non-negativevectors_per_class, size match vsclass_count, andsupport_vectorsdivisibility byvector_count. - Track
LinearClassifier’s expectedfeature_countfrom the coefficients and require runtime input feature-count to match. - Add new negative tests to ensure the intended failures and messages are exercised.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| onnxruntime/test/providers/cpu/ml/svmclassifier_test.cc | Adds regression test for vectors_per_class size mismatch failure. |
| onnxruntime/test/providers/cpu/ml/linearclassifer_test.cc | Adds regression test for LinearClassifier input feature-count mismatch failure. |
| onnxruntime/core/providers/cpu/ml/svmclassifier.cc | Adds additional SVC-mode validation guards to prevent inconsistent attribute layouts. |
| onnxruntime/core/providers/cpu/ml/linearclassifier.h | Adds feature_count_ member to track expected input feature count. |
| onnxruntime/core/providers/cpu/ml/linearclassifier.cc | Computes feature_count_ from coefficients and enforces runtime input feature-count equality. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
tianleiwu
left a comment
There was a problem hiding this comment.
Review Summary
Clean hardening of ML operator input validation for LinearClassifier and SVMClassifier, fixing ICM incidents from crafted models.
LinearClassifier
-
Positive: Precomputing
feature_count_in the constructor and enforcing exact match (num_features == feature_count_) at Compute time is a cleaner design than the previous runtime overflow-checked multiplication. It moves the coefficient/class arithmetic to construction time, making the Compute path simpler and free ofSafeMultiply. TheORT_ENFORCE(feature_count_ > 0)catches degenerate empty-coefficients models early. -
Note: This changes the validation from
coefficients_.size() >= class_count * num_features(surplus coefficients silently ignored) to strict equality. This could reject previously-accepted models if they had surplus coefficients, though such models would be malformed per the ONNX ML spec.
SVMClassifier
- Positive: Good layered validation — validates
vectors_per_class[i] >= 0before thenarrow<ptrdiff_t>()cast, checksvectors_per_class_.size() == class_count_to prevent OOB reads, validatesvector_count_ > 0to prevent division-by-zero in feature count derivation, and checkssupport_vectors_.size() % vector_count_ == 0for consistent dimensionality.
Tests
- Test coverage is thorough: the new
LinearClassifierInputFeatureCountMismatchFailsandSVMClassifierVectorsPerClassSizeMismatchtests directly target the new validation, and the updated existing tests correctly match the changed error messages.
Overall a well-scoped security fix with good test coverage.
|
It seems that backend test failed due to the code change in linux CI. Please take a look. |
Yeah- not sure if the backend tests are faulty or if some chcks are too tight. Will debug when I get some time. |
…into hari/icm_6
Description
Fixes:
https://portal.microsofticm.com/imp/v5/incidents/details/31000000586963
https://portal.microsofticm.com/imp/v5/incidents/details/31000000586944
Motivation and Context
Fix ICM issues