Skip to content

Add validation for negative position_ids in EmbedLayerNorm#27573

Merged
vraspar merged 3 commits intomainfrom
vraspar/embedLayerNorm
Mar 10, 2026
Merged

Add validation for negative position_ids in EmbedLayerNorm#27573
vraspar merged 3 commits intomainfrom
vraspar/embedLayerNorm

Conversation

@vraspar
Copy link
Copy Markdown
Contributor

@vraspar vraspar commented Mar 5, 2026

Description

Adds a missing lower-bound validation check for position_col_index (derived from position_ids input) in the EmbedLayerNormalization contrib operator. A negative value in position_ids could bypass the existing upper-bound-only check and cause an out-of-bounds heap read before the position_embedding buffer.

Motivation and Context

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a missing lower-bound validation check for position_col_index (derived from position_ids input) in the EmbedLayerNormalization CPU contrib op, preventing an out-of-bounds heap read when a negative value is passed as a position ID. A regression test is also added to verify the fix.

Changes:

  • Adds position_col_index < 0 guard in embed_layer_norm.cc (the non-quantized CPU path where position_ids is user-supplied data, fixing a real OOB read vulnerability)
  • Adds the same position_col_index < 0 guard in qembed_layer_norm.cc (the quantized CPU path, where this check is dead code since position_col_index = index % sequence_length and index is always non-negative)
  • Adds a regression test EmbedLayerNormNegativePositionIds in the test file

Reviewed changes

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

File Description
onnxruntime/contrib_ops/cpu/bert/embed_layer_norm.cc Adds < 0 lower-bound check to prevent OOB heap read on negative position_ids — this is the real fix
onnxruntime/contrib_ops/cpu/quantization/qembed_layer_norm.cc Adds < 0 check to position_col_index, which is dead code since the value is always index % sequence_length (non-negative)
onnxruntime/test/contrib_ops/embed_layer_norm_op_test.cc Regression test for the negative position_ids validation fix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread onnxruntime/contrib_ops/cpu/quantization/qembed_layer_norm.cc Outdated
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can commit the suggested changes from lintrunner.

Comment thread onnxruntime/test/contrib_ops/embed_layer_norm_op_test.cc Outdated
yuslepukhin
yuslepukhin previously approved these changes Mar 6, 2026
@yuslepukhin
Copy link
Copy Markdown
Member

Make sure to comment on the Copilot comments and resolve them

Copy link
Copy Markdown
Member

@yuslepukhin yuslepukhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

3 participants