Repository: chipsalliance/caliptra-sw (rust)

Issue:
Mitigate glitching of calls within Trng generate
From Nick Galloway (gallowayn@google.com)

An additional edge case that could be glitched in the early boot sequence before the first random delay happens:

* `rom_entry` calls `caliptra_cfi_lib::cfi_counter::CfiCounter::reset`
  * `reset` calls `caliptra_drivers::trng::Trng::generate`
    * `generate` uses `ureg::read_volatile_slice` and `compiler_builtins::mem::memcpy::memcpy`

Either of which could be attacked with a glitch to trigger a buffer overflow, such as with an instruction skip or bit flip

Pull request description:
Replace memcpy and read_volatile_slice calls from TRNG with unrolled equivalents
Calling memcpy-like functions before the CFI defenses are initialized is
dangerous, as a glitch could lead to an out-of-bounds write. Fixes #922 

Please review each commit independently.

## Specialize RealMmioMut::read_volatile_array() for risc-v.

These specializations unroll register array reads for 12-word and
16-word reads; they were previously used for RealMmio, but I forgot to
add them for RealMmioMut.

This improves code size, and improves our glitch resistence (since there
is no loop conditional to glitch).

Was:

```
    6380:       00440513                add     a0,s0,4
    6384:       300304b7                lui     s1,0x30030
    6388:       07848593                add     a1,s1,120 # 30030078 <DCCM_SIZE+0x30010078>
    638c:       4631                    li      a2,12
    638e:       3609                    jal     5e90 <ureg::read_volatile_slice>
```

Now:

```
    637a:       00440513                add     a0,s0,4
    637e:       300304b7                lui     s1,0x30030
    6382:       07848593                add     a1,s1,120 # 30030078 <DCCM_SIZE+0x30010078>
    6386:       66f010ef                jal     81f4 <ureg::opt_riscv::copy_12words>
```

##  CSRNG driver: Remove implicit memcpy from generate().

By unrolling the loop into uninitialized memory, we convince the optimizer
to get rid of the memcpy call.

Was:

```
    63d6:       188020ef                jal     855e <memcpy>
    63da:       4501                    li      a0,0
    63dc:       c044                    sw      s1,4(s0)
    63de:       a801                    j       63ee <.LBB60_21>

<snip>

000063ee <.LBB60_21>:
    63ee:       c008                    sw      a0,0(s0)
    63f0:       50f2                    lw      ra,60(sp)
    63f2:       5462                    lw      s0,56(sp)
    63f4:       54d2                    lw      s1,52(sp)
    63f6:       6121                    add     sp,sp,64
    63f8:       8082                    ret
```

Now:

```
    63d4:       5214                    lw      a3,32(a2)
    63d6:       8a85                    and     a3,a3,1
    63d8:       def5                    beqz    a3,63d4 <.LBB60_18>
    63da:       4601                    li      a2,0
    63dc:       200026b7                lui     a3,0x20002
    63e0:       52c4                    lw      s1,36(a3)
    63e2:       52d8                    lw      a4,36(a3)
    63e4:       52dc                    lw      a5,36(a3)
    63e6:       52d4                    lw      a3,36(a3)
    63e8:       01142423                sw      a7,8(s0)
    63ec:       00542623                sw      t0,12(s0)
    63f0:       00642823                sw      t1,16(s0)
    63f4:       00742a23                sw      t2,20(s0)
    63f8:       01c42c23                sw      t3,24(s0)
    63fc:       cc48                    sw      a0,28(s0)
    63fe:       d00c                    sw      a1,32(s0)
    6400:       d044                    sw      s1,36(s0)
    6402:       d418                    sw      a4,40(s0)
    6404:       d45c                    sw      a5,44(s0)
    6406:       d814                    sw      a3,48(s0)
    6408:       01042223                sw      a6,4(s0)
    640c:       a801                    j       641c <.LBB60_23>
    ```

