Skip to content

check intmul doesn't overflow modulo 2^128-1#910

Closed
josephjohnston wants to merge 1 commit intomainfrom
intmul-overflow-check
Closed

check intmul doesn't overflow modulo 2^128-1#910
josephjohnston wants to merge 1 commit intomainfrom
intmul-overflow-check

Conversation

@josephjohnston
Copy link
Copy Markdown
Contributor

@josephjohnston josephjohnston commented Sep 2, 2025

TL;DR

Added an overflow check to the integer multiplication gate to prevent potential vulnerabilities.

What changed?

Added an additional constraint to the imul.rs gate implementation that prevents overflow modulo 2^128-1. The implementation now verifies integer multiplication on the least significant bits by:

  1. Extracting the least significant bits using shift-left operations
  2. Adding an AND constraint to verify that x[0] * y[0] = lo[0]

This is accomplished by importing the sll function from the constraint builder module and adding the new AND constraint after the existing multiplication constraint.

How to test?

Run the existing test suite to ensure that the integer multiplication gate still functions correctly with the added overflow protection. Consider adding specific tests that attempt to trigger overflow conditions to verify the new constraint is working as expected.

Why make this change?

This change addresses a potential security vulnerability where overflow modulo 2^128-1 could occur in integer multiplication operations. The additional constraint ensures that the least significant bits of the multiplication are correctly verified, which is sufficient to guard against overflow attacks that could potentially compromise the system's integrity.

Copy link
Copy Markdown
Contributor Author


How to use the Graphite Merge Queue

Add the label merge-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@josephjohnston josephjohnston marked this pull request as ready for review September 2, 2025 08:42
Copy link
Copy Markdown
Contributor Author

Not sure what to do about this snapshot issue

Comment thread crates/frontend/src/compiler/gate/imul.rs
Copy link
Copy Markdown
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

lgtm

@graphite-app
Copy link
Copy Markdown

graphite-app bot commented Sep 2, 2025

Merge activity

  • Sep 2, 3:42 PM UTC: josephjohnston added this pull request to the Graphite merge queue.
  • Sep 2, 3:43 PM UTC: CI is running for this pull request on a draft pull request (#917) due to your merge queue CI optimization settings.
  • Sep 2, 3:50 PM UTC: Merged by the Graphite merge queue via draft PR: #917.

graphite-app bot pushed a commit that referenced this pull request Sep 2, 2025
### TL;DR

Added an overflow check to the integer multiplication gate to prevent potential vulnerabilities.

### What changed?

Added an additional constraint to the `imul.rs` gate implementation that prevents overflow modulo 2^128-1. The implementation now verifies integer multiplication on the least significant bits by:

1. Extracting the least significant bits using shift-left operations
2. Adding an AND constraint to verify that `x[0] * y[0] = lo[0]`

This is accomplished by importing the `sll` function from the constraint builder module and adding the new AND constraint after the existing multiplication constraint.

### How to test?

Run the existing test suite to ensure that the integer multiplication gate still functions correctly with the added overflow protection. Consider adding specific tests that attempt to trigger overflow conditions to verify the new constraint is working as expected.

### Why make this change?

This change addresses a potential security vulnerability where overflow modulo 2^128-1 could occur in integer multiplication operations. The additional constraint ensures that the least significant bits of the multiplication are correctly verified, which is sufficient to guard against overflow attacks that could potentially compromise the system's integrity.
@graphite-app graphite-app bot closed this Sep 2, 2025
@graphite-app graphite-app bot deleted the intmul-overflow-check branch September 2, 2025 15:50
lockedloop pushed a commit that referenced this pull request Sep 8, 2025
### TL;DR

Added an overflow check to the integer multiplication gate to prevent potential vulnerabilities.

### What changed?

Added an additional constraint to the `imul.rs` gate implementation that prevents overflow modulo 2^128-1. The implementation now verifies integer multiplication on the least significant bits by:

1. Extracting the least significant bits using shift-left operations
2. Adding an AND constraint to verify that `x[0] * y[0] = lo[0]`

This is accomplished by importing the `sll` function from the constraint builder module and adding the new AND constraint after the existing multiplication constraint.

### How to test?

Run the existing test suite to ensure that the integer multiplication gate still functions correctly with the added overflow protection. Consider adding specific tests that attempt to trigger overflow conditions to verify the new constraint is working as expected.

### Why make this change?

This change addresses a potential security vulnerability where overflow modulo 2^128-1 could occur in integer multiplication operations. The additional constraint ensures that the least significant bits of the multiplication are correctly verified, which is sufficient to guard against overflow attacks that could potentially compromise the system's integrity.
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