feat: merge-train/avm #19496
Merged
feat: merge-train/avm #19496
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AztecBot
requested review from
IlyasRidhuan,
Maddiaa0,
fcarreiro and
jeanmon
as code owners
Collaborator
Author
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
Part of https://linear.app/aztec-labs/issue/AVM-163/rename-indirect-to-addressing-mode. PIL code will be done in a follow up.
`internal_call_return` forces `internal_call_id' = return_id`. However, during an error (when the call stack is empty) `return_id = 0` which forces `internal_call_id' = 0`. When there is another enqueued call after an error row (specifically this only happens when there is a teardown), there are constraints that enforce `internal_call_id' = 1`. These are contradictory constraints and the solution is to only enforce `internal_call_id' = return_id` if there are no errors. This didnt get detected via other fuzzers or tests because in the single enqueued call variant, the next row is defaulted to zero which enables the buggy relation to pass.
Add debuglog to fuzzer and refactor env getter so we randomly generate bad env vars ids
## Summary - Fix ECC tracegen `add_predicate` bug for points with same y-coordinate but different x-coordinate - Add constraining test for this edge case using real Grumpkin points ## Description The ECC tracegen had a bug where `add_predicate` was set to `(!x_match && !y_match)`, requiring **both** coordinates to differ. However, the PIL constraint `sel = double_op + add_op + INFINITY_PRED` requires `add_op = 1` whenever x-coordinates differ (regardless of y-coordinates). This caused constraint failures when adding two points with different x but same y: - `x_match = 0`, `y_match = 1` - `double_op = 0` (not doubling) - `INFINITY_PRED = x_match * (1 - y_match) = 0` - PIL requires: `add_op = sel - double_op - INFINITY_PRED = 1` - Bug produced: `add_op = 0` → constraint `sel = 0 + 0 + 0 = 0 ≠ 1` fails ### Why this edge case exists On Grumpkin (y² = x³ - 17), two distinct points can share the same y-coordinate due to cube roots of unity in BN254 Fr. If `(x, y)` is on the curve, then `(ω·x, y)` is also valid since `ω³ = 1`. ### Fix ```cpp // Before (BUG): bool add_predicate = (!x_match && !y_match); // After: bool add_predicate = !x_match; ``` ### Test plan - New constraining test EccAddSameYDifferentX using simulation → tracegen → PIL verification - Verified test fails without fix, passes with fix - All 26 existing ECC tests pass --- 🤖 Generated with https://claude.ai/code Co-Authored-By: Claude noreply@anthropic.com
BB_ASSERT was deemed acceptable since this happens only per-call and not on every instruction.
this would have allowed arbitrary public data write injections This PR adds a constraint-level test and an integration test to confirm the fix
## Summary Adds defensive constraints to `bc_hashing.pil` to prevent ghost row injection attacks on the `#[GET_PACKED_FIELD_1]` and `#[GET_PACKED_FIELD_2]` permutations. **The vulnerability pattern:** When `sel=0` (gadget inactive), the sub-selectors `sel_not_padding_1` and `sel_not_padding_2` were only boolean-constrained, not forced to 0. A malicious prover could set these to 1 on ghost rows to fire the permutations into `bc_decomposition`. **The fix:** Add constraints that force these selectors to 0 when `sel=0`: ```pil #[SEL_NOT_PADDING_1_REQUIRES_SEL] sel_not_padding_1 * (1 - sel) = 0; #[SEL_NOT_PADDING_2_REQUIRES_SEL] sel_not_padding_2 * (1 - sel) = 0; ``` Note: This is a defensive fix. The destination trace (bc_decomposition) already has sel_packed * (1 - sel) = 0 which would block ghost destination rows, making the attack non-exploitable in practice. Test Plan - Added NegativeGhostRowInjectionBlocked test verifying the fix
avm_fuzzer_prover_fuzzer crashed on MOV opcode on MacOS only <details><summary>Crash file base64</summary> h65pbnB1dF9wcm9ncmFtc5GEsmluc3RydWN0aW9uX2Jsb2Nrc5GVkgSDqXZhbHVlX3RhZ8QIBAAAAAAAAACucmVzdWx0X2FkZHJlc3OEp2FkZHJlc3PMk7Rwb2ludGVyX2FkZHJlc3Nfc2VlZM14uLBiYXNlX29mZnNldF9zZWVkzozLNdqkbW9kZcQBAKV2YWx1Zc52DTAOkgSDqXZhbHVlX3RhZ8QIBAAAAAAAAACucmVzdWx0X2FkZHJlc3OEp2FkZHJlc3PM8LRwb2ludGVyX2FkZHJlc3Nfc2VlZM07trBiYXNlX29mZnNldF9zZWVkzi2TU+KkbW9kZcQBAKV2YWx1Zc4Yh1NXkguDqWFfYWRkcmVzc5IBhKdhZGRyZXNzzJO0cG9pbnRlcl9hZGRyZXNzX3NlZWTNeLiwYmFzZV9vZmZzZXRfc2VlZM6MyzXapG1vZGXEAQCpYl9hZGRyZXNzkgGEp2FkZHJlc3PM8LRwb2ludGVyX2FkZHJlc3Nfc2VlZM07trBiYXNlX29mZnNldF9zZWVkzi2TU+KkbW9kZcQBAK5yZXN1bHRfYWRkcmVzc4SnYWRkcmVzc86i7sr0tHBvaW50ZXJfYWRkcmVzc19zZWVkzePmsGJhc2Vfb2Zmc2V0X3NlZWTOLE08F6Rtb2RlxAEDkgKDqXZhbHVlX3RhZ8QIBgAAAAAAAACucmVzdWx0X2FkZHJlc3OEp2FkZHJlc3POTsCswrRwb2ludGVyX2FkZHJlc3Nfc2VlZM2D/7BiYXNlX29mZnNldF9zZWVkzlxxEwekbW9kZcQBA6V2YWx1ZUySCIOpdmFsdWVfdGFnxAgGAAAAAAAAAKtzcmNfYWRkcmVzc5IAhaN0YWfECAYAAAAAAAAApWluZGV4zpMUkyC0cG9pbnRlcl9hZGRyZXNzX3NlZWTNSoGwYmFzZV9vZmZzZXRfc2VlZM7ZPYDCpG1vZGXEAQGucmVzdWx0X2FkZHJlc3OEp2FkZHJlc3POfRJOerRwb2ludGVyX2FkZHJlc3Nfc2VlZM0HobBiYXNlX29mZnNldF9zZWVkzi6qFi2kbW9kZcQBArBjZmdfaW5zdHJ1Y3Rpb25zkZIAgbVpbnN0cnVjdGlvbl9ibG9ja19pZHgAqGNhbGxkYXRhkcQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACucmV0dXJuX29wdGlvbnODq3JldHVybl9zaXplALByZXR1cm5fdmFsdWVfdGFnxAgAAAAAAAAAALlyZXR1cm5fdmFsdWVfb2Zmc2V0X2luZGV4ALBjb250cmFjdF9jbGFzc2VzkYSiaWTEIDAuCPg2SqK95QIGhYPLHurcSeJ1kQ7VXV5ictRay29wrGFydGlmYWN0SGFzaMQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAC0cHJpdmF0ZUZ1bmN0aW9uc1Jvb3TEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAArnBhY2tlZEJ5dGVjb2RlxGMpAACTBHYNMA4pAADwBBiHU1cpAAAABAAA488pAgAXBKLuyvQEMJPwFykAAAAEAACD+CkCAAcETsCswicDBwZMKQAAgQROwKzCKQAAAAR9Ek5NLQmBLSgAAAUEAAE7AAAFAACyY29udHJhY3RfaW5zdGFuY2VzkYakc2FsdMQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACoZGVwbG95ZXLEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABktmN1cnJlbnRDb250cmFjdENsYXNzSWTEIDAuCPg2SqK95QIGhYPLHurcSeJ1kQ7VXV5ictRay29wt29yaWdpbmFsQ29udHJhY3RDbGFzc0lkxCAwLgj4NkqiveUCBoWDyx7q3EnidZEO1V1eYnLUWstvcLJpbml0aWFsaXphdGlvbkhhc2jEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAqnB1YmxpY0tleXOEuG1hc3Rlck51bGxpZmllclB1YmxpY0tleYKheMQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAChecQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAC+bWFzdGVySW5jb21pbmdWaWV3aW5nUHVibGljS2V5gqF4xCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAaF5xCAAAAAAAAAAAs8TXnUGpF1jLScNRfEYEpSDP8SNgj8nLL5tYXN0ZXJPdXRnb2luZ1ZpZXdpbmdQdWJsaWNLZXmCoXjEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAoXnEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAtm1hc3RlclRhZ2dpbmdQdWJsaWNLZXmCoXjEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAoXnEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAsmNvbnRyYWN0X2FkZHJlc3Nlc5HEIAYKo3MGHhi9go9y0MVV68JJX+cMrm3MFyWoOX4VtTqlonR4jKRoYXNoqjB4ZGVhZGJlZWarZ2FzU2V0dGluZ3OEqWdhc0xpbWl0c4KlbDJHYXPOAA9CQKVkYUdhc84AD0JAsXRlYXJkb3duR2FzTGltaXRzgqVsMkdhcwClZGFHYXMArW1heEZlZXNQZXJHYXOCq2ZlZVBlckRhR2FzAatmZWVQZXJMMkdhcwG1bWF4UHJpb3JpdHlGZWVzUGVyR2FzgqtmZWVQZXJEYUdhcwCrZmVlUGVyTDJHYXMAsGVmZmVjdGl2ZUdhc0ZlZXOCq2ZlZVBlckRhR2FzAatmZWVQZXJMMkdhcwHZI25vblJldmVydGlibGVDb250cmFjdERlcGxveW1lbnREYXRhgrFjb250cmFjdENsYXNzTG9nc5CrcHJpdmF0ZUxvZ3OQ2SByZXZlcnRpYmxlQ29udHJhY3REZXBsb3ltZW50RGF0YYKxY29udHJhY3RDbGFzc0xvZ3OQq3ByaXZhdGVMb2dzkLxub25SZXZlcnRpYmxlQWNjdW11bGF0ZWREYXRhg6pub3RlSGFzaGVzkKpudWxsaWZpZXJzkcQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAN6tvu+ubDJUb0wxTWVzc2FnZXOQuXJldmVydGlibGVBY2N1bXVsYXRlZERhdGGDqm5vdGVIYXNoZXOQqm51bGxpZmllcnOQrmwyVG9MMU1lc3NhZ2VzkLJzZXR1cEVucXVldWVkQ2FsbHOQtWFwcExvZ2ljRW5xdWV1ZWRDYWxsc5GCp3JlcXVlc3SEqW1zZ1NlbmRlcsQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAGSvY29udHJhY3RBZGRyZXNzxCAGCqNzBh4YvYKPctDFVevCSV/nDK5tzBclqDl+FbU6paxpc1N0YXRpY0NhbGzCrGNhbGxkYXRhSGFzaMQgDRHwusEykJ+Kd6Rb4yoN+tiXAtlX8/GWO3kFRb4/lmyoY2FsbGRhdGGQtHRlYXJkb3duRW5xdWV1ZWRDYWxswLBnYXNVc2VkQnlQcml2YXRlgqVsMkdhcwClZGFHYXMAqGZlZVBheWVyxCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAZLBnbG9iYWxfdmFyaWFibGVziKdjaGFpbklkxCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAad2ZXJzaW9uxCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAatibG9ja051bWJlcgGqc2xvdE51bWJlcsQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAGpdGltZXN0YW1wzgAPQkCoY29pbmJhc2XEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAArGZlZVJlY2lwaWVudMQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACnZ2FzRmVlc4KrZmVlUGVyRGFHYXMBq2ZlZVBlckwyR2FzAbJwcm90b2NvbF9jb250cmFjdHOBsGRlcml2ZWRBZGRyZXNzZXObxCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAxCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAxCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADEIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAxCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMQgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=</details>
1. Improved type conversion in the dispatcher 1. Exception handling is ok 1. Input and output handling: sadly required 1. Checkpoint validation: made it a BB_ASSERT Note: MemoryTag can be used directly because it's validated during instruction fetching. For EnvironmentVariable, we need to use uint8_t until we do the proper validation.
## Summary Fixes ghost row injection vulnerabilities in three PIL traces where sub-selectors firing permutations were not constrained to be 0 when the main selector was inactive. **The vulnerability pattern:** When `sel=0` (gadget inactive), sub-selectors like `is_write_memory_value` could still be set to 1 by a malicious prover. This would fire permutations to memory, allowing arbitrary memory writes without going through legitimate execution paths. **The fix pattern:** Add constraints of the form `sub_selector * (1 - sel) = 0` which forces sub-selectors to 0 when `sel=0`. ## Changes ### PIL Fixes | File | Constraint Added | |------|------------------| | `emit_unencrypted_log.pil` | `#[IS_WRITE_MEMORY_VALUE_REQUIRES_SEL]` | | `get_contract_instance.pil` | `#[IS_VALID_WRITES_IN_BOUNDS_REQUIRES_SEL]` | | `to_radix_mem.pil` | `#[SEL_SHOULD_WRITE_MEM_REQUIRES_SEL]` | ### Test Updates - Updated ghost row injection tests to verify fixes block the attacks - Tests use `EXPECT_THROW_WITH_MESSAGE` to confirm constraints fail as expected - Removed debug output (cout statements) - Renamed tests from `NegativeFullAttackWithAllTraces` to `NegativeGhostRowInjectionBlocked`
Stepan found a bug where we were using a MemoryValue after a set in `mov`. This was already fixed. This PR defensively copies some MemoryValues so that the underlying gadgets (e.g. ALU and bitwise) do not need to make crazy assumptions about the MemoryValue references they get as parameters. I expect that this shouldn't have a noticeable performance impact but we'll see in the benchmarks.
fcarreiro
approved these changes
Jan 12, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BEGIN_COMMIT_OVERRIDE
feat(avm security): add static check for isolated/unused columns (#19489)
feat(avm): use noop calldata hasher in fast sim (#19495)
chore(avm): rename indirect -> addressing mode (#19491)
chore(avm): small cursor optimizations
chore(avm)!: rename indirect -> addressing mode (PIL) (#19493)
fix(avm): constraint when unwinding empty call stack (#19485)
feat(avm): Fuzz debug log and refactor env getter (#19494)
fix!: ecc add predicate completeness bug (#19471)
chore(avm): callstackmetadatacollector clarifications (#19490)
chore: sanity assert in execution for bytecode id (#19486)
fix!: sstore allowed injection of malicious write rows (#19470)
fix!: defensive ghost row constraints in bc_hashing pil (#19481)
fix(avm): fix execution::mov for mac? (#19507)
chore(avm)!: resolve execution TODOs (#19501)
fix!: multiple traces had ghost row injection vulnerabilities (#19480)
fix(avm): defensively copy MemoryValues (#19512)
feat: align TS and BB log levels (#19518)
END_COMMIT_OVERRIDE