[Pixman] Fwd: [PATCH] Avoid out-of-bounds read when accessing individual bytes from mask
Jonathan Kew
jfkthame at gmail.com
Tue Mar 16 15:07:22 UTC 2021
Added note: the use of a 4-byte memcpy() to read what should just be a
single byte dates from commit 32a55aa8acb4048720e18fbbeaa6c7b398b1a081.
Most of the changes in that commit -- for reading two- or four-byte
values -- were fine, but it inadvertently changed a few cases that were
reading a single byte and made them (over-)read 4 bytes instead.
See for example the change at:
https://gitlab.freedesktop.org/pixman/pixman/-/commit/32a55aa8acb4048720e18fbbeaa6c7b398b1a081#c63c4dd467accbd4a6e99979df218fb52b8a85d4_4862_4865
The presence of the (uint32_t) cast in the old code probably contributed
to the error by making it look misleadingly as though 4 bytes were
wanted. Some of the similar code sections use a uint8_t variable
instead, which avoids this misdirection, which is why I've favored that
option in this patch.
-------- Forwarded Message --------
Subject: [PATCH] Avoid out-of-bounds read when accessing individual
bytes from mask
Date: Tue, 16 Mar 2021 12:43:57 +0000
From: Jonathan Kew <jfkthame at gmail.com>
To: pixman at lists.freedesktop.org
The attached patch fixes an out-of-bounds read error detected by ASAN.
The important changes here are a handful of places where we replace
memcpy(&m, mask++, sizeof(uint32_t));
or similar code with
uint8_t m = *mask++;
because we're only supposed to be reading a single byte from *mask,
and accessing a 32-bit value may read out of bounds (besides that
it reads values we don't actually want; whether this matters would
depend exactly how the value in m is subsequently used).
I've also changed a bunch of other places to use this same pattern
(a local 8-bit variable) when reading individual bytes from the mask;
the code was inconsistent about this, sometimes casting the byte to
a uint32_t instead. This makes no actual difference, it just seemed
better to use a consistent pattern throughout the file.
-------------- next part --------------
From 54162f5ef27bb56e307d35e72c8c9b63b512c575 Mon Sep 17 00:00:00 2001
From: Jonathan Kew <jfkthame at gmail.com>
Date: Tue, 16 Mar 2021 12:25:01 +0000
Subject: [PATCH] Avoid out-of-bounds read when accessing individual bytes from
mask.
The important changes here are a handful of places where we replace
memcpy(&m, mask++, sizeof(uint32_t));
or similar code with
uint8_t m = *mask++;
because we're only supposed to be reading a single byte from *mask,
and accessing a 32-bit value may read out of bounds (besides that
it reads values we don't actually want; whether this matters would
depend exactly how the value in m is subsequently used).
I've also changed a bunch of other places to use this same pattern
(a local 8-bit variable) when reading individual bytes from the mask;
the code was inconsistent about this, sometimes casting the byte to
a uint32_t instead. This makes no actual difference, it just seemed
better to use a consistent pattern throughout the file.
---
pixman/pixman-sse2.c | 45 ++++++++++++++++++++++----------------------
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/pixman/pixman-sse2.c b/pixman/pixman-sse2.c
index 2644b0a..ce4e75f 100644
--- a/pixman/pixman-sse2.c
+++ b/pixman/pixman-sse2.c
@@ -3202,7 +3202,7 @@ sse2_composite_over_n_8_8888 (pixman_implementation_t *imp,
uint8_t *mask_line, *mask;
int dst_stride, mask_stride;
int32_t w;
- uint32_t m, d;
+ uint32_t d;
__m128i xmm_src, xmm_alpha, xmm_def;
__m128i xmm_dst, xmm_dst_lo, xmm_dst_hi;
@@ -3257,6 +3257,7 @@ sse2_composite_over_n_8_8888 (pixman_implementation_t *imp,
while (w >= 4)
{
+ uint32_t m;
memcpy(&m, mask, sizeof(uint32_t));
if (srca == 0xff && m == 0xffffffff)
@@ -3477,7 +3478,6 @@ sse2_composite_src_n_8_8888 (pixman_implementation_t *imp,
uint8_t *mask_line, *mask;
int dst_stride, mask_stride;
int32_t w;
- uint32_t m;
__m128i xmm_src, xmm_def;
__m128i xmm_mask, xmm_mask_lo, xmm_mask_hi;
@@ -3529,6 +3529,7 @@ sse2_composite_src_n_8_8888 (pixman_implementation_t *imp,
while (w >= 4)
{
+ uint32_t m;
memcpy(&m, mask, sizeof(uint32_t));
if (srca == 0xff && m == 0xffffffff)
@@ -3595,7 +3596,6 @@ sse2_composite_over_n_8_0565 (pixman_implementation_t *imp,
uint8_t *mask_line, *mask;
int dst_stride, mask_stride;
int32_t w;
- uint32_t m;
__m128i mmx_src, mmx_alpha, mmx_mask, mmx_dest;
__m128i xmm_src, xmm_alpha;
@@ -3627,7 +3627,7 @@ sse2_composite_over_n_8_0565 (pixman_implementation_t *imp,
while (w && (uintptr_t)dst & 15)
{
- m = *mask++;
+ uint8_t m = *mask++;
if (m)
{
@@ -3647,6 +3647,8 @@ sse2_composite_over_n_8_0565 (pixman_implementation_t *imp,
while (w >= 8)
{
+ uint32_t m;
+
xmm_dst = load_128_aligned ((__m128i*) dst);
unpack_565_128_4x128 (xmm_dst,
&xmm_dst0, &xmm_dst1, &xmm_dst2, &xmm_dst3);
@@ -3700,7 +3702,7 @@ sse2_composite_over_n_8_0565 (pixman_implementation_t *imp,
while (w)
{
- m = *mask++;
+ uint8_t m = *mask++;
if (m)
{
@@ -4062,7 +4064,7 @@ sse2_composite_in_n_8_8 (pixman_implementation_t *imp,
uint8_t *dst_line, *dst;
uint8_t *mask_line, *mask;
int dst_stride, mask_stride;
- uint32_t d, m;
+ uint32_t d;
uint32_t src;
int32_t w;
@@ -4089,7 +4091,7 @@ sse2_composite_in_n_8_8 (pixman_implementation_t *imp,
while (w && ((uintptr_t)dst & 15))
{
- m = (uint32_t) *mask++;
+ uint8_t m = *mask++;
d = (uint32_t) *dst;
*dst++ = (uint8_t) pack_1x128_32 (
@@ -4126,7 +4128,7 @@ sse2_composite_in_n_8_8 (pixman_implementation_t *imp,
while (w)
{
- m = (uint32_t) *mask++;
+ uint8_t m = *mask++;
d = (uint32_t) *dst;
*dst++ = (uint8_t) pack_1x128_32 (
@@ -4303,7 +4305,7 @@ sse2_composite_add_n_8_8 (pixman_implementation_t *imp,
int dst_stride, mask_stride;
int32_t w;
uint32_t src;
- uint32_t m, d;
+ uint32_t d;
__m128i xmm_alpha;
__m128i xmm_mask, xmm_mask_lo, xmm_mask_hi;
@@ -4328,7 +4330,7 @@ sse2_composite_add_n_8_8 (pixman_implementation_t *imp,
while (w && ((uintptr_t)dst & 15))
{
- m = (uint32_t) *mask++;
+ uint8_t m = *mask++;
d = (uint32_t) *dst;
*dst++ = (uint8_t) pack_1x128_32 (
@@ -4364,7 +4366,7 @@ sse2_composite_add_n_8_8 (pixman_implementation_t *imp,
while (w)
{
- m = (uint32_t) *mask++;
+ uint8_t m = (uint32_t) *mask++;
d = (uint32_t) *dst;
*dst++ = (uint8_t) pack_1x128_32 (
@@ -4832,7 +4834,6 @@ sse2_composite_over_x888_8_8888 (pixman_implementation_t *imp,
uint32_t *src, *src_line, s;
uint32_t *dst, *dst_line, d;
uint8_t *mask, *mask_line;
- uint32_t m;
int src_stride, mask_stride, dst_stride;
int32_t w;
__m128i ms;
@@ -4861,8 +4862,8 @@ sse2_composite_over_x888_8_8888 (pixman_implementation_t *imp,
while (w && (uintptr_t)dst & 15)
{
+ uint8_t m = *mask++;
s = 0xff000000 | *src++;
- memcpy(&m, mask++, sizeof(uint32_t));
d = *dst;
ms = unpack_32_1x128 (s);
@@ -4880,6 +4881,7 @@ sse2_composite_over_x888_8_8888 (pixman_implementation_t *imp,
while (w >= 4)
{
+ uint32_t m;
memcpy(&m, mask, sizeof(uint32_t));
xmm_src = _mm_or_si128 (
load_128_unaligned ((__m128i*)src), mask_ff000000);
@@ -4916,7 +4918,7 @@ sse2_composite_over_x888_8_8888 (pixman_implementation_t *imp,
while (w)
{
- memcpy(&m, mask++, sizeof(uint32_t));
+ uint8_t m = *mask++;
if (m)
{
@@ -4957,7 +4959,6 @@ sse2_composite_over_8888_8_8888 (pixman_implementation_t *imp,
uint32_t *src, *src_line, s;
uint32_t *dst, *dst_line, d;
uint8_t *mask, *mask_line;
- uint32_t m;
int src_stride, mask_stride, dst_stride;
int32_t w;
@@ -4986,9 +4987,9 @@ sse2_composite_over_8888_8_8888 (pixman_implementation_t *imp,
while (w && (uintptr_t)dst & 15)
{
uint32_t sa;
+ uint8_t m = *mask++;
s = *src++;
- m = (uint32_t) *mask++;
d = *dst;
sa = s >> 24;
@@ -5019,6 +5020,7 @@ sse2_composite_over_8888_8_8888 (pixman_implementation_t *imp,
while (w >= 4)
{
+ uint32_t m;
memcpy(&m, mask, sizeof(uint32_t));
if (m)
@@ -5058,9 +5060,9 @@ sse2_composite_over_8888_8_8888 (pixman_implementation_t *imp,
while (w)
{
uint32_t sa;
+ uint8_t m = *mask++;
s = *src++;
- m = (uint32_t) *mask++;
d = *dst;
sa = s >> 24;
@@ -5927,13 +5929,11 @@ scaled_bilinear_scanline_sse2_8888_8_8888_OVER (uint32_t * dst,
intptr_t unit_x = unit_x_;
BILINEAR_DECLARE_VARIABLES;
uint32_t pix1, pix2;
- uint32_t m;
while (w && ((uintptr_t)dst & 15))
{
uint32_t sa;
-
- m = (uint32_t) *mask++;
+ uint8_t m = *mask++;
if (m)
{
@@ -5969,6 +5969,8 @@ scaled_bilinear_scanline_sse2_8888_8_8888_OVER (uint32_t * dst,
while (w >= 4)
{
+ uint32_t m;
+
__m128i xmm_src, xmm_src_lo, xmm_src_hi, xmm_srca_lo, xmm_srca_hi;
__m128i xmm_dst, xmm_dst_lo, xmm_dst_hi;
__m128i xmm_mask, xmm_mask_lo, xmm_mask_hi;
@@ -6015,8 +6017,7 @@ scaled_bilinear_scanline_sse2_8888_8_8888_OVER (uint32_t * dst,
while (w)
{
uint32_t sa;
-
- m = (uint32_t) *mask++;
+ uint8_t m = *mask++;
if (m)
{
--
2.30.0
More information about the Pixman
mailing list