guest_mem: Add support for guest memory access wrappers for PagedRanges #2444
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull Request Overview
This PR adds guest memory access validation wrappers for PagedRange objects and uses them in the IDE device to validate DMA operations. The implementation adds two new methods to GuestMemory (probe_gpn_readable_range and probe_gpn_writable_range) and integrates them into the IDE DMA path to check memory accessibility before transfers.
Key Changes:
- New
probe_gpn_readable_rangeandprobe_gpn_writable_rangemethods in GuestMemory API - DMA memory validation in IDE device before descriptor processing
- Test case for invalid DMA base address handling
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| vm/vmcore/guestmem/src/lib.rs | Adds two new probe methods for checking readable/writable access to PagedRange memory regions |
| vm/devices/storage/ide/src/lib.rs | Integrates new probe methods into DMA validation logic, adds extensive debug tracing, includes test for invalid DMA addresses, and has several implementation bugs |
| /// Check if a given PagedRange is readable or not. | ||
| pub fn probe_gpn_readable_range(&self, range: &PagedRange<'_>) -> Result<(), GuestMemoryError> { |
There was a problem hiding this comment.
[nitpick] The documentation could be more descriptive. Consider expanding it to clarify what "readable" means in this context and what errors might be returned. For example:
/// Checks if all pages in the given PagedRange are readable.
///
/// This probes each page in the range by attempting to read a single byte.
/// Returns an error if any page in the range is not accessible or is outside
/// the guest's physical address space.
///
/// # Errors
/// Returns `GuestMemoryError` if any page is inaccessible or invalid.
pub fn probe_gpn_readable_range(&self, range: &PagedRange<'_>) -> Result<(), GuestMemoryError>The same applies to probe_gpn_writable_range.
| }) | ||
| } | ||
|
|
||
| /// Check if a given PagedRange is writable or not. |
There was a problem hiding this comment.
[nitpick] Consider adding similar documentation to probe_gpn_writable_range as suggested for probe_gpn_readable_range. The documentation should clarify that this checks writability by attempting a compare-exchange operation.
| /// Check if a given PagedRange is writable or not. | |
| /// Check if a given PagedRange is writable or not. | |
| /// | |
| /// This checks writability by attempting a compare-exchange operation at each address in the range. | |
| /// If the compare-exchange succeeds, the page is considered writable; otherwise, an error is returned. |
SrinivasShekar
force-pushed
the
guestMemAccessWrappers
branch
from
caff9a4 to
ed9df2f
Compare
|
What's the context for this change? |
TFTR John. |
SrinivasShekar
force-pushed
the
guestMemAccessWrappers
branch
from
87e8a7e to
258e514
Compare
| /// Check if a given PagedRange is readable or not. | ||
| pub fn probe_gpn_readable_range(&self, range: &PagedRange<'_>) -> Result<(), GuestMemoryError> { | ||
| self.op_range(GuestMemoryOperation::Probe, range, move |addr, _r| { | ||
| self.read_plain_inner(addr) |
There was a problem hiding this comment.
Missing type parameter for read_plain_inner. This should be self.read_plain_inner::<u8>(addr) to match the pattern used in probe_gpns (line 1895) and probe_page_for_lock (line 1864). Without the explicit type parameter, the Rust compiler won't be able to infer the type.
| self.read_plain_inner(addr) | |
| self.read_plain_inner::<u8>(addr) |
| self.compare_exchange(addr, 0u8, 0) | ||
| .map(|_| ()) | ||
| .map_err(|err| GuestMemoryBackingError::other(addr, err)) | ||
| }) |
There was a problem hiding this comment.
The approach of calling compare_exchange here creates nested error wrapping, since compare_exchange already wraps errors with with_op, and op_range also does the same. This results in converting GuestMemoryError back to GuestMemoryBackingError via GuestMemoryBackingError::other, which loses error context. Consider creating a simpler internal write probe method similar to read_plain_inner that directly returns Result<(), GuestMemoryBackingError>, or use run_on_mapping with AccessType::Write directly to match the pattern used in read_plain_inner.
There was a problem hiding this comment.
@SrinivasShekar, what do you think of this feedback?
| pub fn probe_gpn_writable_range(&self, range: &PagedRange<'_>) -> Result<(), GuestMemoryError> { | ||
| self.op_range(GuestMemoryOperation::Probe, range, move |addr, _r| { | ||
| self.compare_exchange(addr, 0u8, 0) | ||
| .map(|_| ()) | ||
| .map_err(|err| GuestMemoryBackingError::other(addr, err)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The new probe_gpn_writable_range function lacks test coverage. Consider adding tests that verify: (1) probing a writable PagedRange succeeds, (2) probing an unmapped PagedRange fails, (3) probing a read-only PagedRange fails appropriately, and (4) probing a PagedRange that spans multiple pages with mixed write accessibility behaves correctly. The FaultingMapping test infrastructure (line 2631) would be particularly useful for testing write access scenarios.
mattkur
left a comment
mattkur
left a comment
There was a problem hiding this comment.
I think this looks good, but please take a look at the CoPilot feedback to double check we're not missing something.
3 participants
This change adds support for checking whether the given pagedRange is readable or writable.
Background context:
For IDE DMA processing, we would need to check if the buffer that is going to be transferred, is within the guest address space. We would need to probe the entire buffer range to check if its readable or writable depending on the DMA type. These utilities would help in doing the same. Ref: https://github.com/microsoft/openvmm/pull/2373/files (still in draft)