Skip to content

Fix - Restrict address space of sysvar syscalls in SIMD-0219#7832

Merged
Lichtso merged 2 commits into
anza-xyz:masterfrom
Lichtso:fix/restrict_sysvar_syscall_address_space
Sep 9, 2025
Merged

Fix - Restrict address space of sysvar syscalls in SIMD-0219#7832
Lichtso merged 2 commits into
anza-xyz:masterfrom
Lichtso:fix/restrict_sysvar_syscall_address_space

Conversation

@Lichtso
Copy link
Copy Markdown

@Lichtso Lichtso commented Sep 2, 2025

Problem

ABI v1 aligns the account input region to 8 bytes. Direct mapping however uses the account data allocations which align to at least 16 bytes. Syscalls check the host alignment of translated pointers. This means that syscalls which require a 16 byte alignment suddenly pass the alignment check even if their virtual address is only divisible by 8 but not 16. Currently, only the sysvars syscall has a 16 byte alignment requirement. Thus, preventing that from accessing the account input section masks this behavior.

The SDK uses the stack as destination except for the generic get_sysvar() syscall, which could have the account input region as destination. Also, see section "Syscall parameters" in SIMD-0219.

Summary of Changes

Restricts the var_addr parameter of all sysvar syscalls when stricter_abi_and_runtime_constraints is active.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 2, 2025

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.0%. Comparing base (6b92df8) to head (382d12e).
⚠️ Report is 47 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7832   +/-   ##
=======================================
  Coverage    83.0%    83.0%           
=======================================
  Files         808      808           
  Lines      356264   356270    +6     
=======================================
+ Hits       296013   296044   +31     
+ Misses      60251    60226   -25     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Lichtso Lichtso requested a review from a team September 2, 2025 21:23
Comment thread syscalls/src/sysvar.rs
LucasSte
LucasSte previously approved these changes Sep 4, 2025
buffalojoec
buffalojoec previously approved these changes Sep 4, 2025
@Lichtso Lichtso dismissed stale reviews from buffalojoec and LucasSte via 382d12e September 5, 2025 08:34
Comment on lines +106 to +107
// Storing the result of get_sysvar() in the input region is not allowed
// because of the 16 byte alignment requirement of the EpochRewards sysvar.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't this comment say it's not allowed because it's strictly forbidden by the restrictions?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

And the reason why it is forbidden is the alignment requirement. So, the comment just skips the middle.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I thought the whole point was to disallow aliasing of account memory regions as stack space? SIMD-0219 doesn't list EpochRewards sysvar alignment as the motivation.

https://github.com/solana-foundation/solana-improvement-documents/pull/219/files#diff-d4c15e8a609c0d53c78a3db932bce5bb207dfce9d9a9874f2b2a5eedf56dda54R79

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No, there can not be aliasing of account regions and the stack region anyway because we don't do overlapping memory layouts. The SIMD says that these pointers must be in the stack or heap regions (thus implicitly can not be in the account regions).

Yes, I can add the alignment requirement as a motivation for forbidding sysvar syscalls from writing to the account regions in the SIMD. Currently it only mentions the restriction, not the reason for its existence.

@Lichtso Lichtso requested a review from buffalojoec September 8, 2025 12:33
@Lichtso Lichtso requested a review from LucasSte September 8, 2025 23:10
@Lichtso Lichtso merged commit 2581e3f into anza-xyz:master Sep 9, 2025
43 checks passed
@Lichtso Lichtso deleted the fix/restrict_sysvar_syscall_address_space branch September 9, 2025 06:47
@Lichtso Lichtso added the v3.0 label Sep 9, 2025
@mergify
Copy link
Copy Markdown

mergify Bot commented Sep 9, 2025

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify Bot pushed a commit that referenced this pull request Sep 9, 2025
* Restrict address space of sysvar syscalls as well (similar to CPI).

* Adds a test for the new restriction.

(cherry picked from commit 2581e3f)
Lichtso pushed a commit that referenced this pull request Sep 11, 2025
…ackport of #7832) (#7959)

* Fix - Restrict address space of sysvar syscalls in SIMD-0219 (#7832)

* Restrict address space of sysvar syscalls as well (similar to CPI).

* Adds a test for the new restriction.

* Rekeys stricter_abi_and_runtime_constraints.
@Lichtso Lichtso added this to SVM Jan 15, 2026
@github-project-automation github-project-automation Bot moved this to Done in SVM Jan 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants