From 1647420993b8f376e58c7dc5ed15551c3b8fd8d0 Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Wed, 29 Jul 2026 20:05:45 -0300 Subject: [PATCH] drm: close the full-review findings (a third latched flag, and two escapees) The one that matters: the display-cache refresh worker was the THIRD copy of the wedged-flag hazard. catch_unwind covered only the enumeration, and thread::spawn panics on EAGAIN after RUNNING was already swapped true, so either path parked the flag for the process lifetime and every later refresh - including every udev hotplug - returned early forever. Same ownership guard as UINPUT_REFRESH_BUSY (the flag is handed back and re-taken mid-loop, so an unconditional RAII release would clear a replacement worker's flag), plus a fallible spawn whose failure drops the closure and releases the slot. DRM_PROBE_IN_FLIGHT, UINPUT_REFRESH_BUSY, now this: the lesson stays 'grep for every site with the shape', and twice was not enough. Two findings had been flagged in an earlier round and escaped the ledger: - an unrecognized convert-output fourcc fell through to 'present as BGRA' with a debug log, where every sibling validation in that function is a hard error that lets the caller fall back to PipeWire. A 64bpp output passes the stride check and encodes garbage. Hard error now. - the trust-boundary validation constants (fourccs, MAX_DIM, MAX_FRAME_BYTES) were declared independently on both sides of the split. Hoisted into drm_reader, imported by the converter, so the two halves cannot drift apart about what data they will touch. The rest: - the CI symbol extraction dropped any loader symbol containing a digit and degraded to a pass-with-zero-iterations no-op if the b"..." literals were ever refactored; digits allowed, count asserted, notice de-hardcoded. - 'drm' in features was a substring test on the comma-joined string, so a future drm-lease feature would have shipped the consent-bypass deb without --drm. Exact membership now. - the security doc claimed the deb is built on an ubuntu18.04 container; the only deb job runs on ubuntu-24.04. The 18.04 sentence now says what is true: 2.4.95 is an API floor, the binary floor is the build host's. - DRM_DISPLAY_CACHE poison handling was recover-in-the-writer, panic-in-the-readers; both readers now recover like the writer. - the producer prewarm ran on X11 where no consumer can connect, the same inconsistency just fixed for warm_availability. The listener still starts (the service outlives sessions; a later Wayland login must find the socket), only the prewarm is skipped. --- .github/workflows/drm-capture.yml | 18 +++- build.py | 5 +- docs/DRM_CAPTURE_SECURITY.md | 11 +-- libs/scrap/src/common/drm_reader.rs | 21 +++-- libs/scrap/src/common/drm_render.rs | 38 ++++---- src/ipc/drm.rs | 131 ++++++++++++++++++++-------- 6 files changed, 151 insertions(+), 73 deletions(-) diff --git a/.github/workflows/drm-capture.yml b/.github/workflows/drm-capture.yml index e25d0e292..e8b4b4ae6 100644 --- a/.github/workflows/drm-capture.yml +++ b/.github/workflows/drm-capture.yml @@ -172,9 +172,19 @@ jobs: SO="$(cat so_path)" echo "checking $SO" missing=0 - # Every symbol drmtap_dl.rs resolves, derived from the loader itself so the two cannot drift. - for sym in $(grep -oE 'b"drmtap_[a-z_]+"' libs/scrap/src/common/drmtap_dl.rs \ - | sed 's/^b"//; s/"$//' | sort -u); do + # Every symbol drmtap_dl.rs resolves, derived from the loader itself so the two cannot + # drift. The character class allows digits (a drmtap_grab_desc2 would otherwise be + # silently dropped from the loop), and the count is asserted below so a refactor of the + # loader away from b"..." literals cannot quietly turn this whole check into a no-op that + # iterates zero times and passes. + syms=$(grep -oE 'b"drmtap_[a-z0-9_]+"' libs/scrap/src/common/drmtap_dl.rs \ + | sed 's/^b"//; s/"$//' | sort -u) + nsyms=$(echo "$syms" | grep -c .) + if [ "$nsyms" -lt 13 ]; then + echo "::error::extracted only $nsyms loader symbols from drmtap_dl.rs (expected >= 13); the extraction pattern no longer matches the loader" + missing=1 + fi + for sym in $syms; do if ! nm -D --defined-only "$SO" | grep -q " T $sym\$"; then echo "::error::libdrmtap does not export $sym, which the runtime loader resolves" missing=1 @@ -191,7 +201,7 @@ jobs: fi done test "$missing" -eq 0 - echo "::notice::libdrmtap .so contract ok (13 loader symbols, EGL detile present)" + echo "::notice::libdrmtap .so contract ok ($nsyms loader symbols, EGL detile present)" # The bridge generator is a reusable workflow, so this calls the stock one instead of duplicating it. generate-bridge: diff --git a/build.py b/build.py index 350a937a2..14a258b3e 100755 --- a/build.py +++ b/build.py @@ -565,7 +565,10 @@ def build_flutter_deb(version, features): "echo \"#!/bin/sh\" >> tmpdeb/usr/share/rustdesk/files/polkit && chmod a+x tmpdeb/usr/share/rustdesk/files/polkit") # Bundle libdrmtap.so only when this build actually enabled the `drm` feature, so stock packages # stay exactly what they were. The root service dlopens it in-process by absolute path. - ships_so = 'drm' in features + # `features` is the comma-joined string, so split it: a bare substring test would also match any + # future feature merely containing "drm" (drm-lease, vaapi-drm) and rename the deb to the + # consent-bypass variant without --drm ever being passed. + ships_so = 'drm' in features.split(',') if ships_so: stage_libdrmtap_into_deb(build_libdrmtap_so()) diff --git a/docs/DRM_CAPTURE_SECURITY.md b/docs/DRM_CAPTURE_SECURITY.md index 0c2804529..61306ee04 100644 --- a/docs/DRM_CAPTURE_SECURITY.md +++ b/docs/DRM_CAPTURE_SECURITY.md @@ -132,11 +132,12 @@ unprivileged one presents) but reuses RustDesk's own hardened IPC. the capture runs inside the root `--service`, which already holds the capability it needs. Hosts without `/dev/dri` access (or where the library fails to load) transparently fall back to the PipeWire/portal path. -- **Minimum OS: Ubuntu 18.04 (or equivalent, libdrm ≥ 2.4.95).** `libdrmtap` needs the DRM - `GetFB2` framebuffer API (libdrm 2.4.95); Ubuntu 18.04 ships 2.4.101, so 18.04 is the floor. The - `rustdesk-unattended-wayland` deb is built and packaged on an ubuntu18.04 container in CI (a - build-time compatibility check only — DRM capture itself is not installed or exercised there), so - it is built against the 18.04 toolchain and libraries and is compatible with 18.04 and newer. +- **Minimum libdrm: 2.4.95 (Ubuntu 18.04 or equivalent).** `libdrmtap` needs the DRM + `GetFB2` framebuffer API (libdrm 2.4.95); Ubuntu 18.04 ships 2.4.101, so every supported + distribution satisfies the API floor. That is an API statement, not a binary-compatibility one: + the `rustdesk-unattended-wayland` deb in this repo's CI is built on an ubuntu-24.04 runner, so the + shipped binaries carry that build host's glibc floor. Running on an older distribution means + building the deb there (or in a matching container), which the libdrm floor above permits. Capture also requires an active KMS scanout (a Wayland/KMS session with a display on); on hosts where the compositor drives the display outside DRM/KMS (e.g. the proprietary NVIDIA X11 stack) there is no capturable CRTC and the path falls back to PipeWire/portal. diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index 3de240af1..d1398a357 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -18,9 +18,22 @@ use std::ffi::CString; use std::io; use std::os::fd::{FromRawFd, OwnedFd}; +// The validation limits and pixel formats BOTH halves of the split rely on to agree about what data +// they will touch. They live here, once, and `drm_render` (the unprivileged converter) imports them: +// these are trust-boundary guards, so two independently-edited copies that drift apart would silently +// weaken validation on one side of the boundary. +// // Largest scanout we will copy; also bounds w*4*h against overflow. 16384 covers // 8K+ with headroom; anything larger is rejected as a bogus/hostile geometry. -const MAX_DIM: u32 = 16384; +pub(crate) const MAX_DIM: u32 = 16384; +// 256 MiB covers an 8K BGRA frame (7680x4320x4 ~= 127 MiB) with margin. +pub(crate) const MAX_FRAME_BYTES: usize = 256 * 1024 * 1024; +// DRM fourccs of the 32-bit linear formats the split can carry. XRGB/ARGB are little-endian +// B,G,R,{X,A} in memory == `Pixfmt::BGRA`; XBGR/ABGR are R,G,B,{X,A} == `Pixfmt::RGBA`. +pub(crate) const DRM_FORMAT_XRGB8888: u32 = 0x3432_5258; // 'XR24' +pub(crate) const DRM_FORMAT_ARGB8888: u32 = 0x3432_5241; // 'AR24' +pub(crate) const DRM_FORMAT_XBGR8888: u32 = 0x3432_4258; // 'XB24' +pub(crate) const DRM_FORMAT_ABGR8888: u32 = 0x3432_4241; // 'AB24' /// Sentinel cursor id published when the plane reports the cursor hidden, so the /// id changes and the client drops the last shape. Distinct from any real hash. @@ -220,8 +233,6 @@ impl DrmReader { // XBGR8888 passes the stride check above but, labeled BGRA downstream, would ship with red // and blue swapped — so reject any fourcc we cannot present as BGRA. A zero/unknown fourcc // falls through to the stride invariant (kept for libdrmtap builds that do not set it). - const DRM_FORMAT_XRGB8888: u32 = 0x3432_5258; // 'XR24' - const DRM_FORMAT_ARGB8888: u32 = 0x3432_5241; // 'AR24' if frame.format != 0 && frame.format != DRM_FORMAT_XRGB8888 && frame.format != DRM_FORMAT_ARGB8888 @@ -241,8 +252,8 @@ impl DrmReader { // would otherwise resize to gigabytes and, with several concurrent readers, OOM the root // --service. 256 MiB covers an 8K BGRA scanout (7680x4320x4 ~= 127 MiB) with margin; // anything larger (or an overflow) is rejected as unsupported. checked_mul guards the - // multiply on 32-bit usize too. - const MAX_FRAME_BYTES: usize = 256 * 1024 * 1024; + // multiply on 32-bit usize too. MAX_FRAME_BYTES is the file-level shared limit, the + // same one the converter enforces on its side of the boundary. let frame_size = match w.checked_mul(4).and_then(|x| x.checked_mul(h)) { Some(sz) if sz > 0 && sz <= MAX_FRAME_BYTES => sz, other => { diff --git a/libs/scrap/src/common/drm_render.rs b/libs/scrap/src/common/drm_render.rs index 817320d74..1218de1ba 100644 --- a/libs/scrap/src/common/drm_render.rs +++ b/libs/scrap/src/common/drm_render.rs @@ -22,22 +22,15 @@ use std::ffi::CString; use std::io; use std::os::fd::RawFd; -// DRM fourccs of the 32-bit linear formats libdrmtap's convert can emit. XRGB/ARGB -// are little-endian B,G,R,{X,A} in memory == our `Pixfmt::BGRA`; XBGR/ABGR are -// R,G,B,{X,A} == `Pixfmt::RGBA`. libdrmtap normalizes the EGL path to XRGB8888, but -// we read `frame.format` per frame so a CPU-fallback convert that keeps the source -// channel order is still presented correctly (not hardcoded BGRA). -const DRM_FORMAT_XRGB8888: u32 = 0x3432_5258; // 'XR24' -const DRM_FORMAT_ARGB8888: u32 = 0x3432_5241; // 'AR24' -const DRM_FORMAT_XBGR8888: u32 = 0x3432_4258; // 'XB24' -const DRM_FORMAT_ABGR8888: u32 = 0x3432_4241; // 'AB24' - -// Same geometry / size guards as the export side (`drm_reader`), applied to the -// convert OUTPUT so a malformed `frame_info` cannot make us build an out-of-range -// slice from the context-owned pointer. 16384 covers 8K+; 256 MiB covers an 8K -// BGRA frame (7680x4320x4 ~= 127 MiB) with margin. -const MAX_DIM: u32 = 16384; -const MAX_FRAME_BYTES: usize = 256 * 1024 * 1024; +// The geometry/size limits and pixel fourccs are SHARED with the export side, declared once in +// `drm_reader`: they are the guards both halves of the trust boundary rely on to agree about what +// data they will touch, so a private copy here could silently drift from the privileged side's. +// libdrmtap normalizes the EGL path to XRGB8888, but we read `frame.format` per frame so a +// CPU-fallback convert that keeps the source channel order is still presented correctly. +use super::drm_reader::{ + DRM_FORMAT_ABGR8888, DRM_FORMAT_ARGB8888, DRM_FORMAT_XBGR8888, DRM_FORMAT_XRGB8888, + MAX_DIM, MAX_FRAME_BYTES, +}; /// An unprivileged DRM render-node convert context (`drmtap_open_render`). Imports a /// scanout dma-buf (received over SCM_RIGHTS) and EGL-detiles it to linear pixels. @@ -187,11 +180,16 @@ impl RenderConverter { DRM_FORMAT_XBGR8888 | DRM_FORMAT_ABGR8888 => Pixfmt::RGBA, // Unset by an older convert -> libdrmtap's normalized BGRA. 0 => Pixfmt::BGRA, + // Every other invalid frame_info property in this function is a hard error + // that lets the caller fall back to PipeWire; an output format this build + // cannot interpret must be one too. Presenting it as BGRA would pass the + // stride checks (a 64bpp output still satisfies stride >= w*4) and encode + // garbage instead of degrading. other => { - log::debug!( - "drm: convert output fourcc {other:#010x} unrecognized; presenting as BGRA" - ); - Pixfmt::BGRA + return Err(io::Error::new( + io::ErrorKind::Other, + format!("drmtap_convert_dmabuf produced an unsupported output fourcc {other:#010x}"), + )); } }; // Borrow the context-owned pixels. The returned lifetime is tied to `&mut self` diff --git a/src/ipc/drm.rs b/src/ipc/drm.rs index fde747e9e..30b53aa2f 100644 --- a/src/ipc/drm.rs +++ b/src/ipc/drm.rs @@ -314,49 +314,84 @@ fn schedule_drm_cache_refresh() { use std::sync::atomic::{AtomicBool, Ordering}; static RUNNING: AtomicBool = AtomicBool::new(false); static PENDING: AtomicBool = AtomicBool::new(false); + // Ownership of RUNNING, released on every exit including an unwind and a failed spawn. Same + // shape as UinputRefreshGuard in drm_capturer: the flag is deliberately handed back and + // re-taken mid-loop, so the guard tracks whether WE still hold it -- an unconditional release + // on drop would clear a flag a replacement worker owns. Without this, a panic anywhere in the + // loop body outside the catch_unwind below, or `thread::spawn` itself failing (it panics on + // EAGAIN, and that happens AFTER the swap), leaves RUNNING true for the process lifetime and + // every later refresh -- including every udev hotplug -- returns early forever. + struct RefreshSlot(bool); + impl RefreshSlot { + fn release(&mut self) { + if self.0 { + self.0 = false; + RUNNING.store(false, Ordering::Release); + } + } + fn retake(&mut self) -> bool { + self.0 = !RUNNING.swap(true, Ordering::AcqRel); + self.0 + } + } + impl Drop for RefreshSlot { + fn drop(&mut self) { + self.release(); + } + } // Announce a refresh is wanted before trying to run, so an active worker is guaranteed to see it. PENDING.store(true, Ordering::Release); if RUNNING.swap(true, Ordering::AcqRel) { return; // a worker is already active; it will observe PENDING and refresh again } - std::thread::spawn(|| loop { - PENDING.store(false, Ordering::Release); - // Panic-safety: enumeration must not be able to leave RUNNING stuck true (which would wedge - // every future refresh). Catch a panic here and the cache lock is recovered from poison - // below, so the RUNNING/PENDING bookkeeping always runs. - let fresh = std::panic::catch_unwind(drm_enumerate_all_displays).unwrap_or_else(|_| { - log::error!("drm: display enumeration panicked; treating as no displays"); - Vec::new() - }); - let changed = { - let mut cache = match DRM_DISPLAY_CACHE.lock() { - Ok(g) => g, - Err(poisoned) => poisoned.into_inner(), + // We hold the slot from the swap above; hand it to the guard NOW, before the spawn, so a spawn + // failure releases it too (the closure that owns it is dropped along with the error). + let mut slot = RefreshSlot(true); + let spawned = std::thread::Builder::new() + .name("drm-cache-refresh".into()) + .spawn(move || loop { + PENDING.store(false, Ordering::Release); + // Panic-safety, two layers: enumeration panics are caught here so a flaky driver does + // not lose the refresh; anything else that unwinds is covered by `slot`'s Drop. + let fresh = std::panic::catch_unwind(drm_enumerate_all_displays).unwrap_or_else(|_| { + log::error!("drm: display enumeration panicked; treating as no displays"); + Vec::new() + }); + let changed = { + let mut cache = match DRM_DISPLAY_CACHE.lock() { + Ok(g) => g, + Err(poisoned) => poisoned.into_inner(), + }; + if *cache != fresh { + *cache = fresh; + true + } else { + false + } }; - if *cache != fresh { - *cache = fresh; - true - } else { - false + DRM_CACHE_WARMED.store(true, Ordering::Release); + if changed { + DRM_DISPLAY_GENERATION.fetch_add(1, Ordering::Release); + log::info!("drm: display cache refreshed (topology changed)"); } - }; - DRM_CACHE_WARMED.store(true, Ordering::Release); - if changed { - DRM_DISPLAY_GENERATION.fetch_add(1, Ordering::Release); - log::info!("drm: display cache refreshed (topology changed)"); - } - // Exit only if no request arrived during this enumeration. The re-check after releasing - // RUNNING closes the lost-wakeup window (a request that set PENDING just before the release). - if !PENDING.load(Ordering::Acquire) { - RUNNING.store(false, Ordering::Release); + // Exit only if no request arrived during this enumeration. The re-check after releasing + // the slot closes the lost-wakeup window (a request that set PENDING just before the + // release). if !PENDING.load(Ordering::Acquire) { - break; + slot.release(); + if !PENDING.load(Ordering::Acquire) { + break; + } + if !slot.retake() { + break; // another caller re-acquired the slot; it will handle the pending refresh + } } - if RUNNING.swap(true, Ordering::AcqRel) { - break; // another caller re-acquired the slot; it will handle the pending refresh - } - } - }); + }); + if let Err(err) = spawned { + // The closure was dropped without running, and the guard inside it released RUNNING, so the + // next request retries the spawn instead of being locked out forever. + log::error!("drm: could not spawn the display-cache refresh worker: {err}"); + } } /// True if a kernel uevent datagram is a DRM-subsystem topology change (a connector hotplug/modeset). @@ -511,8 +546,18 @@ pub async fn start_drm() { match new_drm_listener() { Ok(mut incoming) => { // Warm libdrmtap/EGL + enumeration off-thread so the first consumer does not pay that - // one-time cost on its critical path. - std::thread::spawn(drm_prewarm); + // one-time cost on its critical path. Skipped when the current session is X11 -- every + // consumer gates on !is_x11(), so none will connect, and the prewarm opens a DrmReader + // and grabs a frame in the root service for nothing. The LISTENER below still starts + // either way: the root service outlives sessions, a later Wayland login must find the + // `_drm` socket, and its first connection re-enumerates on demand (the handshake serves + // a throwaway enumeration when the cache is cold), so skipping the prewarm costs that + // session only the one-time warmup the prewarm exists to hide. + if !scrap::is_x11() { + std::thread::spawn(drm_prewarm); + } else { + log::info!("drm: X11 session; skipping the pre-warm (the _drm listener still runs)"); + } // Watch for connector hotplug/modeset uevents so a mid-session topology change refreshes // the display cache and is pushed to live consumers (best-effort; own thread since it // blocks on recv and re-enumeration is a blocking `!Send` open). @@ -807,7 +852,13 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { let gen = DRM_DISPLAY_GENERATION.load(Ordering::Acquire); if gen != seen_gen { seen_gen = gen; - let fresh = DRM_DISPLAY_CACHE.lock().unwrap().clone(); + // Recover from poison exactly like the writer does: the cache holds plain data whose + // invariants cannot be torn, and a panicking holder elsewhere must not make every live + // connection's topology push panic its task while the refresh worker keeps running. + let fresh = DRM_DISPLAY_CACHE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); // Send even an EMPTY list: when the last active CRTC disappears (all monitors // unplugged) the consumer must learn the topology is now empty, otherwise it keeps // advertising the removed displays indefinitely. @@ -922,7 +973,11 @@ fn drm_capture_worker( // it is empty (all monitors off), which is a real state, not "not ready". Only an unwarmed cache // (a connection racing the pre-warm) triggers a synchronous per-connection enumeration. let displays = if DRM_CACHE_WARMED.load(Ordering::Acquire) { - DRM_DISPLAY_CACHE.lock().unwrap().clone() + // Poison-recovery for the same reason as the topology push above. + DRM_DISPLAY_CACHE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone() } else { drm_enumerate_all_displays() };