diff --git a/.github/workflows/copilot-instructions.md b/.github/workflows/copilot-instructions.md new file mode 100644 index 0000000..fab012a --- /dev/null +++ b/.github/workflows/copilot-instructions.md @@ -0,0 +1,565 @@ +## Copilot Code Generation Guidelines +When generating code, always prioritize these patterns: + + +### no_std and Memory Allocation Guidelines + +- This project is strictly **no_std** and **no_alloc** in production code +- All production paths must be allocation-free and compatible with bare-metal targets + +#### Production Code Requirements + +- **NEVER** use crates or features that require heap allocation in production code +- **DO NOT** use the `heapless` crate in production paths despite its name suggesting compatibility + - *Rationale: While heapless is no_std, it still uses stack allocation with unpredictable growth patterns* + - *Use fixed-size arrays and slices instead for predictable memory usage* +- **DO NOT** use any crate that depends on the `alloc` crate without feature gating +- **ALWAYS** use fixed-size arrays, slices, or static memory allocation +- **ALWAYS** design APIs to accept and return memory provided by the caller + +#### Memory Management in Production Code + +- Buffers must be pre-allocated by the caller and passed as slices +- Collection types must have fixed, compile-time sizes +- All data structures must have predictable, static memory footprints +- No dynamic memory growth patterns are allowed + +#### Test Code Exceptions + +- Test code (annotated with `#[cfg(test)]`) may use allocation if needed +- The `heapless` crate and other no_std compatible collections are permitted in tests +- Standard library features may be used in tests when the `std` feature is enabled +- Test helpers can use more ergonomic APIs that wouldn't be appropriate for production + +#### Example: Production vs. Test Code + +```rust +// Production code - strict no_alloc approach +pub fn process_data(data: &[u8], output: &mut [u8]) -> Result { + if output.len() < data.len() * 2 { + return Err(Error::BufferTooSmall); + } + + // Process data into output buffer + // ... + + Ok(processed_bytes) +} + +// Test code - can use more ergonomic approaches +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_process_data() { + // It's fine to use Vec in tests + let input = vec![1, 2, 3, 4]; + let mut output = vec![0; 16]; + + let result = process_data(&input, &mut output); + assert!(result.is_ok()); + // ... + } + + #[test] + fn test_with_heapless() { + // NOTE: Heapless is STRICTLY FORBIDDEN in production code (see guidelines above). + // It is permitted here ONLY because this is test code. + use heapless::Vec; + input.extend_from_slice(&[1, 2, 3]).unwrap(); + + let mut output = [0u8; 16]; + let result = process_data(&input, &mut output); + assert!(result.is_ok()); + // ... + } +} + + +### Error Handling +```rust +// ✅ GOOD: Always use Result/Option +fn parse_value(input: &str) -> Result { + input.parse().map_err(ParseError::InvalidFormat) +} + +// ❌ BAD: Never use unwrap/expect in production code +fn parse_value(input: &str) -> u32 { + input.parse().unwrap() // FORBIDDEN +} +``` + +### Array Access +```rust +// ✅ GOOD: Safe array access +if let Some(value) = array.get(index) { + process_value(*value); +} + +// ❌ BAD: Direct indexing can panic +let value = array[index]; // FORBIDDEN +``` + +### Hardware Register Access +```rust +// ✅ GOOD: Type-safe PAC usage +let reg_value = peripheral.register.read(); +peripheral.register.write(|w| w.field().set_bit()); + +// ❌ BAD: Raw pointer access +unsafe { + let reg_ptr = 0x4000_0000 as *mut u32; // FORBIDDEN + *reg_ptr = value; +} +``` + +### Constants and Magic Numbers +```rust +// ✅ GOOD: Named constants with documentation +/// ECDSA operation timeout in microseconds +/// Based on hardware specification section 4.2.1 +const ECDSA_TIMEOUT_US: u32 = 1000; + +// ❌ BAD: Magic numbers +thread::sleep(Duration::from_micros(1000)); // FORBIDDEN +``` + +## Code Review Checklist + +Use this checklist during code generation, local development, and code reviews: + +- [ ] Code is completely panic-free (no unwrap/expect/panic/indexing) **except in test code** +- [ ] All fallible operations return Result or Option **except in test code** +- [ ] Integer operations use checked/saturating/wrapping methods where needed +- [ ] Array/slice access uses get() or pattern matching, not direct indexing **except in test code** +- [ ] Error cases are well documented and handled appropriately +- [ ] Tests verify error handling paths, not just happy paths +- [ ] Unsafe code blocks are documented with safety comments **including test code** +- [ ] Hardware register access uses proper volatile operations **including test code** +- [ ] Cryptographic operations use constant-time implementations where applicable +- [ ] Magic numbers are replaced with named constants (register offsets, bit masks, timeouts) **except in test code** +- [ ] All constants include documentation explaining their purpose and source +- [ ] All register access is hidden behind type-safe register access layer (PAC or safe wrapper) **including test code** +- [ ] No direct pointer arithmetic or raw memory access for hardware registers **including test code** + + +## Test Code Exceptions + +Test code (annotated with `#[cfg(test)]` or in `tests/` modules) has relaxed requirements for developer ergonomics, but still maintains safety for hardware access: + +### ✅ Allowed in Test Code Only + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_parse_data() { + // ✅ OK: unwrap() in tests for simplicity + let result = parse_data("123").unwrap(); + assert_eq!(result, 123); + + // ✅ OK: Direct indexing in tests + let data = vec![1, 2, 3, 4]; + assert_eq!(data[0], 1); + + // ✅ OK: expect() with descriptive messages in tests + let config = Config::from_file("test.toml") + .expect("test config file should exist"); + + // ✅ OK: Magic numbers in test data + let test_input = [0x01, 0x02, 0x03, 0x04]; + + // ✅ OK: Vec allocation in tests + let mut buffer = vec![0u8; 1024]; + } +} +``` + +### ❌ Still Forbidden in Test Code + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_hardware_access() { + // ❌ FORBIDDEN: Direct register access even in tests + unsafe { + write_volatile(0x4000_0000 as *mut u32, 0x1234); + } + + // ✅ REQUIRED: Use safe wrappers even in tests + let mut device = TestEcdsaController::new(); + device.enable_engine().unwrap(); + } + + #[test] + fn test_crypto() { + // ❌ FORBIDDEN: Non-constant time crypto even in tests + let mut key = [0u8; 32]; + for (i, byte) in secret_key.iter().enumerate() { + if *byte == target_byte { // Timing attack possible + key[i] = *byte; + } + } + + // ✅ REQUIRED: Use constant-time operations + key.copy_from_slice(&secret_key); + } +} +``` + +### Test Helper Patterns + +```rust +#[cfg(test)] +mod test_helpers { + use super::*; + + // ✅ OK: Test-specific helper that wraps unsafe operations + pub struct TestEcdsaController { + inner: EcdsaController, + } + + impl TestEcdsaController { + pub fn new() -> Self { + // ✅ OK: unwrap() in test helper constructors + let secure = unsafe { ast1060_pac::Secure::steal() }; + Self { + inner: EcdsaController::new(secure).unwrap() + } + } + + pub fn enable_engine(&mut self) -> Result<(), EcdsaError> { + // Still use proper error handling in test helpers + self.inner.enable_engine() + } + + pub fn force_state_for_testing(&mut self, state: u32) { + // ✅ OK: Direct access for test setup + self.inner.test_register_override(0x7c, state).unwrap(); + } + } + + // ✅ OK: Test data with magic numbers + pub const TEST_P384_PRIVATE_KEY: &[u8] = &[ + 0x01, 0x02, 0x03, /* ... */ + ]; +} +``` + +### Rationale for Test Exceptions + +- **Ergonomics**: Tests should be easy to write and maintain +- **Clarity**: Test intent should be obvious without error handling noise +- **Speed**: Test development shouldn't be slowed by production safety requirements +- **Hardware Safety**: Even tests must not bypass hardware safety mechanisms +- **Crypto Safety**: Even tests must not introduce timing attack vulnerabilities + +## Quick Reference: Forbidden Patterns + +**Note**: Test code (marked with `#[cfg(test)]`) may use unwrap(), expect(), direct indexing, and magic numbers for ergonomics. Hardware and crypto safety rules still apply to tests. + +**Clarification**: Only panic-prone `unwrap` methods are forbidden. Safe methods like `unwrap_or()`, `unwrap_or_else()`, and `unwrap_or_default()` are acceptable in production code as they cannot panic. + +| Forbidden Pattern | Required Alternative | Test Exception | +|-------------------|----------------------|----------------| +| `value.unwrap()` | `match value { Some(v) => v, None => return Err(...) }` | ✅ OK in tests | +| `result.expect("msg")` | `match result { Ok(v) => v, Err(e) => return Err(e.into()) }` | ✅ OK in tests | +| `collection[index]` | `collection.get(index).ok_or(Error::OutOfBounds)?` | ✅ OK in tests | +| `a + b` (integers) | `a.checked_add(b).ok_or(Error::Overflow)?` | ⚠️ Use sparingly | +| `ptr.read()` | `ptr.read_volatile()` (for MMIO) | ❌ Never allowed | +| `status & (1 << 20)` | `status & STATUS_COMPLETE_BIT` | ✅ OK in tests | +| `0x1234` (magic numbers) | `const REGISTER_OFFSET: u32 = 0x1234;` | ✅ OK in tests | +| `retry = 1000` (magic timeouts) | `const MAX_RETRIES: u32 = 1000; // Hardware spec: max 5ms` | ✅ OK in tests | +| `(BASE + offset) as *mut u32` | Use PAC register structs instead of raw pointers | ❌ Never allowed | +| `write_volatile(ptr, val)` | `device.register().write(\|w\| w.field().bits(val))` | ❌ Never allowed | +| `read_volatile(ptr)` | `device.register().read().field().bits()` | ❌ Never allowed | + +## Security-Specific Guidelines + +- **Timing attacks**: Use constant-time comparisons for secrets (subtle crate) +- **Zeroization**: Use `zeroize` crate for sensitive data cleanup (keys, passwords, etc.) +- **Memory safety**: Ensure sensitive data is properly zeroized after use +- **Hardware abstraction**: All register access must go through HAL traits +- **Error information**: Don't leak sensitive data in error messages +- **Register access**: All hardware registers must use type-safe access layers (PAC or safe wrappers) + +## Type-Safe Register Access Requirements + +All hardware register access must be hidden behind type-safe abstractions. Direct pointer arithmetic and raw memory access are forbidden. + +### Peripheral Access Crate (PAC) Usage +```rust +// ❌ Forbidden - Direct register access +const CONTROL_REG: usize = 0x1000; +unsafe { write_volatile((BASE + CONTROL_REG) as *mut u32, value) }; +let status = unsafe { read_volatile((BASE + STATUS_REG) as *mut u32) }; + +// ✅ Required - PAC register access +device.control_register().write(|w| w + .enable().set_bit() + .mode().bits(0b10) + .timeout().bits(100) +); + +let status = device.status_register().read(); +if status.complete().bit_is_set() { + // Handle completion +} +``` + +### Safe Register Wrapper (When PAC Unavailable) +```rust +// When PAC doesn't cover all registers, create safe wrappers +pub struct EcdsaController { + // Use PAC for available registers + secure: Secure, + // Safe wrapper for missing registers + control: RegisterRW, + status: RegisterRO, +} + +impl EcdsaController { + pub fn enable_engine(&mut self) -> Result<(), EcdsaError> { + // Use PAC when available + self.secure.secure0b4().write(|w| w.sec_boot_ecceng_enbl().set_bit()); + + // Use safe wrapper for missing registers + self.control.write(EcdsaControlRegister::new() + .with_operation_mode(OperationMode::Verify) + .with_curve_type(CurveType::P384) + ); + + Ok(()) + } + + pub fn wait_for_completion(&self) -> Result { + loop { + let status = self.status.read(); + if status.operation_complete() { + return if status.operation_success() { + Ok(EcdsaResult::Success) + } else { + Err(EcdsaError::InvalidSignature) + }; + } + } + } +} + +// Type-safe register definitions +#[derive(Clone, Copy)] +pub struct EcdsaControlRegister(u32); + +impl EcdsaControlRegister { + pub fn new() -> Self { Self(0) } + + pub fn with_operation_mode(mut self, mode: OperationMode) -> Self { + self.0 = (self.0 & !0x3) | (mode as u32); + self + } + + pub fn with_curve_type(mut self, curve: CurveType) -> Self { + self.0 = (self.0 & !0xC) | ((curve as u32) << 2); + self + } +} + +#[repr(u32)] +pub enum OperationMode { + Verify = 0, + Sign = 1, + KeyGen = 2, +} +``` + +### Memory-Mapped I/O Safety +```rust +// ❌ Forbidden - Raw MMIO +unsafe fn write_sram(base: *mut u8, offset: usize, data: &[u8]) { + for (i, &byte) in data.iter().enumerate() { + write_volatile(base.add(offset + i), byte); + } +} + +// ✅ Required - Safe MMIO wrapper +pub struct SramController { + base: NonNull, + size: usize, +} + +impl SramController { + pub fn write_region(&mut self, region: SramRegion, data: &[u8]) -> Result<(), SramError> { + let offset = region.offset(); + let max_size = region.max_size(); + + if data.len() > max_size { + return Err(SramError::BufferTooLarge); + } + + if offset + data.len() > self.size { + return Err(SramError::OutOfBounds); + } + + unsafe { + for (i, &byte) in data.iter().enumerate() { + write_volatile(self.base.as_ptr().add(offset + i), byte); + } + } + + Ok(()) + } +} + +#[derive(Debug, Clone, Copy)] +pub enum SramRegion { + PublicKeyX, // Offset 0x2080, Max 48 bytes + PublicKeyY, // Offset 0x20C0, Max 48 bytes + SignatureR, // Offset 0x21C0, Max 48 bytes + SignatureS, // Offset 0x2200, Max 48 bytes + MessageHash, // Offset 0x2240, Max 48 bytes +} + +impl SramRegion { + fn offset(self) -> usize { + match self { + Self::PublicKeyX => 0x2080, + Self::PublicKeyY => 0x20C0, + Self::SignatureR => 0x21C0, + Self::SignatureS => 0x2200, + Self::MessageHash => 0x2240, + } + } + + fn max_size(self) -> usize { + match self { + Self::PublicKeyX | Self::PublicKeyY | + Self::SignatureR | Self::SignatureS | + Self::MessageHash => 48, // P384 size + } + } +} +``` + +## Magic Numbers and Constants + +All magic numbers must be replaced with well-documented named constants: + +### Hardware Register Access +```rust +// ❌ Forbidden - Direct register access +if status & (1 << 20) != 0 { + let value = ptr.add(0x7c); +} + +// ❌ Also Forbidden - Bypassing PAC +let base_ptr = (REGISTER_BASE + offset) as *mut u32; +unsafe { write_volatile(base_ptr, value) }; + +// ❌ Also Forbidden - Raw pointer arithmetic +fn sec_wr(&self, offset: usize, val: u32) { + unsafe { write_volatile(self.base.as_ptr().add(offset / 4), val) }; +} + +// ✅ Required - PAC register access +const STATUS_COMPLETE_BIT: u32 = 1 << 20; // Hardware manual section 4.3.2 + +let status = device.status_register().read(); +if status.operation_complete().bit_is_set() { + device.control_register().write(|w| w + .operation_mode().verify() + .curve_type().p384() + .enable().set_bit() + ); +} + +// ✅ Alternative - Safe wrapper when PAC unavailable +device.write_control_register(ControlValue::new() + .with_mode(OperationMode::Verify) + .with_curve(CurveType::P384) +); +``` + +### Timeouts and Retry Limits +```rust +// ❌ Forbidden +let mut retry = 1000; +delay_ns(5000); + +// ✅ Required +const MAX_OPERATION_RETRIES: u32 = 1000; // Hardware spec: max 5ms at 200MHz +const POLL_INTERVAL_NS: u32 = 5000; // Optimal polling interval per datasheet + +let mut retry = MAX_OPERATION_RETRIES; +delay_ns(POLL_INTERVAL_NS); +``` + +### Cryptographic Parameters +```rust +// ❌ Forbidden +let key_size = 32; +let signature_len = 64; + +// ✅ Required +const P256_SCALAR_BYTES: usize = 32; // NIST P-256 field element size +const P256_SIGNATURE_BYTES: usize = 64; // r + s components, 32 bytes each + +let key_size = P256_SCALAR_BYTES; +let signature_len = P256_SIGNATURE_BYTES; +``` + +### Documentation Requirements +Each constant must include: +- **Purpose**: What the value represents +- **Source**: Where the value comes from (datasheet section, RFC, etc.) +- **Units**: For timeouts, sizes, etc. +- **Constraints**: Valid ranges or special considerations + +## Enforcement + +To prevent violations of these guidelines, add these lint denials to your crate root: + +```rust +// In lib.rs or main.rs - Apply rules only to production code +#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::indexing_slicing))] +#![cfg_attr(not(test), warn(clippy::expect_used))] +``` + +For test modules, explicitly allow these patterns: + +```rust +// In test module files (e.g., src/tests/mod.rs) +#![allow(clippy::unwrap_used, clippy::indexing_slicing, clippy::expect_used)] +``` + +For individual test functions that need these patterns: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + #[allow(clippy::unwrap_used, clippy::indexing_slicing)] + fn test_with_ergonomic_patterns() { + let data = vec![1, 2, 3, 4]; + let result = parse_data(&data).unwrap(); + assert_eq!(result[0], expected_value); + } +} +``` + +These lints will: +- `clippy::unwrap_used` - Prevent all `.unwrap()` calls in production code +- `clippy::indexing_slicing` - Prevent direct array indexing like `array[index]` +- `clippy::expect_used` - Warn on `.expect()` usage (allowed in tests but discouraged in production) + +**Key Benefits**: +- Production code is enforced to be panic-free +- Test code retains ergonomic patterns for developer productivity +- Compile-time safety guarantees for production paths diff --git a/code-review.md b/code-review.md new file mode 100644 index 0000000..e823dc3 --- /dev/null +++ b/code-review.md @@ -0,0 +1,187 @@ +# Code Review: Copilot Safety Guidelines Violations + +## Overview +This document identifies violations of our Copilot safety guidelines in the codebase. Each violation includes the file, line number, specific issue, and recommended remediation. + +## Critical Violations + +### 1. Unwrap/Expect Usage (High Priority) + +#### Debug Module (`src/astdebug.rs`) +- **Lines 15, 19, 21, 22, 25, 26, 27, 30, 36, 41, 45, 52, 57, 62, 67**: Multiple `.unwrap()` calls + - **Risk**: Can panic during debug operations, compromising system stability + - **Fix**: Use proper error handling with `?` operator or return `Result` types + +#### Hash Module (`src/hash.rs`) +- **Line 166**: `usize::try_from(i).unwrap()` + - **Risk**: Can panic if conversion fails + - **Fix**: Use checked conversion with error propagation + +#### HMAC Module (`src/hmac.rs`) +- **Line 237**: `usize::try_from(digest_size).unwrap()` + - **Risk**: Can panic if digest size conversion fails + - **Fix**: Use validated conversion or const generics + +#### UART Module (`src/uart.rs`) +- **Line 149**: `u32::from(data)` (part of unsafe write) + - **Risk**: Combined with unsafe operation increases danger + - **Fix**: Validate data before conversion + +#### Main Module (`src/main.rs`) +- **Line 121**: `unsafe { Peripherals::steal() }` + - **Risk**: Unsafe peripheral access without guarantees + - **Fix**: Use singleton pattern or proper initialization + +#### HACE Controller (`src/hace_controller.rs`) +- **Line 493**: `u32::try_from(SPI_CALIB_LEN).unwrap()` + - **Risk**: Can panic if constant conversion fails + - **Fix**: Use compile-time validation or const assertion + +### 2. Array Indexing Without Bounds Checking + +#### Direct Indexing After Bounds Check (Critical Pattern) +- **`src/tests/functional/i2c_test.rs`**: Line 67 `self.buffer[..data.len()].copy_from_slice(data)` +- **`src/hace_controller.rs`**: Line 396 `self.ctx_mut().buffer[..key_len].copy_from_slice(key_bytes)` +- **`src/hace_controller.rs`**: Lines 431, 432, 436, 442, 443 - multiple buffer indexing patterns +- **`src/hash.rs`**: Lines 195, 227 - buffer slice indexing +- **`src/hmac.rs`**: Lines 223, 224, 244, 245 - crypto buffer indexing + - **Rationale**: Direct indexing after a bounds check is forbidden because future refactoring, off-by-one errors, or missed edge cases can still introduce panics. The only allowed pattern is to use `.get()`/`.get_mut()` which returns an `Option`, ensuring panic-free access and clear error propagation. See copilot-instructions.md section 'Array Access'. + - **Fix**: Replace with `.get_mut()` and `.get()` methods consistently: + ```rust + // WRONG (current pattern): + if data.len() <= self.buffer.len() { + self.buffer[..data.len()].copy_from_slice(data); // Still direct indexing! + } + + // CORRECT (required pattern): + if let Some(buf_slice) = self.buffer.get_mut(..data.len()) { + buf_slice.copy_from_slice(data); + } else { + return Err(Error::OutOfBounds); + } + ``` + +#### Register Access Indexing +- **`src/tests/functional/i2c_test.rs`**: Lines 56, 80, 87 - `self.buffer[address as usize]` + - **Risk**: Cast to usize without validation, then direct indexing + - **Fix**: Use `get()` with proper error handling: + ```rust + // WRONG: + if address as usize >= self.buffer.len() { return Err(...); } + self.buffer[address as usize] = data; // Still direct indexing! + + // CORRECT: + let Some(cell) = self.buffer.get_mut(address as usize) else { + return Err(DummyI2CError::OtherError); + }; + *cell = data; + ``` + +#### SPI Controller Validation Issues +- **`src/spi/spicontroller.rs`**: Lines 685, 758 - bounds check followed by potential indexing +- **`src/spi/fmccontroller.rs`**: Lines 645, 711 - similar pattern + - **Risk**: Bounds checking but potential unsafe access patterns elsewhere + - **Fix**: Audit all array access in these modules for consistent `.get()` usage + +### 3. Direct Volatile Register Access + +#### Main Module (`src/main.rs`) +- **Lines 114, 116**: `write_volatile(0x40001e24 as *mut u32, 0x12341234)` + - **Risk**: Direct memory manipulation bypassing type safety + - **Fix**: Use PAC register abstractions with proper field access + +#### ECDSA Module (`src/ecdsa.rs`) +- **Lines 103, 113, 123, 133, 143, 153**: Multiple `write_volatile`/`read_volatile` calls + - **Risk**: Unsafe register manipulation, potential memory corruption + - **Fix**: Replace with PAC-generated register access methods + +#### RSA Module (`src/rsa.rs`) +- **Lines 104, 114, 124, 134, 144, 154**: Similar volatile register access pattern + - **Risk**: Type-unsafe hardware interaction + - **Fix**: Use structured register access through PAC + +### 4. Magic Numbers and Constants + +#### SPI Controller (`src/spi/spicontroller.rs`) +- **Line 147**: `(reg_val & 0x0ff0) << 16` +- **Line 152**: `(reg_val & 0x0ff0_0000) | 0x000f_ffff` +- **Line 157**: `& 0xffff` and `& 0xffff_0000` bit masks +- **Lines 275, 277, 280, 298**: More hardcoded bit manipulation constants + - **Risk**: Code maintainability, unclear bit field purposes + - **Fix**: Define named constants with documentation explaining bit fields + +#### HACE Controller (`src/hace_controller.rs`) +- **Lines 9-13**: `0x0123_4567, 0x89ab_cdef, 0xfedc_ba98, 0x7654_3210, 0xf0e1_d2c3` +- **Lines 20-23**: `0xd89e_05c1, 0x07d5_7c36, 0x17dd_7030, 0x3959_0ef7` + - **Risk**: Unclear cryptographic constants, potential security issues + - **Fix**: Document constants with cryptographic standard references + +### 5. Unsafe Block Documentation Issues + +#### Extensive Unsafe Usage +**Files with undocumented unsafe blocks:** +- `src/astdebug.rs`: Lines 34, 50 (raw pointer slice creation) +- `src/spi/spicontroller.rs`: Lines 44, 46, 84, 125, 127, 134, 136, 218, 220, 266, 272, 296, 302, 371, 427, 482, 489, 493, 501, 529, 542 +- `src/hash.rs`: Line 249 (raw pointer slice creation) +- `src/hmac.rs`: Lines 236, 259 (digest pointer manipulation) +- `src/hace_controller.rs`: Lines 143, 351, 366, 384, 404 +- `src/pinctrl.rs`: Lines 1911, 1918 +- `src/watchdog.rs`: Lines 73, 89, 96, 118 +- `src/uart.rs`: Line 148 +- `src/main.rs`: Lines 112, 121, 129 +- `src/syscon.rs`: Lines 133, 138 + +**Risk**: Each unsafe block lacks proper safety documentation. See copilot-instructions.md section 'Unsafe Block Documentation'. +**Fix**: Add comprehensive safety invariant documentation explaining: + - Why unsafe is necessary + - What invariants are maintained + - How safety is guaranteed + +**Unsafe Block Documentation Template:** +```rust +// SAFETY: Explain why this unsafe block is required, what invariants are upheld, and how this is guaranteed to be safe. +unsafe { + // ...unsafe code... +} +``` + +## Medium Priority Issues + +### 6. Pattern Consistency +- Multiple files use inconsistent error handling patterns +- Some modules mix safe and unsafe approaches for similar operations +- **Fix**: Establish consistent safety patterns across modules + +### 7. Test Code Exceptions +Some violations may be acceptable in test context for ergonomics (see copilot-instructions.md section 'Test Code Exceptions'). However, hardware and cryptographic safety rules still apply in tests: +- Review `src/tests/` directory for legitimate test-only violations +- Ensure test violations don't leak into production code paths +- Even in tests, do not bypass hardware safety mechanisms or introduce timing attack vulnerabilities + +## Summary Statistics +- **Unwrap/Expect violations**: 25+ instances across 7 files +- **Array indexing without bounds checking**: 15+ instances in core modules +- **Direct volatile register access**: 20+ instances in crypto/main modules +- **Magic number constants**: 30+ hardcoded values needing documentation +- **Undocumented unsafe blocks**: 45+ instances requiring safety documentation + +## Severity Assessment +- **Critical**: Unwrap usage in crypto modules (can panic during security operations) +- **High**: Direct register access bypassing type safety +- **High**: Array indexing without bounds checking in crypto code +- **Medium**: Undocumented unsafe blocks +- **Low**: Magic numbers (maintainability issue) + +## Priority Remediation Plan +1. **Week 1**: Eliminate unwrap/expect in crypto modules (ecdsa, rsa, hash, hmac) +2. **Week 2**: Replace direct volatile access with PAC register abstractions +3. **Week 3**: Add bounds checking to all array indexing operations +4. **Week 4**: Document all unsafe blocks with safety invariants +5. **Week 5**: Define named constants to replace magic numbers + +## Notes +- Crypto modules require special attention due to security implications +- Embedded constraints may limit some remediation options +- Test code may have different acceptable risk profiles +- All fixes must maintain real-time performance characteristics +- Some unsafe blocks may be necessary for hardware interaction but must be documented diff --git a/src/lib.rs b/src/lib.rs index 5a5bd51..be167c2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,8 @@ // Licensed under the Apache-2.0 license +// Enforce Copilot coding guidelines - prevent panic-prone patterns in production code only +#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::indexing_slicing))] +#![cfg_attr(not(test), warn(clippy::expect_used))] #![cfg_attr(not(test), no_std)] pub mod astdebug; pub mod common; diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 04e9599..260d47d 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -1,3 +1,6 @@ // Licensed under the Apache-2.0 license +// Test code exceptions: Allow ergonomic patterns for test development +#![allow(clippy::unwrap_used, clippy::indexing_slicing, clippy::expect_used)] + pub mod functional;