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

Add kani proofs for IoVecBuffer types #4108

Merged
merged 4 commits into from
Sep 14, 2023

Conversation

bchalios
Copy link
Contributor

@bchalios bchalios commented Sep 13, 2023

Changes

Add kani proofs for IoVecBuffer::read_at() and IoVecBuffer::write_at(). The proofs check that we do not perform out-of-bounds memory accesses to/from buffers provided by the guest.

Reason

These methods play around with raw pointers and use unsafe primitives, so they're good candidate for Kani checking.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet contribution quality standards.

  • This functionality cannot be added in rust-vmm.

IoVecBuffer(Mut) are wrappers on top of guest buffers that not
necessarily contiguous in memory. The types provide methods for
reading/writing from/to these memory buffers.

Memory accesses to buffers provided by the guest is not safe (in terms
of Rust safety). For example, we cannot create slices on top of the
underlying addresses because we cannot guarantee that the guest will
behave in the way Rust expects it to do. As a result, IoVecBuffer(Mut)
describe the underlying memory using raw pointers and implement
read/write methods on them using unsafe primitives.

This commit adds proof-harnesses for these read/write methods.

Signed-off-by: Babis Chalios <[email protected]>
We have a IoVecSubregion type for describing sub-regions inside an
initial IoVecBuffer(Mut) type. This is useful type for de-duplicating
the code that finds a valid sub-region so that we can do something with
it later. However, the type was essentially duplicating (some of) the
state of the initial IoVecBuffer(Mut). The memory overhead should not be
significant, but the book-keeping overhead (creating yet another Vec)
is noticeable. Moreover, the interface was leading us to have some
nested loops. This was increasing significantly the time for kani
harnesses.

This commit drops the IoVecSubregion type and replaces the functionality
with a simple loop that performs the necessary operation on a valid
sub-range.

Signed-off-by: Babis Chalios <[email protected]>
@bchalios bchalios added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Sep 13, 2023
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02% ⚠️

Comparison is base (d6b309a) 82.35% compared to head (a5510d0) 82.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4108      +/-   ##
==========================================
- Coverage   82.35%   82.34%   -0.02%     
==========================================
  Files         225      225              
  Lines       28469    28445      -24     
==========================================
- Hits        23447    23424      -23     
+ Misses       5022     5021       -1     
Flag Coverage Δ
4.14-c7g.metal 77.76% <100.00%> (-0.02%) ⬇️
4.14-m5d.metal 79.64% <100.00%> (-0.03%) ⬇️
4.14-m6a.metal 78.76% <100.00%> (-0.02%) ⬇️
4.14-m6g.metal 77.76% <100.00%> (-0.02%) ⬇️
4.14-m6i.metal 79.64% <100.00%> (-0.02%) ⬇️
5.10-c7g.metal 80.68% <100.00%> (-0.02%) ⬇️
5.10-m5d.metal 82.33% <100.00%> (-0.01%) ⬇️
5.10-m6a.metal 81.54% <100.00%> (-0.02%) ⬇️
5.10-m6g.metal 80.68% <100.00%> (-0.02%) ⬇️
5.10-m6i.metal 82.31% <100.00%> (-0.01%) ⬇️
6.1-c7g.metal 80.68% <100.00%> (-0.02%) ⬇️
6.1-m5d.metal 82.33% <100.00%> (-0.01%) ⬇️
6.1-m6a.metal 81.54% <100.00%> (-0.02%) ⬇️
6.1-m6g.metal 80.68% <100.00%> (-0.02%) ⬇️
6.1-m6i.metal 82.31% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/vmm/src/devices/virtio/iovec.rs 98.64% <100.00%> (+0.39%) ⬆️

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

@bchalios bchalios requested a review from a team September 14, 2023 12:36
@bchalios bchalios merged commit eb38da7 into firecracker-microvm:main Sep 14, 2023
5 checks passed
@bchalios bchalios deleted the kani-iovec branch September 14, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants