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

drm: fix two review-suggested changes that were wrong, and stop overclaiming in the docs

Browse Source 
an adversarial sweep over the whole batch, aimed at the failure that
kept recurring here (a hazard identified and only some instances
fixed), found that two changes made on review advice were themselves
defects. both are reverted with the trace written down so they do not
get "fixed" again:

- the hotplug renumbering probe reads the pushed list at the CLIENT
  index again, not the service one. `bound_to` is an IDENTITY,
  (device, crtc_id), so comparing it against a slot is not a
  cross-index-space comparison; and `swap_available_displays` installs
  that same list as DRM_STATE two lines later, which IS the client
  space - display_service re-advertises it, input is mapped through
  it, the next rebuild reads `expected` out of it. Probing the service
  index answered a question nothing downstream consumes and went quiet
  in exactly the case the guard exists for: a stream whose wire_idx
  differs from its client index kept running while that index came to
  mean another monitor, so the client rendered monitor A believing it
  was monitor B and routed every click accordingly.
- the pipewire-fallback guard compares raw sizes again. BOTH sides are
  physical: `Display::width()` on the wayland variant returns
  `physical_width()`, and `try_fix_logical_size` only repairs the
  capturable's separate logical_size field. Scaling the drm side
  therefore compared logical against physical and rejected the valid
  stream on precisely the scaled outputs it was meant to rescue. The
  single-display carve-out now needs BOTH sides to be single, since a
  monitor on a card the service cannot open is missing from the drm
  list while the compositor still drives it.

also from the sweep:

- a capture build whose index is out of range of the advertised list
  now fails instead of falling back to the raw index, which the wake
  can have grown the service list back past - that bound a second
  video service to a monitor already being served and recorded its
  health under the wrong identity.
- the security doc no longer claims the privileged process never loads
  GL. That is true of the DEFAULT path and measured there, but the CPU
  fallback converts in-process, and a tiled scanout can only be
  decoded through the GPU, so libdrmtap dlopens libEGL in the calling
  process when the frame needs it. The doc now says which property
  belongs to the path and which to the process, and bounds the cases
  instead of overclaiming.
- the wake latch is described honestly: it self-clears when the
  display is next driven by anything, but nothing retries it, so a
  transient failure can leave it latched on an unattended host.
- the wake's uinput device DECLARES two axes and BTN_LEFT (libinput
  ignores a device that does not look like a mouse) while EMITTING
  only the net-zero axis round trip. the doc said one axis and no
  keys, describing the emit as if it were the declaration.
- the drm CI never ran for a change to the root Cargo.toml, where the
  top-level `drm` feature is defined, or to Cargo.lock, which every
  `--locked` build here resolves against. both triggers list them now.
- the deb assertion checks the packaged BINARY carries the libdrmtap
  dlopen path, not just that the library was staged beside it.
This commit is contained in:
Mariano Abad
2026-07-30 15:28:48 -03:00
parent 0035c89763
commit 76414c46a3
4 changed files with 126 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

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

57
docs/DRM_CAPTURE_SECURITY.md
View File

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

44
src/server/drm_capturer.rs
View File

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

53
src/server/wayland.rs
View File

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