Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly check memory alignment based on size of operand #106

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

mininny
Copy link
Collaborator

@mininny mininny commented Dec 15, 2024

Description

The RISC-V specifications indicate the following in section "14.2. "Zalrsc" Extension for Load-Reserved/Store-Conditional Instructions".

For LR and SC, the Zalrsc extension requires that the address held in rs1 be naturally aligned to the
size of the operand
(i.e., eight-byte aligned for doublewords and four-byte aligned for words). If the
address is not naturally aligned, an address-misaligned exception or an access-fault exception will be
generated.

However, the current implementation only checks that the address is aligned on 4 bytes. This check is not accurate for LR.D and SC.D instructions which use 8-byte size.


This PR ensures that mod(addr, size) == 0 instead of the current logical and check.

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 61.89%. Comparing base (8ff9318) to head (b7dc265).

Files with missing lines Patch % Lines
rvgo/fast/vm.go 0.00% 0 Missing and 1 partial ⚠️
rvgo/slow/vm.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #106   +/-   ##
=======================================
  Coverage   61.89%   61.89%           
=======================================
  Files          27       27           
  Lines        4091     4091           
=======================================
  Hits         2532     2532           
  Misses       1427     1427           
  Partials      132      132           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mininny mininny force-pushed the feature/mininny/audit-19 branch from bc7160a to b7dc265 Compare December 15, 2024 09:00
@BlocksOnAChain BlocksOnAChain added the Audit finding grouping for our audit findings label Dec 16, 2024
@mininny mininny requested a review from refcell January 8, 2025 21:35
@BlocksOnAChain
Copy link

@mininny we can merge now since it was approved by Refcell and approved by the auditors.

@mininny mininny added this pull request to the merge queue Jan 15, 2025
Merged via the queue into master with commit 1ea4ef9 Jan 15, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Audit finding grouping for our audit findings
Development

Successfully merging this pull request may close these issues.

4 participants