ide: Draft changes to address the DMA base address range issue #2373

SrinivasShekar · 2025-11-10T22:10:15Z

Context: This fix is to address the issue reported by IDE fuzzer.

Changes made --

Added a check on the range of the DMA base address if it's within the guest memory range.
Validated the fix by running IDE fuzzer for about 20 mins with no reported errors and the added unit test cases
Dependent PR - guest_mem: Add support for guest memory access wrappers for PagedRanges #2444

vm/devices/storage/ide/src/lib.rs

github-actions · 2025-12-19T15:19:21Z

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

mattkur

Nice, thanks for doing this. A few comments, but the core logic looks right.

vm/devices/storage/ide/src/lib.rs

mattkur · 2026-01-17T05:20:18Z

vm/devices/storage/ide/src/lib.rs

+                    tracing::trace!(paged_range = ?PagedRange::new(0,  gpns.len() * PAGE_SIZE64 as usize , &gpns), "PagedRange of GPNs");
+                    tracing::trace!(gpns_vector = ?gpns, gpns_len = ?gpns.len(), "PagedRange values");


Not this line specifically, but it's when this raised to the level of my attention: do we really need this much tracing here? It seems like we're tracing every possible line.

These were added as part of my debugging, removed them now.

vm/devices/storage/ide/src/lib.rs

mattkur · 2026-01-17T05:29:38Z

vm/devices/storage/ide/src/lib.rs

    }

