commit 8491a425bc47375a8bf1450689bcab30db7343bc
author Xin Li <delphij@google.com> Thu Aug 27 10:16:06 2020 -0700
committer Xin Li <delphij@google.com> Thu Aug 27 10:16:06 2020 -0700
tree 1252c9eeb76b56797b373a02e78ab1a190fd5339
parent cd311c1dbee378c2d1711a09414c16e3b258a012
parent 2af782b5ae1abca5b00c5f6ec5e3db782fed0cb8

Merge Android R (rvc-dev-plus-aosp-without-vendor@6692709)

Bug: 166295507
Merged-In: I29d286d476565af2af7cb6b5e2165ea8de2b2b69
Change-Id: Id72cfafe90e134554fab7d21fa0f94181d68e11d

libavb/avb_descriptor.c

diff --git a/libavb/avb_descriptor.c b/libavb/avb_descriptor.c
index 7030a40..222a616 100644
--- a/libavb/avb_descriptor.c
+++ b/libavb/avb_descriptor.c
@@ -48,6 +48,8 @@
   const uint8_t* desc_start;
   const uint8_t* desc_end;
   const uint8_t* p;
+  uint64_t desc_offset = 0;
+  uint64_t desc_size = 0;
 
   if (image_data == NULL) {
     avb_error("image_data is NULL\n.");
@@ -59,6 +61,9 @@
     goto out;
   }
 
+  /* The data is supposed to have been cryptographically verified at this point,
+   * This check is just adding defense in depth.
+   */
   if (image_size < sizeof(AvbVBMetaImageHeader)) {
     avb_error("Length is smaller than header.\n");
     goto out;
@@ -75,27 +80,50 @@
   header = (const AvbVBMetaImageHeader*)image_data;
   image_end = image_data + image_size;
 
-  desc_start = image_data + sizeof(AvbVBMetaImageHeader) +
-               avb_be64toh(header->authentication_data_block_size) +
-               avb_be64toh(header->descriptors_offset);
+  /* Since the data is supposed to have been cryptographically verified at this
+   * point, being overflow-safe is just for defense.
+   * The following lines are overflow-safe version of:
+   * desc_offset = sizeof(AvbVBMetaImageHeader) +
+   *               avb_be64toh(header->authentication_data_block_size)) +
+   *               avb_be64toh(header->descriptors_offset)
+   */
+  if (!avb_safe_add(&desc_offset,
+                    sizeof(AvbVBMetaImageHeader),
+                    avb_be64toh(header->authentication_data_block_size))) {
+    avb_error("Invalid authentication data block size.\n");
+    goto out;
+  }
+  if (!avb_safe_add_to(&desc_offset, avb_be64toh(header->descriptors_offset))) {
+    avb_error("Invalid descriptors offset.\n");
+    goto out;
+  }
 
-  desc_end = desc_start + avb_be64toh(header->descriptors_size);
-
-  if (desc_start < image_data || desc_start > image_end ||
-      desc_end < image_data || desc_end > image_end || desc_end < desc_start) {
+  if (desc_offset > (uint64_t)(image_end - image_data)) {
     avb_error("Descriptors not inside passed-in data.\n");
     goto out;
   }
 
+  desc_start = image_data + desc_offset;
+
+  desc_size = avb_be64toh(header->descriptors_size);
+  if (desc_size > (uint64_t)(image_end - desc_start)) {
+    avb_error("Descriptors not inside passed-in data.\n");
+    goto out;
+  }
+
+  desc_end = desc_start + desc_size;
+
   for (p = desc_start; p < desc_end;) {
     if (p + sizeof(AvbDescriptor) > desc_end) {
       avb_error("Invalid descriptor length.\n");
       goto out;
     }
     const AvbDescriptor* dh = (const AvbDescriptor*)p;
-    avb_assert_aligned(dh);
-    uint64_t nb_following = avb_be64toh(dh->num_bytes_following);
+    uint64_t nb_following;
     uint64_t nb_total = 0;
+    avb_assert_aligned(dh);
+    nb_following = avb_be64toh(dh->num_bytes_following);
+
     if (!avb_safe_add(&nb_total, sizeof(AvbDescriptor), nb_following)) {
       avb_error("Invalid descriptor length.\n");
       goto out;
@@ -106,7 +134,7 @@
       goto out;
     }
 
-    if (nb_total + p < desc_start || nb_total + p > desc_end) {
+    if (nb_total > (uint64_t)(desc_end - p)) {
       avb_error("Invalid data in descriptors array.\n");
       goto out;
     }
@@ -115,10 +143,7 @@
       goto out;
     }
 
-    if (!avb_safe_add_to((uint64_t*)(&p), nb_total)) {
-      avb_error("Invalid descriptor length.\n");
-      goto out;
-    }
+    p += nb_total;
   }
 
   ret = true;