From 509449efff48b83dec84d28eb2430ac083dd7af0 Mon Sep 17 00:00:00 2001
From: Ken Russell <kbr@chromium.org>
Date: Fri, 22 May 2026 23:15:02 -0700
Subject: [PATCH] Properly handle EXT_texture_norm16 formats in ReadPixels.

Add defense-in-depth by ensuring the computed size for pixel formats
is non-zero, and that if ReadPixels is called with a non-zero area,
the resulting size is non-zero.

Added unit test from the bug report.

Co-authored by jetski-cli.

Fixed: 513165325
Change-Id: I4fb6e8e37b7286d328b3f79798311da24e805f06
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7872324
Commit-Queue: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1635378}
---

diff --git a/gpu/command_buffer/common/gles2_cmd_utils.cc b/gpu/command_buffer/common/gles2_cmd_utils.cc
index fc0aff1..f7583385 100644
--- a/gpu/command_buffer/common/gles2_cmd_utils.cc
+++ b/gpu/command_buffer/common/gles2_cmd_utils.cc
@@ -569,15 +569,18 @@
     case GL_RGB:
     case GL_RGB_INTEGER:
     case GL_SRGB_EXT:
+    case GL_RGB16_EXT:
       return 3;
     case GL_LUMINANCE_ALPHA:
     case GL_RG_EXT:
     case GL_RG_INTEGER:
+    case GL_RG16_EXT:
       return 2;
     case GL_RGBA:
     case GL_RGBA_INTEGER:
     case GL_BGRA_EXT:
     case GL_SRGB_ALPHA_EXT:
+    case GL_RGBA16_EXT:
       return 4;
     case GL_ALPHA:
     case GL_LUMINANCE:
@@ -589,6 +592,7 @@
     case GL_DEPTH_STENCIL_OES:
     case GL_RED_EXT:
     case GL_RED_INTEGER:
+    case GL_R16_EXT:
       return 1;
     default:
       return 0;
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index c079b3b..1c5abf4d 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -11279,6 +11279,18 @@
     return error::kOutOfBounds;
   }
 
+  // Defense-in-depth: read_pixel_format may accept enums (e.g. sized norm16
+  // internal formats) that GLES2Util::ElementsPerGroup does not recognize, in
+  // which case ComputeImageDataSizesES3 succeeds with a 0-byte size. If we
+  // proceeded, GetSharedMemoryAs would validate 0 bytes and we would hand the
+  // pointer to the native driver, which writes width*height*bpp bytes -> OOB
+  // write past the renderer's transfer buffer in the GPU process.
+  if (width > 0 && height > 0 && pixels_size == 0) {
+    LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, func_name,
+                       "unsupported format/type combination");
+    return error::kNoError;
+  }
+
   uint8_t* pixels = nullptr;
   Buffer* buffer = state_.bound_pixel_pack_buffer.get();
   if (pixels_shm_id == 0) {
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
index d9f96879..b39cc0e 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
@@ -14,6 +14,9 @@
 #include "base/containers/heap_array.h"
 #include "base/memory/raw_ptr.h"
 #include "base/strings/string_number_conversions.h"
+#if defined(ADDRESS_SANITIZER)
+#include <sanitizer/asan_interface.h>
+#endif
 #include "gpu/command_buffer/common/gles2_cmd_format.h"
 #include "gpu/command_buffer/common/gles2_cmd_utils.h"
 #include "gpu/command_buffer/service/context_group.h"
@@ -26,6 +29,7 @@
 #include "gpu/command_buffer/service/program_manager.h"
 #include "gpu/command_buffer/service/test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/abseil-cpp/absl/cleanup/cleanup.h"
 #include "ui/gl/gl_implementation.h"
 #include "ui/gl/gl_mock.h"
 #include "ui/gl/gl_surface_stub.h"
@@ -828,6 +832,93 @@
   EXPECT_EQ(GL_NO_ERROR, GetGLError());
 }
 