Patch:
diff --git a/FROZEN_IMAGES.sha384sum b/FROZEN_IMAGES.sha384sum
index 75d3a0df..1903485b 100644
--- a/FROZEN_IMAGES.sha384sum
+++ b/FROZEN_IMAGES.sha384sum
@@ -1,3 +1,3 @@
 # WARNING: Do not update this file without the approval of the Caliptra TAC
-6c8651061bc86da0ab38a62631282112c30d78980ba9ae37f3148b264e520bc200ccf03a2b76f903a492746745ee0195  caliptra-rom-no-log.bin
-6a5ed1989df5f11820534fbdebf71f6090c5a2784140506d31a008a347c7281eaf3db50f80846a122f91af2d42047672  caliptra-rom-with-log.bin
+7e2c06d929577e439df7d8d3e6659d098e7416d41b01d4b523577bb91514ea8c3b512445512eef15cdac119f74eb8df5  caliptra-rom-no-log.bin
+67f4c0ec7b003d0201af45651591bb6faa118e36dcc2d65f7f646e6d14d8fc15d253fee07a31b87b85fa262084e998d7  caliptra-rom-with-log.bin
diff --git a/drivers/src/csrng.rs b/drivers/src/csrng.rs
index ff846cd0..2ec38a0d 100644
--- a/drivers/src/csrng.rs
+++ b/drivers/src/csrng.rs
@@ -26,21 +26,12 @@ use caliptra_registers::csrng::CsrngReg;
 use caliptra_registers::entropy_src::{self, regs::AlertFailCountsReadVal, EntropySrcReg};
 use caliptra_registers::soc_ifc::{self, SocIfcReg};
 
-use core::array;
+use core::mem::MaybeUninit;
 
 // https://opentitan.org/book/hw/ip/csrng/doc/theory_of_operation.html#command-description
 const MAX_SEED_WORDS: usize = 12;
 const WORDS_PER_BLOCK: usize = 4;
 