+    #[async_test]
+    async fn enlightened_cmd_test_invalid_dma_base() {


I assume this fails without your fix?

It doesn't fail rather it silently ignores the error when doing the DMA transfer.

if let Err(err) = r { tracelimit::error_ratelimited!( error = &err as &dyn std::error::Error, "dma transfer failed" ); }

Ref: https://github.com/microsoft/openvmm/blob/34cc2bba15a50185a5beefd83d2d95e1439847c8/vm/devices/storage/ide/src/drive/hard_drive.rs#L661C9-L666C10

Copilot

Pull request overview

This PR adds validation for IDE DMA base addresses against guest memory, using the new GuestMemory paged-range probing utilities, and introduces a negative test for out-of-bounds DMA descriptors.

Changes:

Import and use guestmem::ranges::PagedRange plus a page-size helper to model DMA buffers as paged ranges.
Extend Channel::perform_dma_memory_phase to validate each DMA descriptor’s buffer range via probe_gpn_readable_range / probe_gpn_writable_range, aborting the transfer and flagging an error on invalid ranges or arithmetic overflow.
Add an async test enlightened_cmd_test_invalid_dma_base to exercise an invalid DMA base address scenario for enlightened commands.

vm/devices/storage/ide/src/lib.rs

Copilot · 2026-01-27T13:04:58Z

vm/devices/storage/ide/src/lib.rs

+        /*
+        This is a negative test case where the DMA base address is invalid.
+        The test sets the DMA base address to an out-of-bounds memory
+        address of the guest range and expects the device to not read any data.
+        */


This negative test documents that it "expects the device to not read any data" for an invalid DMA base, but it only drives the enlightened command and waits for completion without asserting any resulting device state (e.g., DMA error flag, PRD exhaustion, or unchanged guest buffers). To fully cover the new DMA base validation behavior, please add explicit assertions that the transfer was rejected (for example by inspecting the bus master status or drive state) so that the test would fail if the device still attempted a DMA.

Copilot · 2026-01-27T13:04:58Z

vm/devices/storage/ide/src/lib.rs

+        device_select(&mut ide_device, &dev_path).await;
+        prep_ide_channel(&mut ide_device, DriveType::Hard, &dev_path);
+
+        // READ SECTORS - enlightened


The inline comment "// READ SECTORS - enlightened" does not match the command being invoked in this test, which uses WRITE_DMA_ALT; consider updating the comment to describe the write-DMA path being exercised to avoid confusion when reading the test.

Suggested change

// READ SECTORS - enlightened

// WRITE DMA (enlightened) with invalid DMA base

Copilot · 2026-01-27T13:04:59Z

vm/devices/storage/ide/src/lib.rs

+                let r = if let Some(end_gpa) = end_gpa {
+                    let start_gpn = Self::gpa_to_gpn(cur_desc_table_entry.mem_physical_base.into());
+                    let end_gpn = Self::gpa_to_gpn(end_gpa.into());
+                    let gpns: Vec<u64> = (start_gpn..end_gpn).collect();
+
+                    let paged_range = PagedRange::new(0, gpns.len() * PAGE_SIZE64 as usize, &gpns).unwrap();


The DMA range validation here builds gpns using start_gpn..end_gpn and then creates a PagedRange with offset 0 and length gpns.len() * PAGE_SIZE64, which means that when the DMA buffer lies entirely within a single page (start_gpn == end_gpn), gpns is empty and the probe becomes a no-op, so out-of-range single-page transfers will not be detected. In addition, the PagedRange being anchored at offset 0 on the first page does not reflect a non–page-aligned mem_physical_base, so the probed region can be a strict superset of the actual DMA buffer and require extra pages/bytes to be valid. To correctly validate the buffer, compute the first and last page numbers using a ceiling-style calculation (similar to storvsp/src/test_helpers.rs:217-221), derive the byte offset as mem_physical_base % PAGE_SIZE, and pass the actual transfer length (dma.transfer_bytes_left) into PagedRange::new, so that the probed range exactly matches the DMA buffer the device will access.

Suggested change

let r = if let Some(end_gpa) = end_gpa {

let start_gpn = Self::gpa_to_gpn(cur_desc_table_entry.mem_physical_base.into());

let end_gpn = Self::gpa_to_gpn(end_gpa.into());

let gpns: Vec<u64> = (start_gpn..end_gpn).collect();

let paged_range = PagedRange::new(0, gpns.len() * PAGE_SIZE64 as usize, &gpns).unwrap();

let r = if let Some(end_gpa_exclusive) = end_gpa {

let start_gpn =

Self::gpa_to_gpn(cur_desc_table_entry.mem_physical_base.into());

// end_gpa_exclusive is one past the last byte; subtract 1 to get the

// last byte included in the transfer, then compute its page number.

let last_gpa_inclusive = end_gpa_exclusive - 1;

let end_gpn = Self::gpa_to_gpn(last_gpa_inclusive.into());

let gpns: Vec<u64> = (start_gpn..=end_gpn).collect();

// The DMA buffer may start at a non–page-aligned address; reflect that

// in the probed range, and use the actual transfer length.

let offset =

(cur_desc_table_entry.mem_physical_base % PAGE_SIZE64) as usize;

let length = dma.transfer_bytes_left as usize;

let paged_range = PagedRange::new(offset, length, &gpns).unwrap();

github-actions · 2026-01-27T14:03:13Z

At least one Petri test failed.

draft changes to address the DMA base address range issue

e79718d

SrinivasShekar requested a review from mattkur November 10, 2025 22:10

Addressed review comments-Changed logic to use GPNs

5803c91

SrinivasShekar commented Nov 19, 2025

View reviewed changes

vm/devices/storage/ide/src/lib.rs Outdated Show resolved Hide resolved

SrinivasShekar added 2 commits December 19, 2025 20:06

Merge branch 'main' into ideFuzzerBug

4424b58

Updated guest_memory method

8bab01c

github-actions bot added the unsafe Related to unsafe code label Dec 19, 2025

mattkur reviewed Jan 17, 2026

View reviewed changes

Merge branch 'main' into ideFuzzerBug

2a0acdd

github-actions bot removed the unsafe Related to unsafe code label Jan 22, 2026

Removed debug logs and addressed comments

b544b5e

SrinivasShekar force-pushed the ideFuzzerBug branch from f509705 to b544b5e Compare January 27, 2026 05:40

SrinivasShekar added 2 commits January 27, 2026 05:44

Adding back the debug log

1b4799f

Merge branch 'main' into ideFuzzerBug

33f8979

SrinivasShekar marked this pull request as ready for review January 27, 2026 12:59

SrinivasShekar requested review from a team as code owners January 27, 2026 12:59

Copilot AI review requested due to automatic review settings January 27, 2026 12:59

SrinivasShekar self-assigned this Jan 27, 2026

Copilot started reviewing on behalf of SrinivasShekar January 27, 2026 13:00 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

		tracing::trace!(paged_range = ?PagedRange::new(0, gpns.len() * PAGE_SIZE64 as usize , &gpns), "PagedRange of GPNs");
		tracing::trace!(gpns_vector = ?gpns, gpns_len = ?gpns.len(), "PagedRange values");

	// READ SECTORS - enlightened
	// WRITE DMA (enlightened) with invalid DMA base

-                let r = if let Some(end_gpa) = end_gpa {
-                    let start_gpn = Self::gpa_to_gpn(cur_desc_table_entry.mem_physical_base.into());
-                    let end_gpn = Self::gpa_to_gpn(end_gpa.into());
-                    let gpns: Vec<u64> = (start_gpn..end_gpn).collect();
-                    let paged_range = PagedRange::new(0, gpns.len() * PAGE_SIZE64 as usize, &gpns).unwrap();
+                let r = if let Some(end_gpa_exclusive) = end_gpa {
+                    let start_gpn =
+                        Self::gpa_to_gpn(cur_desc_table_entry.mem_physical_base.into());
+                    // end_gpa_exclusive is one past the last byte; subtract 1 to get the
+                    // last byte included in the transfer, then compute its page number.
+                    let last_gpa_inclusive = end_gpa_exclusive - 1;
+                    let end_gpn = Self::gpa_to_gpn(last_gpa_inclusive.into());
+                    let gpns: Vec<u64> = (start_gpn..=end_gpn).collect();
+                    // The DMA buffer may start at a non–page-aligned address; reflect that
+                    // in the probed range, and use the actual transfer length.
+                    let offset =
+                        (cur_desc_table_entry.mem_physical_base % PAGE_SIZE64) as usize;
+                    let length = dma.transfer_bytes_left as usize;
+                    let paged_range = PagedRange::new(offset, length, &gpns).unwrap();

ide: Draft changes to address the DMA base address range issue #2373

Are you sure you want to change the base?

ide: Draft changes to address the DMA base address range issue #2373

Conversation

SrinivasShekar commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 19, 2025

Uh oh!

mattkur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattkur Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

SrinivasShekar Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattkur Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

SrinivasShekar Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SrinivasShekar commented Nov 10, 2025 •

edited

Loading