+TEST_P(GLES3DecoderManualInitTest, ReadPixelsR16ExtZeroSizeValidationOOB) {
+  InitState init;
+  init.extensions = "GL_EXT_texture_norm16";
+  init.gl_version = "OpenGL ES 3.0";
+  init.context_type = CONTEXT_TYPE_OPENGLES3;
+  InitDecoder(init);
+
+  // Step 1: directly demonstrate the validator/size-calculator desync.
+  uint32_t computed = 0xdeadbeef;
+  EXPECT_TRUE(GLES2Util::ComputeImageDataSizes(64, 64, 1, GL_R16_EXT,
+                                               GL_UNSIGNED_SHORT, 4, &computed,
+                                               nullptr, nullptr));
+  EXPECT_EQ(8192u, computed);
+
+  // Step 2: send a hostile ReadPixels that a compromised renderer could craft.
+  // pixels_shm_offset = end-of-buffer; the decoder validates 0 bytes so this
+  // passes and returns base + kSharedBufferSize (one-past-end).
+  surface_->SetSize(gfx::Size(INT_MAX, INT_MAX));
+  const GLsizei kWidth = 32;
+  const GLsizei kHeight = 32;
+  // Native R16 + UNSIGNED_SHORT = 2 bytes/pixel -> 32*32*2 = 2048 bytes
+  // written by the driver, all of it past the validated buffer end.
+  const uint32_t kPixelsOffset = kSharedBufferSize;  // one-past-end
+
+#if defined(ADDRESS_SANITIZER)
+  // The transfer buffer is mmap'd shared memory (no ASAN redzones). Poison the
+  // page slack past the logical buffer end so ASAN deterministically reports
+  // the OOB write instead of relying on whatever mapping happens to follow.
+  // kSharedBufferSize = 2048; the mapping is at least one 4096-byte page, so
+  // [base+2048, base+4096) is mapped slack we can safely poison.
+
+  // SAFETY: this is the only feasible way to get the pointer we need.
+  // This test is temporary regardless and will be removed along with
+  // the validating command decoder.
+  uint8_t* poison_at = UNSAFE_BUFFERS(
+      static_cast<uint8_t*>(shared_memory_base_.get()) + kSharedBufferSize);
+  static_assert(kSharedBufferSize == 2048);
+  const size_t kPoisonLen = 2048;
+  ASAN_POISON_MEMORY_REGION(poison_at, kPoisonLen);
+  absl::Cleanup unpoison = [poison_at] {
+    ASAN_UNPOISON_MEMORY_REGION(poison_at, kPoisonLen);
+  };
+#endif
+
+  // The decoder falls through to the native-driver IMPLEMENTATION_COLOR_READ_*
+  // query because (GL_R16_EXT, GL_UNSIGNED_SHORT) is not in the hardcoded
+  // accepted list. Simulate an Android native driver that returns the sized
+  // enum (the precondition documented in the report).
+  EXPECT_CALL(*gl_, GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_FORMAT, _))
+      .Times(AnyNumber())
+      .WillRepeatedly(SetArgPointee<1>(GL_R16_EXT));
+  EXPECT_CALL(*gl_, GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_TYPE, _))
+      .Times(AnyNumber())
+      .WillRepeatedly(SetArgPointee<1>(GL_UNSIGNED_SHORT));
+  EXPECT_CALL(*gl_, GetError())
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(GL_NO_ERROR));
+
+  // The "native driver" computes its own write size for R16 + UNSIGNED_SHORT
+  // (2 bytes/pixel) and writes into the pointer the decoder validated for 0
+  // bytes. With kWidth*kHeight*2 = 8192 bytes starting at one-past-end of a
+  // 2048-byte transfer buffer, this is a hard OOB write past the shm mapping.
+  EXPECT_CALL(
+      *gl_, ReadPixels(0, 0, kWidth, kHeight, GL_R16_EXT, GL_UNSIGNED_SHORT, _))
+      .Times(AnyNumber())
+      .WillRepeatedly([](GLint, GLint, GLsizei w, GLsizei h, GLenum, GLenum,
+                         void* pixels) {
+        // What a native R16 glReadPixels does: write w*h*2 bytes.
+
+        // SAFETY: taking a shortcut to fill this memory since this
+        // test is temporary and will be removed soon along with the
+        // validating command decoder.
+        UNSAFE_BUFFERS(memset(pixels, 0xAB, static_cast<size_t>(w) * h * 2u));
+      });
+
+  cmds::ReadPixels cmd;
+  cmd.Init(0, 0, kWidth, kHeight, GL_R16_EXT, GL_UNSIGNED_SHORT,
+           shared_memory_id_, kPixelsOffset, /*result_shm_id=*/0,
+           /*result_shm_offset=*/0, /*async=*/false);
+  // OOB write fires inside this call: ASAN reports a 2048-byte write past the
+  // end of the transfer buffer (the region the decoder validated for 0 bytes).
+  // After the bandaid fix, the command is rejected with GL_INVALID_OPERATION
+  // before reaching glReadPixelsFn.
+  ExecuteCmd(cmd);
+  GetGLError();  // Consume the INVALID_OPERATION emitted by the fixed decoder.
+}
+
 TEST_P(GLES3DecoderTest, ReadPixelsPixelPackBufferIsNotLargeEnough) {
   const GLsizei kWidth = 5;
   const GLsizei kHeight = 3;

diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
index d9f96879..b39cc0e 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc
@@ -14,6 +14,9 @@
 #include "base/containers/heap_array.h"
 #include "base/memory/raw_ptr.h"
 #include "base/strings/string_number_conversions.h"
+#if defined(ADDRESS_SANITIZER)
+#include <sanitizer/asan_interface.h>
+#endif
 #include "gpu/command_buffer/common/gles2_cmd_format.h"
 #include "gpu/command_buffer/common/gles2_cmd_utils.h"
 #include "gpu/command_buffer/service/context_group.h"
@@ -26,6 +29,7 @@
 #include "gpu/command_buffer/service/program_manager.h"
 #include "gpu/command_buffer/service/test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/abseil-cpp/absl/cleanup/cleanup.h"
 #include "ui/gl/gl_implementation.h"
 #include "ui/gl/gl_mock.h"
 #include "ui/gl/gl_surface_stub.h"
@@ -828,6 +832,93 @@
   EXPECT_EQ(GL_NO_ERROR, GetGLError());
 }
 
