TheLanc3/rustdesk
1
0
Fork 0
mirror of https://github.com/rustdesk/rustdesk.git synced 2026-09-08 05:20:59 +03:00
Code Issues Packages Projects Releases Wiki Activity

drm: close the full-review findings (a third latched flag, and two escapees)

Browse Source 
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.
This commit is contained in:
Mariano Abad
2026-07-29 20:05:45 -03:00
parent a35ed16508
commit 1647420993
6 changed files with 151 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
.github/workflows/drm-capture.yml vendored
View File

@@ -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:

5
build.py
View File

@@ -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())

11
docs/DRM_CAPTURE_SECURITY.md
View File

@@ -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.

21
libs/scrap/src/common/drm_reader.rs
View File

@@ -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 => {

38
libs/scrap/src/common/drm_render.rs
View File

@@ -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`

131
src/ipc/drm.rs
View File

@@ -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()
};