[skcms] Fix crash by reading before CLUT skcms uses an optimization for gather_24 and gather_48 to load 4 or 8 bytes and then throw away a junk byte (instead of individually doing 3 loads, one for r, one for g, one for b). It did this before (added in [1]) by subtracting of 1 or 2 from the pointer This was thought to be safe since there should only be data before this table that is also in the ICC profile (and not an OOB read), however, due to the use of moxcms to parse ICC profiles, the CLUT table can be in its own buffer (e.g. at the beginning of a memory page) and thus we don't have any of the padding we expect before or after it. Instead, we can still have similar performance (and actually save a subtraction op per read) by just reading 4 or 8 bytes at the beginning of the CLUT and then masking off the junk byte (which might *not* actually be needed anyway since the callers extract just the r, g, b values they need). This can now walk off the end of the buffer but there should be padding [2] [3] [1] https://review.skia.org/116740 [2] https://www.color.org/specification/ICC.1-2022-05.pdf [3] https://review.skia.org/1205176 Bug: b/498869813 Bug: b/498927031 Change-Id: I88445d9be9e899e670f2cf9d55e0d2f79446f535 Reviewed-on: https://skia-review.googlesource.com/c/skcms/+/1201671 Commit-Queue: Kaylee Lubick <kjlubick@google.com> Reviewed-by: Florin Malita <fmalita@google.com>
diff --git a/src/Transform_inl.h b/src/Transform_inl.h index 0d48337..88d6c87 100644 --- a/src/Transform_inl.h +++ b/src/Transform_inl.h
@@ -493,16 +493,12 @@ } SI U32 gather_24(const uint8_t* p, I32 ix) { - // First, back up a byte. Any place we're gathering from has a safe junk byte to read - // in front of it, either a previous table value, or some tag metadata. - p -= 1; - // Load the i'th 24-bit value from p, and 1 extra byte. auto load_24_32 = [p](int i) { return load<uint32_t>(p + 3*i); }; - // Now load multiples of 4 bytes (a junk byte, then r,g,b). + // Now load multiples of 4 bytes (r,g,b, then a junk byte). #if N == 1 U32 v = load_24_32(ix); #elif N == 4 @@ -530,15 +526,12 @@ U32 v = (U32)_mm512_i32gather_epi32((__m512i)(3*ix), p4, 1); #endif - // Shift off the junk byte, leaving r,g,b in low 24 bits (and zero in the top 8). - return v >> 8; + // Mask off the junk byte, leaving r,g,b in low 24 bits. + return v & 0x00FFFFFF; } #if !defined(__arm__) SI void gather_48(const uint8_t* p, I32 ix, U64* v) { - // As in gather_24(), with everything doubled. - p -= 2; - // Load the i'th 48-bit value from p, and 2 extra bytes. auto load_48_64 = [p](int i) { return load<uint64_t>(p + 6*i); @@ -589,7 +582,7 @@ store((char*)v + 64, hi); #endif - *v >>= 16; + *v &= 0x0000FFFFFFFFFFFFULL; } #endif
diff --git a/tests.c b/tests.c index bb20401..76e5800 100644 --- a/tests.c +++ b/tests.c
@@ -18,6 +18,11 @@ #include <stdlib.h> #include <string.h> +#if !defined(_MSC_VER) + #include <sys/mman.h> + #include <unistd.h> +#endif + #if defined(_MSC_VER) #define DEBUGBREAK __debugbreak #elif defined(__clang__) @@ -2139,6 +2144,77 @@ free(ptr); } +static void test_CLUT_PageBoundary(void) { +#if !defined(_MSC_VER) + // This test ensures that skcms does not read memory before the provided CLUT buffer. + // In the past, skcms used a "junk byte" optimization to load 3-byte or 6-byte values + // by backing up the pointer by 1-2 bytes to perform an aligned 4-byte or 8-byte load. + // + // This optimization is dangerous if the buffer is allocated exactly at the start of + // a memory page, as backing up will access the previous page. If that page is + // unmapped (a common technique for detecting buffer overflows), it causes a crash. + + // 1. Get the system's memory page size (usually 4KB, 16KB, or 64KB). + size_t pagesize = (size_t)sysconf(_SC_PAGESIZE); + + // 2. Allocate two contiguous pages of memory. + // [ Page 1 (valid) ] [ Page 2 (valid) ] + void* ptr = mmap(NULL, 2 * pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (MAP_FAILED == ptr) { + return; + } + + // Initialize Page 2 so it is actually allocated and mapped by the OS. + memset((char*)ptr + pagesize, 0, pagesize); + + // 3. Unmap the first page. + // [ ABYSS (unmapped) ] [ Page 2 (valid) ] + // Any access to Page 1 will now trigger an immediate Segmentation Fault. + munmap(ptr, pagesize); + + // 4. Place our CLUT data at the very beginning of the second page. + // This means Page 1 (the abyss) is immediately before 'grid'. + void* grid = (char*)ptr + pagesize; + + skcms_ICCProfile profile; + skcms_Init(&profile); + profile.has_A2B = true; + profile.A2B.input_channels = 3; + profile.A2B.output_channels = 3; + profile.A2B.grid_points[0] = 2; + profile.A2B.grid_points[1] = 2; + profile.A2B.grid_points[2] = 2; + + // Use identity-ish curves to focus the test on the CLUT sampling code. + for (int i = 0; i < 3; ++i) { + profile.A2B.input_curves[i].table_entries = 0; + profile.A2B.input_curves[i].parametric = *skcms_Identity_TransferFunction(); + profile.A2B.output_curves[i].table_entries = 0; + profile.A2B.output_curves[i].parametric = *skcms_Identity_TransferFunction(); + } + + float src[3] = { 0, 0, 0 }; + float dst[3]; + + // 5. Test 16-bit CLUT (uses gather_48). + // This used to crash on the old implementation which read the previous page. + profile.A2B.grid_16 = (const uint8_t*)grid; + profile.A2B.grid_8 = NULL; + expect(skcms_Transform(src, skcms_PixelFormat_RGB_fff, skcms_AlphaFormat_Unpremul, &profile, + dst, skcms_PixelFormat_RGB_fff, skcms_AlphaFormat_Unpremul, NULL, 1)); + + // 6. Test 8-bit CLUT (uses gather_24). + // This used to crash on the old implementation which read the previous page. + profile.A2B.grid_8 = (const uint8_t*)grid; + profile.A2B.grid_16 = NULL; + expect(skcms_Transform(src, skcms_PixelFormat_RGB_fff, skcms_AlphaFormat_Unpremul, &profile, + dst, skcms_PixelFormat_RGB_fff, skcms_AlphaFormat_Unpremul, NULL, 1)); + + // Cleanup Page 2. + munmap(grid, pagesize); +#endif +} + static void test_B2A(void) { void* ptr; size_t len; @@ -2242,6 +2318,7 @@ test_RGBA_8888_sRGB(); test_ParseWithA2BPriority(); test_B2A(); + test_CLUT_PageBoundary(); test_Parse(regenTestData); test_sRGB_AllBytes();