[Pixman] Fwd: [PATCH] Avoid out-of-bounds read when accessing individual bytes from mask

[Adam Jackson]

Ugh, my fault there. That explanation all makes a ton of sense, thanks
for the patch! I've opened a merge request for it here:

https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/45

This is probably worth a minor release on its own, if anyone has a
favorite bug or patch they want to see addressed now would be a good
time to get it landed.

- ajax

On Tue, Mar 16, 2021 at 11:07 AM Jonathan Kew <jfkthame at gmail.com> wrote:
>
> 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.
>
> _______________________________________________
> Pixman mailing list
> Pixman at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman