glx: fix DrawableGone use-after-free without rejecting duplicate drawables - #3329
Conversation
…rawableGone)" This reverts commit 5d96fc0.
…ables A window can have more than one __GLXdrawable registered under its X id. On teardown DrawableGone() freed the partner with FreeResourceByType(), which matches only id+type and so frees an arbitrary one, leaving the survivor dangling -> later use-after-free (issue X11Libre#1491). Add FreeResourceByTypeValue() (internal, dix/resource_priv.h) that also matches the value, and use it in DrawableGone() to free the entry for this exact drawable. Fixes the UAF while keeping duplicate glXCreateWindow working, unlike the previous fix that rejected it and broke clients. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Kiyoshi Spreclerg <kiyoshi_pip@protonmail.com>
PR #3329 Test ReportI built and installed PR #3329 at commit Hardware and software:
I compiled and ran the minimal reproducer linked from issue #1491: cc -g -O0 -o minimal minimal.c -lX11 -lGL
./minimalResult:
I also tested with software rendering: LIBGL_ALWAYS_SOFTWARE=1 ./minimalResult:
I could not reproduce the Xorg crash on this Intel Iris Xe system with either This is not confirmation that the issue is fixed universally, but the |
stefan11111
left a comment
There was a problem hiding this comment.
A bit hacky, but lgtm. I can't test the patch yet, but it seems others did. If any further changes are made, don't forget to keep the revert commit clean for bisection purposes.
|
@X11Libre/dev @kiyoshispreclerg shall this be backported ? |
SummaryFixes a use-after-free in GLX Backport assessmentBackport candidate (critical bugfix). This is a client-triggerable use-after-free (memory safety / crash). Likely applies to NVIDIA driver ABINo ABI impact. The change adds one new VerdictClean. Approve for master and recommend backport to all active release lines. |
|
@metux @X11Libre/dev Why would clients be hitting this code path though? Not that it matters for this patch. If clients do hit this, we can't break it. As for the backports, I say we apply this where the broken commit was applied, and we leave things as-is (at least until this patch gets more testing) for the releases that don't contain the broken commit this series is reverting. |
Only master had the broken commit now reverted, but I think backporting the other can be valid if the triggerable UAF can be exploited. Of course, wait for more testing is a very good idea. |
This draft has another approach to fix #1491 while allowing duplicate GLX drawables for a window. Instead of rejecting their creation, we added a new function that frees exactly them, as the crash was happening in their teardown, not creation.
I'm no expert and can't guarantee this code is bulletproof, but in the only machine mine that can trigger the crash from #1491, the code in this PR makes not crash anymore, while still allowing clients broken by #3145 to keep working, even if they're working "wrongly" :)
This could fix #3284 and its duplicates.
Backport dashboard
release/25.2release/25.1release/25.0