-struct IsCompleteBlocks<const NUM_WORDS: usize>;
-
-impl<const NUM_WORDS: usize> IsCompleteBlocks<NUM_WORDS> {
-    const ASSERT: () = assert!(
-        NUM_WORDS != 0 && NUM_WORDS % WORDS_PER_BLOCK == 0,
-        "NUM_WORDS must be non-zero and divisible by WORDS_PER_BLOCK"
-    );
-}
-
 /// A unique handle to the underlying CSRNG peripheral.
 pub struct Csrng {
     csrng: CsrngReg,
@@ -141,51 +132,35 @@ impl Csrng {
     /// }
     /// ```
     pub fn generate12(&mut self) -> CaliptraResult<[u32; 12]> {
-        self.generate()
-    }
-
-    /// Return 16 randomly generated [`u32`]s.
-    ///
-    /// # Errors
-    ///
-    /// Returns an error if the internal generate command fails.
-    ///
-    /// # Examples
-    ///
-    /// ```no_run
-    /// let mut csrng = ...;
-    ///
-    /// let random_words: [u32; 16] = csrng.generate()?;
-    ///
-    /// for word in random_words {
-    ///     // Do something with `word`.
-    /// }
-    /// ```
-    pub fn generate16(&mut self) -> CaliptraResult<[u32; 16]> {
-        self.generate()
-    }
-
-    fn generate<const N: usize>(&mut self) -> CaliptraResult<[u32; N]> {
-        #[allow(clippy::let_unit_value)]
-        let _ = IsCompleteBlocks::<N>::ASSERT;
-
         check_for_alert_state(self.entropy_src.regs())?;
 
         send_command(
             &mut self.csrng,
             Command::Generate {
-                num_128_bit_blocks: N / WORDS_PER_BLOCK,
+                num_128_bit_blocks: 12 / WORDS_PER_BLOCK,
             },
         )?;
 
-        Ok(array::from_fn(|i| {
-            if i % WORDS_PER_BLOCK == 0 {
-                // Wait for CSRNG to generate next block of words.
-                wait::until(|| self.csrng.regs().genbits_vld().read().genbits_vld());
-            }
-
-            self.csrng.regs().genbits().read()
-        }))
+        let mut result = MaybeUninit::<[u32; 12]>::uninit();
+        let dest = result.as_mut_ptr() as *mut u32;
+        unsafe {
+            wait::until(|| self.csrng.regs().genbits_vld().read().genbits_vld());
+            dest.add(0).write(self.csrng.regs().genbits().read());
+            dest.add(1).write(self.csrng.regs().genbits().read());
+            dest.add(2).write(self.csrng.regs().genbits().read());
+            dest.add(3).write(self.csrng.regs().genbits().read());
+            wait::until(|| self.csrng.regs().genbits_vld().read().genbits_vld());
+            dest.add(4).write(self.csrng.regs().genbits().read());
+            dest.add(5).write(self.csrng.regs().genbits().read());
+            dest.add(6).write(self.csrng.regs().genbits().read());
+            dest.add(7).write(self.csrng.regs().genbits().read());
+            wait::until(|| self.csrng.regs().genbits_vld().read().genbits_vld());
+            dest.add(8).write(self.csrng.regs().genbits().read());
+            dest.add(9).write(self.csrng.regs().genbits().read());
+            dest.add(10).write(self.csrng.regs().genbits().read());
+            dest.add(11).write(self.csrng.regs().genbits().read());
+            Ok(result.assume_init())
+        }
     }
 
     pub fn reseed(&mut self, seed: Seed) -> CaliptraResult<()> {
diff --git a/ureg/src/lib.rs b/ureg/src/lib.rs
index 11de8b8f..0a113924 100644
--- a/ureg/src/lib.rs
+++ b/ureg/src/lib.rs
@@ -237,6 +237,11 @@ impl Mmio for RealMmioMut<'_> {
     unsafe fn read_volatile<T: Clone + Copy>(&self, src: *const T) -> T {
         core::ptr::read_volatile(src)
     }
+
+    #[cfg(any(target_arch = "riscv32", target_arch = "riscv64"))]
+    unsafe fn read_volatile_array<const LEN: usize, T: Uint>(&self, dst: *mut T, src: *mut T) {
+        opt_riscv::read_volatile_array::<LEN, T>(dst, src)
+    }
 }
 impl MmioMut for RealMmioMut<'_> {
     /// Performs a volatile write of `src` to `dst`.


Test patch:
diff --git a/drivers/test-fw/src/bin/csrng_tests.rs b/drivers/test-fw/src/bin/csrng_tests.rs
index 9ca549ad..727f5134 100644
--- a/drivers/test-fw/src/bin/csrng_tests.rs
+++ b/drivers/test-fw/src/bin/csrng_tests.rs
@@ -31,10 +31,9 @@ fn test_ctr_drbg_ctr0_smoke() {
         0x5600419c, 0xca79b0b0, 0xdda33b5c, 0xa468649e, 0xdf5d73fa,
     ]);
 
-    const EXPECTED_OUTPUT: [u32; 16] = [
-        0xe48bb8cb, 0x1012c84c, 0x5af8a7f1, 0xd1c07cd9, 0xdf82ab22, 0x771c619b, 0xd40fccb1,
-        0x87189e99, 0x510494b3, 0x64f7ac0c, 0x2581f391, 0x80b1dc2f, 0x793e01c5, 0x87b107ae,
-        0xdb17514c, 0xa43c41b7,
+    const EXPECTED_OUTPUT: [u32; 12] = [
+        0x725eda90, 0xc79b4a14, 0xe43b74ac, 0x9d9a938b, 0xc395a610, 0x4c5a1483, 0xa45f15e8,
+        0x2708cbef, 0x89eb63a9, 0x70cdc6bc, 0x710daba1, 0xed39808c,
     ];
 
     let mut csrng =
@@ -42,12 +41,12 @@ fn test_ctr_drbg_ctr0_smoke() {
 
     // The original OpenTitan test tosses the first call to generate.
     let _ = csrng
-        .generate16()
+        .generate12()
         .expect("first call to generate should work");
 
     assert_eq!(
         csrng
-            .generate16()
+            .generate12()
             .expect("second call to generate should work"),
         EXPECTED_OUTPUT
     );