commit 010c6d14687da8bfb1f49281665dce74b2ca2ea4
author Kelvin Zhang <zhangkelvin@google.com> Tue May 16 17:38:04 2023 -0700
committer Kelvin Zhang <zhangkelvin@google.com> Wed May 17 10:15:34 2023 -0700
tree 8a93bda23e76dea8bdeed55a53f37ed389b5af7d
parent 32b1d7bdcbdff8d5c2c01b2af8c25ef5e43806a3

Fix boot image parsing when input image is too small

ReadFileChunks() returns true if read ends due to an EOF. But caller
will still assume that the output blob contains full expected data,
resulting in a segfault.

Test: th
Bug: 282104567
Change-Id: I59b9b7513dbf4db9b75395c898cca7080e893930

payload_generator/boot_img_filesystem.cc

diff --git a/payload_generator/boot_img_filesystem.cc b/payload_generator/boot_img_filesystem.cc
index cad267e..3e324f1 100644
--- a/payload_generator/boot_img_filesystem.cc
+++ b/payload_generator/boot_img_filesystem.cc
@@ -16,10 +16,15 @@
 
 #include "update_engine/payload_generator/boot_img_filesystem.h"
 
+#include <array>
+
+#include <android-base/unique_fd.h>
+#include <android-base/file.h>
 #include <base/logging.h>
 #include <bootimg.h>
 #include <brillo/secure_blob.h>
 #include <puffin/utils.h>
+#include <sys/stat.h>
 
 #include "update_engine/common/utils.h"
 #include "update_engine/payload_generator/delta_diff_generator.h"
@@ -33,11 +38,26 @@
 
 unique_ptr<BootImgFilesystem> BootImgFilesystem::CreateFromFile(
     const string& filename) {
-  if (filename.empty())
+  if (filename.empty()) {
     return nullptr;
-
-  if (brillo::Blob header_magic;
-      !utils::ReadFileChunk(filename, 0, BOOT_MAGIC_SIZE, &header_magic) ||
+  }
+  android::base::unique_fd fd(open(filename.c_str(), O_RDONLY));
+  if (!fd.ok()) {
+    PLOG(ERROR) << "Failed to open " << filename;
+    return nullptr;
+  }
+  struct stat st {};
+  if (fstat(fd.get(), &st) < 0) {
+    return nullptr;
+  }
+  if (static_cast<size_t>(st.st_size) < sizeof(boot_img_hdr_v0)) {
+    LOG(INFO) << "Image " << filename
+              << " is too small to be a boot image. file size: " << st.st_size;
+    return nullptr;
+  }
+  std::array<char, BOOT_MAGIC_SIZE> header_magic{};
+  if (!android::base::ReadFullyAtOffset(
+          fd, header_magic.data(), BOOT_MAGIC_SIZE, 0) ||
       memcmp(header_magic.data(), BOOT_MAGIC, BOOT_MAGIC_SIZE) != 0) {
     return nullptr;
   }
@@ -48,11 +68,11 @@
   // See details in system/tools/mkbootimg/include/bootimg/bootimg.h
   constexpr size_t header_version_offset =
       BOOT_MAGIC_SIZE + 8 * sizeof(uint32_t);
-  brillo::Blob header_version_blob;
-  if (!utils::ReadFileChunk(filename,
-                            header_version_offset,
-                            sizeof(uint32_t),
-                            &header_version_blob)) {
+  std::array<char, sizeof(uint32_t)> header_version_blob{};
+  if (!android::base::ReadFullyAtOffset(fd,
+                                        header_version_blob.data(),
+                                        sizeof(uint32_t),
+                                        header_version_offset)) {
     return nullptr;
   }
   uint32_t header_version =
@@ -66,8 +86,8 @@
   // Read the bytes of boot image header based on the header version.
   size_t header_size =
       header_version == 3 ? sizeof(boot_img_hdr_v3) : sizeof(boot_img_hdr_v0);
-  brillo::Blob header_blob;
-  if (!utils::ReadFileChunk(filename, 0, header_size, &header_blob)) {
+  brillo::Blob header_blob(header_size);
+  if (!ReadFullyAtOffset(fd, header_blob.data(), header_size, 0)) {
     return nullptr;
   }
 
@@ -120,7 +140,8 @@
   file.extents = {ExtentForBytes(kBlockSize, offset, size)};
 
   brillo::Blob data;
-  if (utils::ReadFileChunk(filename_, offset, size, &data)) {
+  if (utils::ReadFileChunk(filename_, offset, size, &data) &&
+      data.size() == size) {
     constexpr size_t kGZipHeaderSize = 10;
     // Check GZip header magic.
     if (data.size() > kGZipHeaderSize && data[0] == 0x1F && data[1] == 0x8B) {