+TEST_P(GLES3DecoderManualInitTest, ReadPixelsR16ExtZeroSizeValidationOOB) {
+  InitState init;
+  init.extensions = "GL_EXT_texture_norm16";
+  init.gl_version = "OpenGL ES 3.0";
+  init.context_type = CONTEXT_TYPE_OPENGLES3;
+  InitDecoder(init);
+
+  // Step 1: directly demonstrate the validator/size-calculator desync.
+  uint32_t computed = 0xdeadbeef;
+  EXPECT_TRUE(GLES2Util::ComputeImageDataSizes(64, 64, 1, GL_R16_EXT,
+                                               GL_UNSIGNED_SHORT, 4, &computed,
+                                               nullptr, nullptr));
+  EXPECT_EQ(8192u, computed);
+
+  // Step 2: send a hostile ReadPixels that a compromised renderer could craft.
+  // pixels_shm_offset = end-of-buffer; the decoder validates 0 bytes so this
+  // passes and returns base + kSharedBufferSize (one-past-end).
+  surface_->SetSize(gfx::Size(INT_MAX, INT_MAX));
+  const GLsizei kWidth = 32;
+  const GLsizei kHeight = 32;
+  // Native R16 + UNSIGNED_SHORT = 2 bytes/pixel -> 32*32*2 = 2048 bytes
+  // written by the driver, all of it past the validated buffer end.
+  const uint32_t kPixelsOffset = kSharedBufferSize;  // one-past-end
+
+#if defined(ADDRESS_SANITIZER)
+  // The transfer buffer is mmap'd shared memory (no ASAN redzones). Poison the
+  // page slack past the logical buffer end so ASAN deterministically reports
+  // the OOB write instead of relying on whatever mapping happens to follow.
+  // kSharedBufferSize = 2048; the mapping is at least one 4096-byte page, so
+  // [base+2048, base+4096) is mapped slack we can safely poison.
+
+  // SAFETY: this is the only feasible way to get the pointer we need.
+  // This test is temporary regardless and will be removed along with
+  // the validating command decoder.
+  uint8_t* poison_at = UNSAFE_BUFFERS(
+      static_cast<uint8_t*>(shared_memory_base_.get()) + kSharedBufferSize);
+  static_assert(kSharedBufferSize == 2048);
+  const size_t kPoisonLen = 2048;
+  ASAN_POISON_MEMORY_REGION(poison_at, kPoisonLen);
+  absl::Cleanup unpoison = [poison_at] {
+    ASAN_UNPOISON_MEMORY_REGION(poison_at, kPoisonLen);
+  };
+#endif
+
+  // The decoder falls through to the native-driver IMPLEMENTATION_COLOR_READ_*
+  // query because (GL_R16_EXT, GL_UNSIGNED_SHORT) is not in the hardcoded
+  // accepted list. Simulate an Android native driver that returns the sized
+  // enum (the precondition documented in the report).
+  EXPECT_CALL(*gl_, GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_FORMAT, _))
+      .Times(AnyNumber())
+      .WillRepeatedly(SetArgPointee<1>(GL_R16_EXT));
+  EXPECT_CALL(*gl_, GetIntegerv(GL_IMPLEMENTATION_COLOR_READ_TYPE, _))
+      .Times(AnyNumber())
+      .WillRepeatedly(SetArgPointee<1>(GL_UNSIGNED_SHORT));
+  EXPECT_CALL(*gl_, GetError())
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(GL_NO_ERROR));
+
+  // The "native driver" computes its own write size for R16 + UNSIGNED_SHORT
+  // (2 bytes/pixel) and writes into the pointer the decoder validated for 0
+  // bytes. With kWidth*kHeight*2 = 8192 bytes starting at one-past-end of a
+  // 2048-byte transfer buffer, this is a hard OOB write past the shm mapping.
+  EXPECT_CALL(
+      *gl_, ReadPixels(0, 0, kWidth, kHeight, GL_R16_EXT, GL_UNSIGNED_SHORT, _))
+      .Times(AnyNumber())
+      .WillRepeatedly([](GLint, GLint, GLsizei w, GLsizei h, GLenum, GLenum,
+                         void* pixels) {
+        // What a native R16 glReadPixels does: write w*h*2 bytes.
+
+        // SAFETY: taking a shortcut to fill this memory since this
+        // test is temporary and will be removed soon along with the
+        // validating command decoder.
+        UNSAFE_BUFFERS(memset(pixels, 0xAB, static_cast<size_t>(w) * h * 2u));
+      });
+
+  cmds::ReadPixels cmd;
+  cmd.Init(0, 0, kWidth, kHeight, GL_R16_EXT, GL_UNSIGNED_SHORT,
+           shared_memory_id_, kPixelsOffset, /*result_shm_id=*/0,
+           /*result_shm_offset=*/0, /*async=*/false);
+  // OOB write fires inside this call: ASAN reports a 2048-byte write past the
+  // end of the transfer buffer (the region the decoder validated for 0 bytes).
+  // After the bandaid fix, the command is rejected with GL_INVALID_OPERATION
+  // before reaching glReadPixelsFn.
+  ExecuteCmd(cmd);
+  GetGLError();  // Consume the INVALID_OPERATION emitted by the fixed decoder.
+}
+
 TEST_P(GLES3DecoderTest, ReadPixelsPixelPackBufferIsNotLargeEnough) {
   const GLsizei kWidth = 5;
   const GLsizei kHeight = 3;