commit b192f24cc667036262f20ae0ae2844d44bbcdcb3
author Philipp Wagner <phw@lowrisc.org> Wed Jan 13 12:03:29 2021 +0000
committer Philipp Wagner <mail@philipp-wagner.com> Wed Jan 13 16:43:38 2021 +0000
tree e11cc47b3215b2fb665e9f791dc146816e6c27ce
parent 351c14bf617c40c9b64a6f24dffe0665ae6dde56

[dif/otbn] Minor code style cleanups

Address review comments in a related PR regarding the OTBN DIF.

Signed-off-by: Philipp Wagner <phw@lowrisc.org>

sw/device/lib/dif/dif_otbn.c

diff --git a/sw/device/lib/dif/dif_otbn.c b/sw/device/lib/dif/dif_otbn.c
index 64fa8ea..4071075 100644
--- a/sw/device/lib/dif/dif_otbn.c
+++ b/sw/device/lib/dif/dif_otbn.c
@@ -34,6 +34,21 @@
   return true;
 }
 
+/**
+ * Ensures that `offset` and `size` are valid for a given `mem_size`.
+ *
+ * Valid are 32b word accesses to 32b-aligned memory locations within
+ * `mem_size`.
+ */
+static bool check_offset_len(uint32_t offset_bytes, size_t len_bytes,
+                             size_t mem_size) {
+  // The overflow check below assumes/requires two unsigned inputs.
+  return (len_bytes % sizeof(uint32_t) == 0 &&
+          offset_bytes % sizeof(uint32_t) == 0 &&
+          offset_bytes + len_bytes >= len_bytes &&
+          offset_bytes + len_bytes <= mem_size);
+}
+
 dif_otbn_result_t dif_otbn_init(const dif_otbn_config_t *config,
                                 dif_otbn_t *otbn) {
   if (config == NULL || otbn == NULL) {
@@ -173,7 +188,7 @@
 
 dif_otbn_result_t dif_otbn_start(const dif_otbn_t *otbn,
                                  unsigned int start_addr) {
-  if (otbn == NULL || start_addr % 4 != 0 ||
+  if (otbn == NULL || start_addr % sizeof(uint32_t) != 0 ||
       start_addr >= OTBN_IMEM_SIZE_BYTES) {
     return kDifOtbnBadArg;
   }
@@ -193,10 +208,7 @@
   }
 
   uint32_t status = mmio_region_read32(otbn->base_addr, OTBN_STATUS_REG_OFFSET);
-  *busy = bitfield_field32_read(status, (bitfield_field32_t){
-                                            .mask = 1,
-                                            .index = OTBN_STATUS_BUSY_BIT,
-                                        });
+  *busy = bitfield_bit32_read(status, OTBN_STATUS_BUSY_BIT);
 
   return kDifOtbnOk;
 }
@@ -233,10 +245,8 @@
 dif_otbn_result_t dif_otbn_imem_write(const dif_otbn_t *otbn,
                                       uint32_t offset_bytes, const void *src,
                                       size_t len_bytes) {
-  // Only 32b-aligned 32b word accesses are allowed.
-  if (otbn == NULL || src == NULL || len_bytes % 4 != 0 ||
-      offset_bytes % 4 != 0 ||
-      offset_bytes + len_bytes > OTBN_IMEM_SIZE_BYTES) {
+  if (otbn == NULL || src == NULL ||
+      !check_offset_len(offset_bytes, len_bytes, OTBN_IMEM_SIZE_BYTES)) {
     return kDifOtbnBadArg;
   }
 
@@ -249,10 +259,8 @@
 dif_otbn_result_t dif_otbn_imem_read(const dif_otbn_t *otbn,
                                      uint32_t offset_bytes, void *dest,
                                      size_t len_bytes) {
-  // Only 32b-aligned 32b word accesses are allowed.
-  if (otbn == NULL || dest == NULL || len_bytes % 4 != 0 ||
-      offset_bytes % 4 != 0 ||
-      offset_bytes + len_bytes > OTBN_IMEM_SIZE_BYTES) {
+  if (otbn == NULL || dest == NULL ||
+      !check_offset_len(offset_bytes, len_bytes, OTBN_IMEM_SIZE_BYTES)) {
     return kDifOtbnBadArg;
   }
 
@@ -265,10 +273,8 @@
 dif_otbn_result_t dif_otbn_dmem_write(const dif_otbn_t *otbn,
                                       uint32_t offset_bytes, const void *src,
                                       size_t len_bytes) {
-  // Only 32b-aligned 32b word accesses are allowed.
-  if (otbn == NULL || src == NULL || len_bytes % 4 != 0 ||
-      offset_bytes % 4 != 0 ||
-      offset_bytes + len_bytes > OTBN_DMEM_SIZE_BYTES) {
+  if (otbn == NULL || src == NULL ||
+      !check_offset_len(offset_bytes, len_bytes, OTBN_DMEM_SIZE_BYTES)) {
     return kDifOtbnBadArg;
   }
 
@@ -281,10 +287,8 @@
 dif_otbn_result_t dif_otbn_dmem_read(const dif_otbn_t *otbn,
                                      uint32_t offset_bytes, void *dest,
                                      size_t len_bytes) {
-  // Only 32b-aligned 32b word accesses are allowed.
-  if (otbn == NULL || dest == NULL || len_bytes % 4 != 0 ||
-      offset_bytes % 4 != 0 ||
-      offset_bytes + len_bytes > OTBN_DMEM_SIZE_BYTES) {
+  if (otbn == NULL || dest == NULL ||
+      !check_offset_len(offset_bytes, len_bytes, OTBN_IMEM_SIZE_BYTES)) {
     return kDifOtbnBadArg;
   }
 

sw/device/tests/dif/dif_otbn_unittest.cc

diff --git a/sw/device/tests/dif/dif_otbn_unittest.cc b/sw/device/tests/dif/dif_otbn_unittest.cc
index cad1d69..230ba00 100644
--- a/sw/device/tests/dif/dif_otbn_unittest.cc
+++ b/sw/device/tests/dif/dif_otbn_unittest.cc
@@ -373,6 +373,22 @@
   EXPECT_EQ(result, kDifOtbnBadArg);
 }
 
+TEST_F(ImemWriteTest, BadAddressBeyondMemorySize) {
+  dif_otbn_result_t result;
+  uint32_t test_data[] = {0};
+
+  result = dif_otbn_imem_write(&dif_otbn_, OTBN_IMEM_SIZE_BYTES, test_data, 4);
+  EXPECT_EQ(result, kDifOtbnBadArg);
+}
+
+TEST_F(ImemWriteTest, BadAddressIntegerOverflow) {
+  dif_otbn_result_t result;
+  uint32_t test_data[4] = {0};
+
+  result = dif_otbn_imem_write(&dif_otbn_, 0xFFFFFFFC, test_data, 16);
+  EXPECT_EQ(result, kDifOtbnBadArg);
+}
+
 TEST_F(ImemWriteTest, SuccessWithoutOffset) {
   // Test assumption.
   ASSERT_GE(OTBN_IMEM_SIZE_BYTES, 8);