keymaster: fix pointer overflow checks

Compiler can optimize away pointer overflow checks. Cast pointers
to uintptr_t to make sure this doesn't happen.

Bug: 27774248
Change-Id: Ib1d054ea5586cf110ae6cbbcd8ca1cd9e157c170

2 files changed, 17 insertions, 7 deletions

diff --git a/include/keymaster/serializable.h b/include/keymaster/serializable.h
index 6dd31b2..60973fb 100644
--- a/include/keymaster/serializable.h
+++ b/include/keymaster/serializable.h
@@ -62,6 +62,14 @@ class Serializable {
  */
 
 /**
+ * Convert a pointer into a value.  This is used to make sure compiler won't optimize away pointer
+ * overflow checks. (See http://www.kb.cert.org/vuls/id/162289)
+ */
+template <typename T> inline uintptr_t __pval(const T *p) {
+    return reinterpret_cast<uintptr_t>(p);
+}
+
+/**
  * Append a byte array to a buffer.  Note that by itself this function isn't very useful, because it
  * provides no indication in the serialized buffer of what the array size is.  For writing arrays,
  * see \p append_size_and_data_to_buf().
@@ -111,7 +119,8 @@ template <typename T>
 inline uint8_t* append_uint32_array_to_buf(uint8_t* buf, const uint8_t* end, const T* data,
                                            size_t count) {
     // Check for overflow
-    if (count >= (UINT32_MAX / sizeof(uint32_t)) || buf + count * sizeof(uint32_t) < buf)
+    if (count >= (UINT32_MAX / sizeof(uint32_t)) ||
+        __pval(buf) + count * sizeof(uint32_t) < __pval(buf))
         return buf;
     buf = append_uint32_to_buf(buf, end, count);
     for (size_t i = 0; i < count; ++i)
@@ -172,8 +181,9 @@ inline bool copy_uint32_array_from_buf(const uint8_t** buf_ptr, const uint8_t* e
     if (!copy_uint32_from_buf(buf_ptr, end, count))
         return false;
 
-    const uint8_t* array_end = *buf_ptr + *count * sizeof(uint32_t);
-    if (*count >= UINT32_MAX / sizeof(uint32_t) || array_end < *buf_ptr || array_end > end)
+    uintptr_t array_end = __pval(*buf_ptr) + *count * sizeof(uint32_t);
+    if (*count >= UINT32_MAX / sizeof(uint32_t) ||
+        array_end < __pval(*buf_ptr) || array_end > __pval(end))
         return false;
 
     data->reset(new (std::nothrow) T[*count]);
diff --git a/serializable.cpp b/serializable.cpp
index 5db64f8..94b92d0 100644
--- a/serializable.cpp
+++ b/serializable.cpp
@@ -25,7 +25,7 @@
 namespace keymaster {
 
 uint8_t* append_to_buf(uint8_t* buf, const uint8_t* end, const void* data, size_t data_len) {
-    if (buf + data_len < buf)  // Pointer wrap check
+    if (__pval(buf) + data_len < __pval(buf))  // Pointer wrap check
         return buf;
 
     if (buf + data_len <= end) {
@@ -36,7 +36,7 @@ uint8_t* append_to_buf(uint8_t* buf, const uint8_t* end, const void* data, size_
 }
 
 bool copy_from_buf(const uint8_t** buf_ptr, const uint8_t* end, void* dest, size_t size) {
-    if (*buf_ptr + size < *buf_ptr)  // Pointer wrap check
+    if (__pval(*buf_ptr) + size < __pval(*buf_ptr))  // Pointer wrap check
         return false;
 
     if (end < *buf_ptr + size)
@@ -51,7 +51,7 @@ bool copy_size_and_data_from_buf(const uint8_t** buf_ptr, const uint8_t* end, si
     if (!copy_uint32_from_buf(buf_ptr, end, size))
         return false;
 
-    if (*buf_ptr + *size < *buf_ptr)  // Pointer wrap check
+    if (__pval(*buf_ptr) + *size < __pval(*buf_ptr))  // Pointer wrap check
         return false;
 
     if (*buf_ptr + *size > end)
@@ -96,7 +96,7 @@ bool Buffer::Reinitialize(size_t size) {
 
 bool Buffer::Reinitialize(const void* data, size_t data_len) {
     Clear();
-    if (static_cast<const uint8_t*>(data) + data_len < data)  // Pointer wrap check
+    if (__pval(data) + data_len < __pval(data))  // Pointer wrap check
         return false;
     buffer_.reset(new (std::nothrow) uint8_t[data_len]);
     if (!buffer_.get())