Merge pull request #1657 from joshlf/unsafe-cleanup

Document or remove some uses of `unsafe`

Author: briansmith

Cargo.toml

@@ -168,6 +168,7 @@ name = "ring"
 [dependencies]
 getrandom = { version = "0.2.8" }
 untrusted = { version = "0.9" }
+zerocopy = { version = "0.7.5", features = ["derive"] }
 
 [target.'cfg(any(target_arch = "x86",target_arch = "x86_64", all(any(target_arch = "aarch64", target_arch = "arm"), any(target_os = "android", target_os = "fuchsia", target_os = "linux", target_os = "windows"))))'.dependencies]
 spin = { version = "0.9.2", default-features = false, features = ["once"] }

src/aead/gcm.rs

@@ -126,6 +126,12 @@ impl Context {
         debug_assert!(input_bytes > 0);
 
         let input = input.as_ptr() as *const [u8; BLOCK_LEN];
+        // SAFETY:
+        // - `[[u8; BLOCK_LEN]]` has the same bit validity as `[u8]`.
+        // - `[[u8; BLOCK_LEN]]` has the same alignment requirement as `[u8]`.
+        // - `input_bytes / BLOCK_LEN` ensures that the total length in bytes of
+        //   the new `[[u8; BLOCK_LEN]]` will not be longer than the original
+        //   `[u8]`.
         let input = unsafe { core::slice::from_raw_parts(input, input_bytes / BLOCK_LEN) };
 
         // Although these functions take `Xi` and `h_table` as separate

src/digest.rs

@@ -24,12 +24,9 @@
 // The goal for this implementation is to drive the overhead as close to zero
 // as possible.
 
-use crate::{
-    c, cpu, debug,
-    endian::{ArrayEncoding, BigEndian},
-    polyfill,
-};
+use crate::{c, cpu, debug, endian::BigEndian, polyfill};
 use core::num::Wrapping;
+use zerocopy::{AsBytes, FromBytes};
 
 mod sha1;
 mod sha2;
@@ -114,7 +111,9 @@ impl BlockContext {
 
         Digest {
             algorithm: self.algorithm,
-            value: (self.algorithm.format_output)(self.state),
+            // SAFETY: Invariant on this field promises that this is the correct
+            // format function for this algorithm's block size.
+            value: unsafe { (self.algorithm.format_output)(self.state) },
         }
     }
 }
@@ -248,8 +247,11 @@ impl Digest {
 impl AsRef<[u8]> for Digest {
     #[inline(always)]
     fn as_ref(&self) -> &[u8] {
-        let as64 = unsafe { &self.value.as64 };
-        &as64.as_byte_array()[..self.algorithm.output_len]
+        let data = (&self.value as *const Output).cast::<u8>();
+        // SAFETY: So long as `self.algorithm` is the correct algorithm, all
+        // code initializes all bytes of `self.value` in the range `[0,
+        // self.algorithm.output_len)`.
+        unsafe { core::slice::from_raw_parts(data, self.algorithm.output_len) }
     }
 }
 
@@ -270,7 +272,9 @@ pub struct Algorithm {
     len_len: usize,
 
     block_data_order: unsafe extern "C" fn(state: &mut State, data: *const u8, num: c::size_t),
-    format_output: fn(input: State) -> Output,
+    // INVARIANT: This is always set to the correct output for the algorithm's
+    // block size.
+    format_output: unsafe fn(input: State) -> Output,
 
     initial_state: State,
 
@@ -474,14 +478,38 @@ pub const MAX_OUTPUT_LEN: usize = 512 / 8;
 /// algorithms in this module.
 pub const MAX_CHAINING_LEN: usize = MAX_OUTPUT_LEN;
 
-fn sha256_format_output(input: State) -> Output {
+fn sha256_format_output(input: State) -> Output
+where
+    [BigEndian<u32>; 256 / 8 / core::mem::size_of::<BigEndian<u32>>()]: FromBytes,
+    [BigEndian<u64>; 512 / 8 / core::mem::size_of::<BigEndian<u64>>()]: AsBytes,
+{
+    // SAFETY: There are two cases:
+    // - The union is initialized as `as32`, in which case this is trivially
+    //   sound.
+    // - The union is initialized as `as64`. In this case, the `as64` variant is
+    //   longer than the `as32` variant, so all bytes of `as32` are initialized
+    //   as they are in the prefix of `as64`. Since `as64`'s type is `AsBytes`
+    //   (see the where bound on this function), all of its bytes are
+    //   initialized (ie, none are padding). Since `as32`'s type is `FromBytes`,
+    //   any initialized sequence of bytes constitutes a valid instance of the
+    //   type, so this is sound.
     let input = unsafe { &input.as32 };
     Output {
         as32: input.map(BigEndian::from),
     }
 }
 
-fn sha512_format_output(input: State) -> Output {
+/// # Safety
+///
+/// The caller must ensure that all bytes of `State` have been initialized.
+unsafe fn sha512_format_output(input: State) -> Output
+where
+    [BigEndian<u64>; 512 / 8 / core::mem::size_of::<BigEndian<u64>>()]: FromBytes,
+{
+    // SAFETY: Caller has promised that all bytes are initialized. Since
+    // `input.as64` is `FromBytes`, we know that this is sufficient to guarantee
+    // that the input has been initialized to a valid instance of the type of
+    // `input.as64`.
     let input = unsafe { &input.as64 };
     Output {
         as64: input.map(BigEndian::from),

src/endian.rs

@@ -1,10 +1,12 @@
-use core::num::Wrapping;
+use core::{mem, num::Wrapping};
+
+use zerocopy::{AsBytes, FromBytes, FromZeroes};
 
 /// An `Encoding` of a type `T` can be converted to/from its byte
 /// representation without any byte swapping or other computation.
 ///
 /// The `Self: Copy` constraint addresses `clippy::declare_interior_mutable_const`.
-pub trait Encoding<T>: From<T> + Into<T>
+pub trait Encoding<T>: From<T> + Into<T> + FromBytes + AsBytes
 where
     Self: Copy,
 {
@@ -25,7 +27,7 @@ pub trait FromByteArray<T> {
 
 macro_rules! define_endian {
     ($endian:ident) => {
-        #[derive(Clone, Copy)]
+        #[derive(Clone, Copy, FromZeroes, FromBytes, AsBytes)]
         #[repr(transparent)]
         pub struct $endian<T>(T);
 
@@ -48,7 +50,7 @@ macro_rules! impl_from_byte_array {
         {
             #[inline]
             fn from_byte_array(a: &[u8; $elems * core::mem::size_of::<$base>()]) -> Self {
-                unsafe { core::mem::transmute_copy(a) }
+                zerocopy::transmute!(*a)
             }
         }
     };
@@ -58,11 +60,24 @@ macro_rules! impl_array_encoding {
     ($endian:ident, $base:ident, $elems:expr) => {
         impl ArrayEncoding<[u8; $elems * core::mem::size_of::<$base>()]>
             for [$endian<$base>; $elems]
+        where
+            Self: AsBytes,
         {
             #[inline]
             fn as_byte_array(&self) -> &[u8; $elems * core::mem::size_of::<$base>()] {
+                const _: () = assert!(
+                    mem::size_of::<[$endian<$base>; $elems]>()
+                        == $elems * core::mem::size_of::<$base>()
+                );
                 let as_bytes_ptr =
                     self.as_ptr() as *const [u8; $elems * core::mem::size_of::<$base>()];
+                // SAFETY:
+                // - `Self: AsBytes`, so it's sound to observe the bytes of
+                //   `self` and to have a reference to those bytes alive at the
+                //   same time as `&self`.
+                // - As confirmed by the preceding assertion, the sizes of
+                //   `Self` and the return type are equal.
+                // - `[u8; N]` has no alignment requirement.
                 unsafe { &*as_bytes_ptr }
             }
         }

src/rsa/public_exponent.rs

@@ -12,8 +12,14 @@ impl PublicExponent {
 
     // TODO: Use `NonZeroU64::new(...).unwrap()` when `feature(const_panic)` is
     // stable.
-    pub(super) const _3: Self = Self(unsafe { NonZeroU64::new_unchecked(3) });
-    pub(super) const _65537: Self = Self(unsafe { NonZeroU64::new_unchecked(65537) });
+    pub(super) const _3: Self = Self(match NonZeroU64::new(3) {
+        Some(nz) => nz,
+        None => unreachable!(),
+    });
+    pub(super) const _65537: Self = Self(match NonZeroU64::new(65537) {
+        Some(nz) => nz,
+        None => unreachable!(),
+    });
 
     // This limit was chosen to bound the performance of the simple
     // exponentiation-by-squaring implementation in `elem_exp_vartime`. In
@@ -29,7 +35,10 @@ impl PublicExponent {
     //
     // TODO: Use `NonZeroU64::new(...).unwrap()` when `feature(const_panic)` is
     // stable.
-    const MAX: Self = Self(unsafe { NonZeroU64::new_unchecked((1u64 << 33) - 1) });
+    const MAX: Self = Self(match NonZeroU64::new((1u64 << 33) - 1) {
+        Some(nz) => nz,
+        None => unreachable!(),
+    });
 
     pub(super) fn from_be_bytes(
         input: untrusted::Input,