High chrome Integer Overflow 📄 Reporter bug report 🔧 Commit mapped

Overview

High
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
ImpactInteger overflow in Skia
DescriptionInteger overflow in Skia
ComponentSkia
Bug ClassInteger Overflow
Tracker382786791
Fix commit8b030e47588a (skia) +18/-5
CISA KEVNot listed
CreditedHan Zheng (HexHive)
Disclosed2025-01-14

Changed Functions

FunctionChangeNotes
for
src/gpu/ganesh/ops/AAHairLinePathRenderer.cpp
modified

Files Changed

  • src/gpu/ganesh/ops/AAHairLinePathRenderer.cpp
From 8b030e47588af50f56ef380d81a17667baeb582b Mon Sep 17 00:00:00 2001
From: James Godfrey-Kittle <jamesgk@google.com>
Date: Tue, 17 Dec 2024 12:14:17 -0500
Subject: [PATCH] [ganesh] Avoid overflow when combining AAHairlineOps

Bug: b/382786791
Change-Id: I955d943015cce76f75221df9fab0897a6f22fe4b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/930577
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: James Godfrey-Kittle <jamesgk@google.com>
---

diff --git a/src/gpu/ganesh/ops/AAHairLinePathRenderer.cpp b/src/gpu/ganesh/ops/AAHairLinePathRenderer.cpp
index 3527904..2c04f31 100644
--- a/src/gpu/ganesh/ops/AAHairLinePathRenderer.cpp
+++ b/src/gpu/ganesh/ops/AAHairLinePathRenderer.cpp
@@ -27,6 +27,7 @@
 #include "include/private/base/SkPoint_impl.h"
 #include "include/private/base/SkTArray.h"
 #include "include/private/gpu/ganesh/GrTypesPriv.h"
+#include "src/base/SkSafeMath.h"
 #include "src/core/SkGeometry.h"
 #include "src/core/SkMatrixPriv.h"
 #include "src/core/SkPointPriv.h"
@@ -1219,16 +1220,28 @@
 
     int instanceCount = fPaths.size();
     bool convertConicsToQuads = !target->caps().shaderCaps()->fFloatIs32Bits;
-    for (int i = 0; i < instanceCount; i++) {
+    SkSafeMath safeMath;
+    for (int i = 0; i < instanceCount && safeMath.ok(); i++) {
         const PathData& args = fPaths[i];
-        quadCount += gather_lines_and_quads(args.fPath, args.fViewMatrix, args.fDevClipBounds,
-                                            args.fCapLength, convertConicsToQuads, &lines, &quads,
-                                            &conics, &qSubdivs, &cWeights);
+        quadCount = safeMath.addInt(quadCount,
+                                    gather_lines_and_quads(args.fPath,
+                                                           args.fViewMatrix,
+                                                           args.fDevClipBounds,
+                                                           args.fCapLength,
+                                                           convertConicsToQuads,
+                                                           &lines,
+                                                           &quads,
+                                                           &conics,
+                                                           &qSubdivs,
+                                                           &cWeights));
     }
 
     int lineCount = lines.size() / 2;
     int conicCount = conics.size() / 3;
-    int quadAndConicCount = conicCount + quadCount;
+    int quadAndConicCount = safeMath.addInt(conicCount, quadCount);
+    if (!safeMath.ok()) {
+        return;
+    }
 
     static constexpr int kMaxLines = SK_MaxS32 / kLineSegNumVertices;
     static constexpr int kMaxQuadsAndConics = SK_MaxS32 / kQuadNumVertices;
Loading diff…

Original Bug Report

reported by kd...@gmail.com

Security: Skia integer overflow (results in OOB) at AAHairlineOp::onPrepareDraws

Steps to reproduce the problem

  1. [optional] apply skia patch (only accelerate the computation to enhance bug reproduce)
  2. build skpbench with ubsan
  3. run script to generate path.skp and run ./skpbench –src poc/path.skp –config gles

Problem Description

In function AAHairlineOp::onPrepareDraws, the quadCount iterate every path and add gather_lines_and_quads, which can be controlled by fPath. by creating crafted skia path, we’re able to increase the gather_lines_and_quads results to 0xf0f4 in the poc. and there are no additional verification for the quadCount to prevent overflow.

    int instanceCount = fPaths.size();
    bool convertConicsToQuads = !target->caps().shaderCaps()->fFloatIs32Bits;
    for (int i = 0; i < instanceCount; i++) {
        const PathData& args = fPaths[i];
        int quadCount += gather_lines_and_quads(args.fPath, args.fViewMatrix, args.fDevClipBounds,
                                             args.fCapLength, convertConicsToQuads, &lines, &quads,
                                             &conics, &qSubdivs, &cWeights);
        // [1] gather_lines_and_quads can be super large (e.g. 0xf0f4)
        // therefore the quadCount may overflow
    }

Subsequently, the quadCount is used for allocation at [2-3], endup being used to allocate buffer at [4]. Therefore, the OOB will happen at [5]


    int quadAndConicCount = conicCount + quadCount;          //[2]

    static constexpr int kMaxLines = SK_MaxS32 / kLineSegNumVertices;
    static constexpr int kMaxQuadsAndConics = SK_MaxS32 / kQuadNumVertices;
    if (lineCount > kMaxLines || quadAndConicCount > kMaxQuadsAndConics) {
        return;
    }

        int vertexCount = kQuadNumVertices * quadAndConicCount; // [3]
        void* vertices = target->makeVertexSpace(sizeof(BezierVertex), vertexCount, &vertexBuffer,
                                                 &firstVertex); // [4]

        if (!vertices || !quadsIndexBuffer) {
            SkDebugf("Could not allocate vertices\n");
            return;
        }

        BezierVertex* bezVerts = reinterpret_cast<BezierVertex*>(vertices);

        int unsubdivQuadCnt = quads.size() / 3;
        for (int i = 0; i < unsubdivQuadCnt; ++i) {
            SkASSERT(qSubdivs[i] >= 0);
            if (!quads[3*i].isFinite() || !quads[3*i+1].isFinite() || !quads[3*i+2].isFinite()) {
                return;
            }
            add_quads(&quads[3*i], qSubdivs[i], toDevice, toSrc, &bezVerts); //[5]
        }

Summary

Security: Skia integer overflow (results in OOB) at AAHairlineOp::onPrepareDraws

Custom Questions

Type of crash:

gpu

Reporter credit:

Han Zheng (HexHive)

Additional Data

Category: Security
Chrome Channel: Not sure
Regression: N/A

View on issue tracker