From 3fb94f3c5ca73f15a78dbc6904380b9b9e402bf4 Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Fri, 23 Jul 2021 09:23:45 +0100 Subject: [PATCH] dix/privates.c: Avoid undefined behaviour after realloc() Adding the offset between the realloc result and the old allocation to update pointers into the new allocation is undefined behaviour: the old pointers are no longer valid after realloc() according to the C standard. While this works on almost all architectures and compilers, it causes problems on architectures that track pointer bounds (e.g. CHERI or Arm's Morello): the DevPrivateKey pointers will still have the bounds of the previous allocation and therefore any dereference will result in a run-time trap. I found this due to a crash (dereferencing an invalid capability) while trying to run `XVnc` on a CHERI-RISC-V system. With this commit I can successfully connect to the XVnc instance running inside a QEMU with a VNC viewer on my host. This also changes the check whether the allocation was moved to use uintptr_t instead of a pointer since according to the C standard: "The value of a pointer becomes indeterminate when the object it points to (or just past) reaches the end of its lifetime." Casting to an integer type avoids this undefined behaviour. Signed-off-by: Alex Richardson (cherry picked from commit f9f705bf3cf0d169d54a70f235cc99e106dbda43) --- dix/privates.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/dix/privates.c b/dix/privates.c index 384936fbd..71a72fb22 100644 --- a/dix/privates.c +++ b/dix/privates.c @@ -155,14 +155,13 @@ dixMovePrivates(PrivatePtr *privates, int new_offset, unsigned bytes) static Bool fixupOneScreen(ScreenPtr pScreen, FixupFunc fixup, unsigned bytes) { - intptr_t dist; - char *old; + uintptr_t old; char *new; DevPrivateKey *keyp, key; DevPrivateType type; int size; - old = (char *) pScreen->devPrivates; + old = (uintptr_t) pScreen->devPrivates; size = global_keys[PRIVATE_SCREEN].offset; if (!fixup (&pScreen->devPrivates, size, bytes)) return FALSE; @@ -182,9 +181,7 @@ fixupOneScreen(ScreenPtr pScreen, FixupFunc fixup, unsigned bytes) if (fixup == dixMovePrivates) new += bytes; - dist = new - old; - - if (dist) { + if ((uintptr_t) new != old) { for (type = PRIVATE_XSELINUX; type < PRIVATE_LAST; type++) /* Walk the privates list, being careful as the @@ -199,10 +196,11 @@ fixupOneScreen(ScreenPtr pScreen, FixupFunc fixup, unsigned bytes) * is contained within the allocation. Privates * stored elsewhere will be left alone */ - if (old <= (char *) key && (char *) key < old + size) + if (old <= (uintptr_t) key && (uintptr_t) key < old + size) { - /* Compute new location of key */ - key = (DevPrivateKey) ((char *) key + dist); + /* Compute new location of key (deriving from the new + * allocation to avoid UB) */ + key = (DevPrivateKey) (new + ((uintptr_t) key - old)); /* Patch the list */ *keyp = key;