Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements SD/MMC block device driver support by adding the SdMmcDriver struct that wraps the simple-sdmmc library and implements the BlockDriverOps trait.
- Added new SD/MMC driver module with block device operations (read, write, flush)
- Integrated the driver into the library with feature-gated compilation
- Added
simple-sdmmcdependency to support the hardware interface
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| axdriver_block/src/sdmmc.rs | New driver implementation providing SD/MMC block device operations |
| axdriver_block/src/lib.rs | Added feature-gated module declaration for sdmmc driver |
| axdriver_block/Cargo.toml | Added simple-sdmmc dependency and sdmmc feature flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
axdriver_block/src/sdmmc.rs
Outdated
| } | ||
|
|
||
| for (i, block) in blocks.iter_mut().enumerate() { | ||
| self.0.read_block(block_id as u32 + i as u32, block); |
There was a problem hiding this comment.
Multiple integer overflow risks: (1) casting block_id from u64 to u32 can silently truncate if block_id exceeds u32::MAX, and (2) the addition (block_id as u32 + i as u32) can overflow when processing multiple blocks. Both issues could lead to accessing incorrect block addresses. Consider validating block_id and the entire block range fit within u32 bounds, or use checked arithmetic.
axdriver_block/src/sdmmc.rs
Outdated
| } | ||
|
|
||
| for (i, block) in blocks.iter().enumerate() { | ||
| self.0.write_block(block_id as u32 + i as u32, block); |
There was a problem hiding this comment.
Multiple integer overflow risks: (1) casting block_id from u64 to u32 can silently truncate if block_id exceeds u32::MAX, and (2) the addition (block_id as u32 + i as u32) can overflow when processing multiple blocks. Both issues could lead to accessing incorrect block addresses. Consider validating block_id and the entire block range fit within u32 bounds, or use checked arithmetic.
| fn read_block(&mut self, block_id: u64, buf: &mut [u8]) -> DevResult { | ||
| let (blocks, remainder) = buf.as_chunks_mut::<{ SdMmc::BLOCK_SIZE }>(); | ||
|
|
||
| if !remainder.is_empty() { | ||
| return Err(DevError::InvalidParam); | ||
| } | ||
|
|
||
| for (i, block) in blocks.iter_mut().enumerate() { | ||
| self.0.read_block(block_id as u32 + i as u32, block); | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Missing bounds validation. The method should verify that block_id plus the number of blocks being read (blocks.len()) does not exceed num_blocks(). Without this check, out-of-bounds reads could occur, potentially causing hardware errors or reading invalid data.
| fn write_block(&mut self, block_id: u64, buf: &[u8]) -> DevResult { | ||
| let (blocks, remainder) = buf.as_chunks::<{ SdMmc::BLOCK_SIZE }>(); | ||
|
|
||
| if !remainder.is_empty() { | ||
| return Err(DevError::InvalidParam); | ||
| } | ||
|
|
||
| for (i, block) in blocks.iter().enumerate() { | ||
| self.0.write_block(block_id as u32 + i as u32, block); | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Missing bounds validation. The method should verify that block_id plus the number of blocks being written (blocks.len()) does not exceed num_blocks(). Without this check, out-of-bounds writes could occur, potentially causing hardware errors or data corruption.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
axdriver_block/src/sdmmc.rs
Outdated
| for (i, block) in blocks.iter().enumerate() { | ||
| self.0.write_block(block_id as u32 + i as u32, block); |
There was a problem hiding this comment.
The underlying write_block method is called without error handling. If the simple-sdmmc library can return errors from these operations, they should be propagated to the caller. Silently ignoring potential I/O errors could lead to data corruption or incorrect behavior.
axdriver_block/src/sdmmc.rs
Outdated
| // check that block_id + blocks.len() does not overflow u32 | ||
| if block_id.checked_add(blocks.len() as u32).is_none() { | ||
| return Err(DevError::InvalidParam); | ||
| } |
There was a problem hiding this comment.
The overflow check verifies that block_id + blocks.len() doesn't overflow u32, but it doesn't validate that the resulting block range is within the device's capacity. Consider adding a check like block_id + blocks.len() as u32 > self.0.num_blocks() as u32 to prevent out-of-bounds access.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
3 participants
SdMmcDriverinaxdriver_block/src/sdmmc.rs, providing block device operations (read, write, flush) using thesimple-sdmmclibrary.