Skip to content

fix(virtqueue): Fix usage of unaligned values in Vring<_>#1024

Closed
fogti wants to merge 1 commit into
hermit-os:mainfrom
fogti:fix-vring
Closed

fix(virtqueue): Fix usage of unaligned values in Vring<_>#1024
fogti wants to merge 1 commit into
hermit-os:mainfrom
fogti:fix-vring

Conversation

@fogti
Copy link
Copy Markdown
Contributor

@fogti fogti commented Jul 3, 2025

This attempts to tackle the first issue raised in #571.

Personally, I would prefer it if Vring wouldn't store a simple pointer in the first place, but at least a &mut [u8] slice, such that it can catch cases where the access would go out of bounds, but that would require messing with virtio.rs, which I didn't dare to touch yet.

@jounathaen
Copy link
Copy Markdown
Member

I much appreciate your effort, but src/virtqueue.rs is very likely to be thrown out completely in favour of #643. I just don't have the time currently to finalize that one.

So I can review this one, but in general I'd suggest ignoring the virtqueue parts for the moment.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 3, 2025

Codecov Report

❌ Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.88%. Comparing base (d246cc0) to head (921980b).
⚠️ Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
src/virtqueue.rs 0.00% 23 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1024      +/-   ##
==========================================
- Coverage   74.21%   73.88%   -0.33%     
==========================================
  Files          28       28              
  Lines        3141     3155      +14     
==========================================
  Hits         2331     2331              
- Misses        810      824      +14     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fogti
Copy link
Copy Markdown
Contributor Author

fogti commented Jul 3, 2025

Note that the new code has similar issues, albeit I'm surprised to see AtomicPtr there, given that the code doesn't look like the base addressed should change concurrently (and if they do, the code probably wouldn't do the right thing, as additional locking or such would be necessary to ensure consistency; in particular, the code for advance_index looks particularly concerning, given that it retrieves the pointer twice, but pretty much assumes it points to the same location)

@fogti
Copy link
Copy Markdown
Contributor Author

fogti commented Jul 3, 2025

Afaik there's no read_unaligned_volatile or such, so the code anyways needs to make sure that stuff (mem) is aligned, preferably to align_of::<u64>(), as that's the largest member of the Descriptor, which should make all relevant accesses aligned and is probably the easiest way.

@n0toose n0toose added refactor Code refactors feature/security Concerns security-related behavior, soundness, isolation or reliability. labels Aug 7, 2025
@fogti fogti closed this Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature/security Concerns security-related behavior, soundness, isolation or reliability. refactor Code refactors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants