Add, standardise or temporarily opt out of safety comments for keystore2.

These will soon be required by a lint.

Some functions were incorrectly marked as safe which were not actually
safe, so I've fixed those too.

Bug: 290018030
Test: m rust
Change-Id: I38df6a8162d430617f123ab1aace38b741458fce
diff --git a/keystore2/src/crypto/lib.rs b/keystore2/src/crypto/lib.rs
index 08b7589..f8fc574 100644
--- a/keystore2/src/crypto/lib.rs
+++ b/keystore2/src/crypto/lib.rs
@@ -49,8 +49,8 @@
 /// Generate an AES256 key, essentially 32 random bytes from the underlying
 /// boringssl library discretely stuffed into a ZVec.
 pub fn generate_aes256_key() -> Result<ZVec, Error> {
-    // Safety: key has the same length as the requested number of random bytes.
     let mut key = ZVec::new(AES_256_KEY_LENGTH)?;
+    // Safety: key has the same length as the requested number of random bytes.
     if unsafe { randomBytes(key.as_mut_ptr(), AES_256_KEY_LENGTH) } {
         Ok(key)
     } else {
@@ -65,8 +65,8 @@
 
 /// Generate random data of the given size.
 pub fn generate_random_data(size: usize) -> Result<Vec<u8>, Error> {
-    // Safety: data has the same length as the requested number of random bytes.
     let mut data = vec![0; size];
+    // Safety: data has the same length as the requested number of random bytes.
     if unsafe { randomBytes(data.as_mut_ptr(), size) } {
         Ok(data)
     } else {
@@ -209,6 +209,8 @@
         let pw = self.get_key();
         let mut result = ZVec::new(key_length)?;
 
+        // Safety: We checked that the salt is exactly 16 bytes long. The other pointers are valid,
+        // and have matching lengths.
         unsafe {
             generateKeyFromPassword(
                 result.as_mut_ptr(),
@@ -324,10 +326,10 @@
 /// Calls the boringssl ECDH_compute_key function.
 pub fn ecdh_compute_key(pub_key: &EC_POINT, priv_key: &ECKey) -> Result<ZVec, Error> {
     let mut buf = ZVec::new(EC_MAX_BYTES)?;
+    let result =
     // Safety: Our ECDHComputeKey wrapper passes EC_MAX_BYES to ECDH_compute_key, which
     // writes at most that many bytes to the output.
     // The two keys are valid objects.
-    let result =
         unsafe { ECDHComputeKey(buf.as_mut_ptr() as *mut std::ffi::c_void, pub_key, priv_key.0) };
     if result == -1 {
         return Err(Error::ECDHComputeKeyFailed);
@@ -490,6 +492,8 @@
         let key = vec![0; 16];
         let iv = vec![0; 12];
         let mut tag = vec![0; 16];
+        // SAFETY: The various pointers are obtained from references so they are valid, and
+        // `AES_gcm_encrypt` and `AES_gcm_decrypt` don't do anything with them after they return.
         unsafe {
             let res = AES_gcm_encrypt(
                 input.as_ptr(),
@@ -521,8 +525,10 @@
     fn test_create_key_id() {
         let blob = vec![0; 16];
         let mut out: u64 = 0;
+        // SAFETY: The pointers are obtained from references so they are valid, the length matches
+        // the length of the array, and `CreateKeyId` doesn't access them after it returns.
         unsafe {
-            let res = CreateKeyId(blob.as_ptr(), 16, &mut out);
+            let res = CreateKeyId(blob.as_ptr(), blob.len(), &mut out);
             assert!(res);
             assert_ne!(out, 0);
         }
@@ -533,8 +539,17 @@
         let mut key = vec![0; 16];
         let pw = vec![0; 16];
         let salt = vec![0; 16];
+        // SAFETY: The pointers are obtained from references so they are valid, the salt is the
+        // expected length, the other lengths match the lengths of the arrays, and
+        // `generateKeyFromPassword` doesn't access them after it returns.
         unsafe {
-            generateKeyFromPassword(key.as_mut_ptr(), 16, pw.as_ptr(), 16, salt.as_ptr());
+            generateKeyFromPassword(
+                key.as_mut_ptr(),
+                key.len(),
+                pw.as_ptr(),
+                pw.len(),
+                salt.as_ptr(),
+            );
         }
         assert_ne!(key, vec![0; 16]);
     }
diff --git a/keystore2/src/crypto/zvec.rs b/keystore2/src/crypto/zvec.rs
index 5a173c3..c917a89 100644
--- a/keystore2/src/crypto/zvec.rs
+++ b/keystore2/src/crypto/zvec.rs
@@ -45,6 +45,7 @@
         let v: Vec<u8> = vec![0; size];
         let b = v.into_boxed_slice();
         if size > 0 {
+            // SAFETY: The address range is part of our address space.
             unsafe { mlock(b.as_ptr() as *const std::ffi::c_void, b.len()) }?;
         }
         Ok(Self { elems: b, len: size })
@@ -71,11 +72,16 @@
 impl Drop for ZVec {
     fn drop(&mut self) {
         for i in 0..self.elems.len() {
-            unsafe { write_volatile(self.elems.as_mut_ptr().add(i), 0) };
+            // SAFETY: The pointer is valid and properly aligned because it came from a reference.
+            unsafe { write_volatile(&mut self.elems[i], 0) };
         }
         if !self.elems.is_empty() {
             if let Err(e) =
-                unsafe { munlock(self.elems.as_ptr() as *const std::ffi::c_void, self.elems.len()) }
+                // SAFETY: The address range is part of our address space, and was previously locked
+                // by `mlock` in `ZVec::new` or the `TryFrom<Vec<u8>>` implementation.
+                unsafe {
+                    munlock(self.elems.as_ptr() as *const std::ffi::c_void, self.elems.len())
+                }
             {
                 log::error!("In ZVec::drop: `munlock` failed: {:?}.", e);
             }
@@ -130,6 +136,7 @@
         v.resize(v.capacity(), 0);
         let b = v.into_boxed_slice();
         if !b.is_empty() {
+            // SAFETY: The address range is part of our address space.
             unsafe { mlock(b.as_ptr() as *const std::ffi::c_void, b.len()) }?;
         }
         Ok(Self { elems: b, len })
diff --git a/keystore2/src/keystore2_main.rs b/keystore2/src/keystore2_main.rs
index 31c1e29..059d59d 100644
--- a/keystore2/src/keystore2_main.rs
+++ b/keystore2/src/keystore2_main.rs
@@ -68,6 +68,8 @@
     fn sqlite_log_handler(err: c_int, message: &str) {
         log::error!("[SQLITE3] {}: {}", err, message);
     }
+    // SAFETY: There are no other threads yet, `sqlite_log_handler` is threadsafe, and it doesn't
+    // invoke any SQLite calls.
     unsafe { sqlite_trace::config_log(Some(sqlite_log_handler)) }
         .expect("Error setting sqlite log callback.");
 
diff --git a/keystore2/src/km_compat/lib.rs b/keystore2/src/km_compat/lib.rs
index 2632ec4..e61a13a 100644
--- a/keystore2/src/km_compat/lib.rs
+++ b/keystore2/src/km_compat/lib.rs
@@ -21,6 +21,7 @@
 
 #[allow(missing_docs)] // TODO remove this
 pub fn add_keymint_device_service() -> i32 {
+    // SAFETY: This is always safe to call.
     unsafe { addKeyMintDeviceService() }
 }
 
diff --git a/keystore2/src/utils.rs b/keystore2/src/utils.rs
index acac7ee..584a51c 100644
--- a/keystore2/src/utils.rs
+++ b/keystore2/src/utils.rs
@@ -215,8 +215,8 @@
 /// as an integer.
 pub fn get_current_time_in_milliseconds() -> i64 {
     let mut current_time = libc::timespec { tv_sec: 0, tv_nsec: 0 };
-    // Following unsafe block includes one system call to get monotonic time.
-    // Therefore, it is not considered harmful.
+    // SAFETY: The pointer is valid because it comes from a reference, and clock_gettime doesn't
+    // retain it beyond the call.
     unsafe { libc::clock_gettime(libc::CLOCK_MONOTONIC_RAW, &mut current_time) };
     current_time.tv_sec as i64 * 1000 + (current_time.tv_nsec as i64 / 1_000_000)
 }