From 63220100145f1e1e78b4a2aaad8c014e5cc1af00 Mon Sep 17 00:00:00 2001
From: Corentin Wallez <cwallez@chromium.org>
Date: Mon, 17 Dec 2018 17:08:41 +0000
Subject: [PATCH] Buffer: fix overflows in SetSubData and mapping validation

Also adds tests for overflows.

BUG=chromium:914240

Change-Id: Ibb4f86c04ffab1f1d834b8a439ba542a5291441f
Reviewed-on: https://dawn-review.googlesource.com/c/3320
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
---
 src/dawn_native/Buffer.cpp                    | 18 +++++--
 .../validation/BufferValidationTests.cpp      | 47 ++++++++++++++++++-
 2 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp
index 8027b63f15..887356775a 100644
--- a/src/dawn_native/Buffer.cpp
+++ b/src/dawn_native/Buffer.cpp
@@ -166,8 +166,12 @@ namespace dawn_native {
     }
 
     MaybeError BufferBase::ValidateSetSubData(uint32_t start, uint32_t count) const {
-        // TODO(cwallez@chromium.org): check for overflows.
-        if (start + count > GetSize()) {
+        if (count > GetSize()) {
+            return DAWN_VALIDATION_ERROR("Buffer subdata with too much data");
+        }
+
+        // Note that no overflow can happen because we already checked for GetSize() >= count
+        if (start > GetSize() - count) {
             return DAWN_VALIDATION_ERROR("Buffer subdata out of range");
         }
 
@@ -181,9 +185,13 @@ namespace dawn_native {
     MaybeError BufferBase::ValidateMap(uint32_t start,
                                        uint32_t size,
                                        dawn::BufferUsageBit requiredUsage) const {
-        // TODO(cwallez@chromium.org): check for overflows.
-        if (start + size > GetSize()) {
-            return DAWN_VALIDATION_ERROR("Buffer map read out of range");
+        if (size > GetSize()) {
+            return DAWN_VALIDATION_ERROR("Buffer mapping with too big a region");
+        }
+
+        // Note that no overflow can happen because we already checked for GetSize() >= size
+        if (start > GetSize() - size) {
+            return DAWN_VALIDATION_ERROR("Buffer mapping out of range");
         }
 
         if (mIsMapped) {
diff --git a/src/tests/unittests/validation/BufferValidationTests.cpp b/src/tests/unittests/validation/BufferValidationTests.cpp
index 53f95b7992..a026b740f3 100644
--- a/src/tests/unittests/validation/BufferValidationTests.cpp
+++ b/src/tests/unittests/validation/BufferValidationTests.cpp
@@ -187,6 +187,36 @@ TEST_F(BufferValidationTest, MapWriteOutOfRange) {
     ASSERT_DEVICE_ERROR(buf.MapWriteAsync(0, 5, ToMockBufferMapWriteCallback, userdata));
 }
 
+// Test map reading out of range causes an error, with an overflow
+TEST_F(BufferValidationTest, MapReadOutOfRangeOverflow) {
+    dawn::Buffer buf = CreateMapReadBuffer(4);
+
+    dawn::CallbackUserdata userdata = 40599;
+    EXPECT_CALL(*mockBufferMapReadCallback, Call(DAWN_BUFFER_MAP_ASYNC_STATUS_ERROR, nullptr, userdata))
+        .Times(1);
+
+    // An offset that when added to "2" would overflow to be zero and pass validation without
+    // overflow checks.
+    uint32_t offset = uint32_t(int32_t(0) - int32_t(2));
+
+    ASSERT_DEVICE_ERROR(buf.MapReadAsync(offset, 2, ToMockBufferMapReadCallback, userdata));
+}
+
+// Test map writing out of range causes an error, with an overflow
+TEST_F(BufferValidationTest, MapWriteOutOfRangeOverflow) {
+    dawn::Buffer buf = CreateMapWriteBuffer(4);
+
+    dawn::CallbackUserdata userdata = 40599;
+    EXPECT_CALL(*mockBufferMapWriteCallback, Call(DAWN_BUFFER_MAP_ASYNC_STATUS_ERROR, nullptr, userdata))
+        .Times(1);
+
+    // An offset that when added to "2" would overflow to be zero and pass validation without
+    // overflow checks.
+    uint32_t offset = uint32_t(int32_t(0) - int32_t(2));
+
+    ASSERT_DEVICE_ERROR(buf.MapWriteAsync(offset, 5, ToMockBufferMapWriteCallback, userdata));
+}
+
 // Test map reading a buffer with wrong current usage
 TEST_F(BufferValidationTest, MapReadWrongUsage) {
     dawn::BufferDescriptor descriptor;
@@ -437,8 +467,21 @@ TEST_F(BufferValidationTest, SetSubDataSuccess) {
 TEST_F(BufferValidationTest, SetSubDataOutOfBounds) {
     dawn::Buffer buf = CreateSetSubDataBuffer(1);
 
-    uint8_t foo = 0;
-    ASSERT_DEVICE_ERROR(buf.SetSubData(0, 2, &foo));
+    uint8_t foo[2] = {0, 0};
+    ASSERT_DEVICE_ERROR(buf.SetSubData(0, 2, foo));
+}
+
+// Test error case for SetSubData out of bounds with an overflow
+TEST_F(BufferValidationTest, SetSubDataOutOfBoundsOverflow) {
+    dawn::Buffer buf = CreateSetSubDataBuffer(1000);
+
+    uint8_t foo[2] = {0, 0};
+
+    // An offset that when added to "2" would overflow to be zero and pass validation without
+    // overflow checks.
+    uint32_t offset = uint32_t(int32_t(0) - int32_t(2));
+
+    ASSERT_DEVICE_ERROR(buf.SetSubData(offset, 2, foo));
 }
 
 // Test error case for SetSubData with the wrong usage