diff --git a/.github/workflows/drm-capture.yml b/.github/workflows/drm-capture.yml index ce07ba0f2..c058770e6 100644 --- a/.github/workflows/drm-capture.yml +++ b/.github/workflows/drm-capture.yml @@ -28,6 +28,12 @@ on: - "libs/scrap/src/common/drmtap_dl.rs" - "libs/scrap/src/common/mod.rs" - "libs/scrap/Cargo.toml" + # The ROOT manifest is where the top-level `drm` feature is DEFINED, and the lock file is + # what every cargo invocation here resolves against (`--locked`). Without these two, a PR + # that changes what `drm` pulls in, or bumps a dependency the drm path uses, skips the + # entire verification this workflow exists for. + - "Cargo.toml" + - "Cargo.lock" - "src/ipc.rs" - "src/ipc/**" - "src/server/drm_capturer.rs" @@ -52,6 +58,12 @@ on: - "libs/scrap/src/common/drmtap_dl.rs" - "libs/scrap/src/common/mod.rs" - "libs/scrap/Cargo.toml" + # The ROOT manifest is where the top-level `drm` feature is DEFINED, and the lock file is + # what every cargo invocation here resolves against (`--locked`). Without these two, a PR + # that changes what `drm` pulls in, or bumps a dependency the drm path uses, skips the + # entire verification this workflow exists for. + - "Cargo.toml" + - "Cargo.lock" - "src/ipc.rs" - "src/ipc/**" - "src/server/drm_capturer.rs" @@ -336,6 +348,16 @@ jobs: echo "::notice::built $deb ($(stat -c %s "$deb") bytes)" dpkg -c "$deb" | grep -E 'usr/lib/rustdesk/libdrmtap\.so\.0\.[0-9]+\.[0-9]+$' dpkg -c "$deb" | grep -E 'usr/lib/rustdesk/libdrmtap\.so\.0 ->' + # The library alone does not make this a drm build: build.py stages it whenever --drm is + # passed, independently of what was compiled, and the deb name is what tells a user this + # is the consent-bypass variant. Assert the BINARY too, by the absolute dlopen path that + # only exists when the feature is compiled in -- otherwise a stock binary could ship + # under the unattended-wayland name with a library it can never reach. + rm -rf /tmp/debassert && dpkg-deb -R "$deb" /tmp/debassert + if ! grep -qF /usr/lib/rustdesk/libdrmtap.so.0 /tmp/debassert/usr/share/rustdesk/lib/librustdesk.so; then + echo "::error::the packaged librustdesk.so has no libdrmtap dlopen path; this is not a drm build" + exit 1 + fi mv "$deb" "${deb%.deb}-x86_64.deb" # MEASURE the glibc floor rather than describing it. This job builds on the runner instead of the diff --git a/docs/DRM_CAPTURE_SECURITY.md b/docs/DRM_CAPTURE_SECURITY.md index e9f96a540..8d6592b41 100644 --- a/docs/DRM_CAPTURE_SECURITY.md +++ b/docs/DRM_CAPTURE_SECURITY.md @@ -18,14 +18,31 @@ pixels: it exports the active scanout as a DMA-BUF and passes just that service-scoped IPC channel (`_drm`) via `SCM_RIGHTS`. The `--server` keeps an **import-once EGLImage cache** (keyed on the buffer, so a given scanout buffer is imported once and re-imports are elided), detiles/converts it to linear RGBA in -its own unprivileged address space, and feeds the encoder — so the root service -never loads libEGL/libGLESv2 and never copies scanout pixels. Only the **CPU -fallback path** (used when the seat/driver cannot produce a transferable DMA-BUF, -or the consumer has no render node of its own, see *When the CPU fallback is -chosen* below) copies the scanout to packed BGRA inside the root service and -streams those bytes over `_drm`. This mirrors -the Windows `portable_service` split (a privileged process captures, an -unprivileged one presents) but reuses RustDesk's own hardened IPC. +its own unprivileged address space, and feeds the encoder — so **on that path** +the root service never copies scanout pixels and never loads libEGL/libGLESv2 +(measured on the running service, see *Auditing*). Only the **CPU fallback path** +(used when the seat/driver cannot produce a transferable DMA-BUF, or the consumer +has no render node of its own, see *When the CPU fallback is chosen* below) +copies the scanout to packed BGRA inside the root service and streams those bytes +over `_drm`. + +**The no-GL property is a property of the default path, not of the process.** Be +precise about it, because the CPU fallback is the whole reason the split exists: +converting a scanout in-process means decoding whatever layout it is in, and a +tiled scanout (the common case on modern Intel and AMD) can only be decoded +through the GPU. `drmtap_grab_mapped` therefore reaches libdrmtap's auto-process +step, which lazily `dlopen`s libEGL/libGLESv2 **in the calling process** when the +scanout needs a GPU detile. So a host that has fallen back to the CPU path can +map the GL stack inside the `CAP_SYS_ADMIN` service. What the design does about +that is bound the cases: the fallback is entered only for the three reasons +listed below, never as a silent degradation of the split path (the loader refuses +a `libdrmtap` that cannot export the fd at all, precisely so "old library" cannot +turn into "convert in the privileged process"), and a linear or CPU-mappable +scanout is converted without touching GL. Every host measured here runs the split +path with zero GL regions in the service; a CPU-fallback host is a different +posture and is worth measuring separately. This mirrors the Windows +`portable_service` split (a privileged process captures, an unprivileged one +presents) but reuses RustDesk's own hardened IPC. - `libdrmtap.so` is loaded through a small `dlopen` loader (`drmtap_dl`); if the library or one of its runtime deps is missing the load fails cleanly and the @@ -118,10 +135,14 @@ unprivileged one presents) but reuses RustDesk's own hardened IPC. - **The display wake injects synthetic input from the root service.** A compositor that idles long enough DISABLES a connector, leaving no scanout for any backend, so on a `_drm` handshake that finds a CONNECTED display with no - CRTC the service emits one synthetic pointer round trip over `/dev/uinput` - (`+1` then `-1` on one relative axis: net-zero displacement, no button press, - no keys) to make the compositor re-enable it. This is deliberate input - injection by privileged code, so its bounds are worth stating precisely: + CRTC the service emits one synthetic pointer round trip over `/dev/uinput` to + make the compositor re-enable it. The virtual device **declares** two relative + axes and `BTN_LEFT`, because libinput classifies a device before it will treat + its events as pointer activity at all and a single axis with no buttons is + ignored outright (measured three ways on the same idle machine). What it + actually **emits** is `+1` then `-1` on one axis: net-zero displacement, no + button press, no key events. This is deliberate input injection by privileged + code, so its bounds are worth stating precisely: - it can only be reached through an **already-authorized** `_drm` connection (same per-connection authz as every other use of the channel), so it grants nothing to a local attacker that the channel itself does not; @@ -132,8 +153,16 @@ unprivileged one presents) but reuses RustDesk's own hardened IPC. uid, refuse root, and are desktop-specific; - the trigger is narrow — a connected-but-undriven connector, not "no frames" — and connectors a wake demonstrably cannot bring back are - remembered by connector identity and stop triggering (the memory drops any - entry later seen scanning out, so it cannot wedge); + remembered by connector identity and stop triggering. That memory is + per-connector rather than global, so a permanently dark connector cannot + suppress the wake for a different panel, and it drops any entry later seen + scanning out. Note what that recovery rule does and does not give you: it + clears the moment the display is driven **by anything**, but nothing else + retries, so a connector latched after a wake that failed for a transient + reason stays latched until that display comes back some other way — on an + unattended host, typically not until the service restarts. It is a + deliberate trade against waking on every connection forever for a display + that is never coming; - it is rate limited to **one wake per 20 s process-wide** with exactly one concurrent winner (compare-exchange claim), so a reconnect storm cannot become an input-injection storm, and it is useless as a way to keep a diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index acfaad303..81f266de2 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -575,9 +575,19 @@ async fn recv_thread( } } } - // No identity to hold the service to (a caller without a cached list); the raw index is - // all there is, exactly the pre-identity behaviour. - None => display.max(0) as usize, + // No identity to hold the service to. From get_capturer_info this means DRM_STATE does not + // describe `display` at all -- the index is out of range of the advertised list -- and + // falling back to the raw index would resolve it against the SERVICE's list, which the + // wake can have grown back past that index. The build would then succeed against an + // arbitrary monitor: two video services bound to one display, and the per-display health + // that gates demotion recorded under the wrong identity. Fail instead, so the video + // service rebuilds once the advertised list and the index agree again. + None => { + let _ = tx.send(Err(anyhow!( + "display {display} is not in the advertised list; not guessing a monitor for it" + ))); + return; + } }; // The service binds this stream to (device, crtc_id) at DrmStart, which survives a topology change. // Everything on this side is addressed by LIST INDEX, which does not: drm_enumerate_all_displays @@ -845,15 +855,27 @@ async fn recv_thread( // already takes. Checked BEFORE the list is swapped in, so the comparison is against // the topology this stream was started on. // - // The probe uses wire_idx, not `display`: this pushed list is in the SERVICE'S index - // space (the same fresh-enumeration construction as the handshake list), and wire_idx - // is where our monitor sat in that space when the stream was bound. `display` is a - // position in the list the CLIENT chose from, which is exactly the index space that - // can disagree with the service's whenever a wake or hotplug renumbered entries -- - // probing it here would pit slot `display` against slot wire_idx and either tear down - // a healthy stream or miss a genuine renumbering. + // The probe asks about `display`, the CLIENT's index, and that is deliberate -- + // it was briefly changed to wire_idx and that was wrong. Two facts settle it: + // + // - `bound_to` is an IDENTITY, `(device, crtc_id)`, not a position. So comparing + // it against any slot is not a cross-index-space comparison; it is the question + // "does that slot name MY monitor". + // - `swap_available_displays` two lines below installs this very list as + // DRM_STATE, which IS the client-space list: display_service re-advertises it, + // input is mapped through it, and the next rebuild reads `expected` out of it + // at `display`. So the only question worth asking here is whether slot + // `display` will still name this stream's monitor once that publish lands. + // + // Probing wire_idx instead answers a question nothing downstream consumes, and it + // stays quiet in exactly the case this guard exists for: a stream whose + // wire_idx != display keeps running while the client's index silently comes to + // mean another monitor, so the client renders monitor A believing it is monitor B + // and every pointer coordinate it computes lands on the wrong output. Checked + // BEFORE the swap, so the comparison is against the topology this stream started + // on. let now_at_our_index = list - .get(wire_idx) + .get(display.max(0) as usize) .map(|d| (d.device.clone(), d.crtc_id)); if bound_to.is_some() && now_at_our_index != bound_to { swap_available_displays(list); diff --git a/src/server/wayland.rs b/src/server/wayland.rs index f08a19717..290bffd24 100644 --- a/src/server/wayland.rs +++ b/src/server/wayland.rs @@ -457,37 +457,40 @@ pub(super) fn get_capturer_for_display( None => (None, false), }; if let Some(advertised) = advertised { - // Compare LOGICAL against LOGICAL. The advertised DRM geometry carries the - // PHYSICAL mode plus the compositor scale (augment_with_wayland_geometry sets - // x/y/scale and deliberately leaves width/height physical), while the PipeWire - // rect is already logical (try_fix_logical_size). Comparing the two raw made a - // scaled output disagree with itself -- 2880x1800 against 1440x900 -- and the - // guard then rejected a portal stream that really was this display, leaving it - // advertised offline instead of degrading to PipeWire. + // BOTH SIDES ARE PHYSICAL, so compare them raw. Traced rather than assumed, + // because it was twice "corrected" to a scale conversion that broke it: + // `rect` is built above from `Display::width()/height()`, and the WAYLAND + // variant of those returns `physical_width()/physical_height()` + // (scrap `common/wayland.rs`), i.e. `PipeWireCapturable.physical_size`. + // `try_fix_logical_size` only repairs the capturable's SEPARATE + // `logical_size` field and never touches `physical_size`, so the rect is not + // logical. The advertised DRM geometry is physical too + // (`augment_with_wayland_geometry` sets x/y/scale and deliberately leaves + // width/height as the DRM mode). Dividing one side by the scale therefore + // compares logical against physical and rejects the valid stream on exactly + // the scaled outputs it was meant to rescue. // - // The size check itself stays, because on a multi-monitor host it is what - // tells a single connector apart from the whole-desktop rect the portal - // usually exposes. On a single-display host that rect IS this display by - // construction, so only the position has to agree. - let scale = if advertised.scale > 0.0 { - advertised.scale - } else { - 1.0 - }; - let logical_w = (advertised.width as f64 / scale).round() as usize; - let logical_h = (advertised.height as f64 / scale).round() as usize; + // The size check is what tells one connector apart from the whole-desktop + // rect the portal usually exposes. It is skipped only when BOTH sides say + // there is a single display -- the DRM list has one entry and the PipeWire + // map has one -- because only then is "the whole-desktop stream IS this + // display" true by construction. (The portal can report a different physical + // size for a Full Workspace selection than the connector's mode, which is why + // that case needs the carve-out at all.) The DRM count alone is not enough: + // a monitor on a card the service cannot open is missing from the DRM list + // while the compositor still drives it. + let single_display = single_display && cap_display_info.num == 1; let consistent = advertised.x == rect.0 .0 && advertised.y == rect.0 .1 - && (single_display || (logical_w == rect.1 && logical_h == rect.2)); + && (single_display + || (advertised.width as usize == rect.1 + && advertised.height as usize == rect.2)); if !consistent { - // Report the LOGICAL numbers, the ones actually compared, so the message - // does not look like a mismatch of quantities that were never expected to - // match on a scaled output. bail!( - "drm display {} demoted with no geometry-consistent PipeWire stream (advertised {}x{} logical +{}+{} vs stream {}x{}+{}+{}); advertised offline", + "drm display {} demoted with no geometry-consistent PipeWire stream (advertised {}x{}+{}+{} vs stream {}x{}+{}+{}); advertised offline", display_idx, - logical_w, - logical_h, + advertised.width, + advertised.height, advertised.x, advertised.y, rect.1,