From 75af53b9bf7dabde5a6581af4fa93815dce64161 Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Tue, 21 Jul 2026 15:13:33 -0300 Subject: [PATCH] drm: address review findings 3.1, 4.2, 4.3, 4.4, 4.7 + minors 3.1: snapshot the stock flutter bundle before the CI drm relink and restore it before makepkg, so the official Arch package ships the stock cdylib, not the drm-enabled one. 4.2: wrap the drm block in a failure-tolerant subshell so a drm-only failure no longer aborts the stock deb/rpm/arch publish. 4.3: narrow the publish glob to rustdesk-[0-9]*.deb so the consent-bypass unattended-wayland deb stays an artifact, not on the public release. 4.4: rewrite the three stale DRM_CAPTURE_SECURITY.md statements to the split (default path passes a read-only scanout dma-buf fd over SCM_RIGHTS with an import-once cache; export validation is metadata-only; BGRA-over-the-wire is the fallback) and document that grab_desc's fd is O_RDONLY (DRM_RDWR dropped upstream, dup preserves it). 4.7: only short-circuit to the DRM cursor when it is authoritative (visible, or hidden in a pure-DRM session); fall through to the normal cursor path in a mixed DRM+PipeWire session. minors: thread the deb variant by feature not glob; TODO for the ld.so.conf.d system path; drop a stray blank line. All gated or whitespace so the drm-off build stays byte-identical. --- .github/workflows/flutter-build.yml | 26 +++++++++++++- DRM_CAPTURE_SECURITY.md | 55 +++++++++++++++++++++-------- build.py | 28 +++++++++++++-- libs/scrap/src/common/drm_reader.rs | 18 +++++++--- src/platform/linux.rs | 35 +++++++++++++----- src/server/display_service.rs | 18 +++++++++- 6 files changed, 148 insertions(+), 32 deletions(-) diff --git a/.github/workflows/flutter-build.yml b/.github/workflows/flutter-build.yml index 2b5623da9..d5895ab10 100644 --- a/.github/workflows/flutter-build.yml +++ b/.github/workflows/flutter-build.yml @@ -1683,6 +1683,21 @@ jobs: # the stock deb/rpm above. x86_64 only (the unattended/kiosk/server use # case); the package Conflicts/Replaces the stock rustdesk package. if [[ "${{ matrix.job.arch }}" == "x86_64" ]]; then + # The drm variant below rebuilds target/release/liblibrustdesk.so with the + # drm feature and re-runs `flutter build linux`, which overwrites the stock + # cdylib inside flutter/build/linux/x64/release/bundle/. The archlinux + # `makepkg` step (a later, separate action) ships THAT bundle verbatim, so + # without care the official Arch package would carry the drm cdylib. + # Snapshot the stock bundle now and restore it after the drm build + # (unconditionally, below) so makepkg only ever sees the stock cdylib. + STOCK_BUNDLE=/workspace/flutter/build/linux/x64/release/bundle + rm -rf /workspace/.stock-bundle + cp -a "$STOCK_BUNDLE" /workspace/.stock-bundle + # Build the drm variant in a failure-tolerant subshell: a drm-only breakage + # (EOL ubuntu18.04 apt, pip, the libdrmtap fork clone, meson) must NOT abort + # the stock deb/rpm/arch publish steps that run after this container. + ( + set -e pushd /workspace echo -e "start packaging unattended-wayland (DRM) deb" # drm-only build deps (meson builds libdrmtap.so from the cloned source), @@ -1755,6 +1770,11 @@ jobs: mv "$name" "${name%%.deb}-${{ matrix.job.arch }}.deb" done popd + ) || echo "WARN: unattended-wayland (DRM) variant build failed; shipping stock release only" + # Restore the stock flutter bundle so the archlinux makepkg step ships the + # stock cdylib, whether the drm variant above succeeded OR failed. + rm -rf "$STOCK_BUNDLE" + mv /workspace/.stock-bundle "$STOCK_BUNDLE" fi - name: Publish debian/rpm package @@ -1763,8 +1783,12 @@ jobs: with: prerelease: true tag_name: ${{ env.TAG_NAME }} + # Stock debs only: `rustdesk--.deb` (version starts with a digit). + # `rustdesk-[0-9]*.deb` deliberately EXCLUDES `rustdesk-unattended-wayland-*.deb`, + # the opt-in consent-bypass variant, which stays an Actions artifact (uploaded by + # name in the step below) and must never land on the public release. files: | - rustdesk-*.deb + rustdesk-[0-9]*.deb rustdesk-*.rpm - name: Upload deb diff --git a/DRM_CAPTURE_SECURITY.md b/DRM_CAPTURE_SECURITY.md index b711de74c..0b82da39b 100644 --- a/DRM_CAPTURE_SECURITY.md +++ b/DRM_CAPTURE_SECURITY.md @@ -12,11 +12,19 @@ Reading the active scanout needs `CAP_SYS_ADMIN` (to map other clients' framebuffers). RustDesk's root `--service` already runs with `CAP_SYS_ADMIN`, so the `drm` feature does the read **in-process in that root service**: it `dlopen`s `libdrmtap.so` and calls it in direct mode — no privileged child, no -`setcap` helper. Captured frames are copied to packed BGRA and streamed to the -unprivileged user `--server` over a dedicated service-scoped IPC channel -(`_drm`), which feeds them to the encoder. This mirrors the Windows -`portable_service` split (a privileged process captures, an unprivileged one -presents) but reuses RustDesk's own hardened IPC. +`setcap` helper. On the **default (split) path** the root service does not touch +pixels: it exports the active scanout as a DMA-BUF and passes just that +**read-only** fd to the unprivileged user `--server` over a dedicated +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 loaded `libdrmtap` predates the split export) 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. - `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 @@ -39,17 +47,34 @@ presents) but reuses RustDesk's own hardened IPC. prompt. On a misconfigured install it could expose the login screen, the lock screen, or another local user's graphical session. - **The scanout parse runs in the root service.** Moving the read in-process - removes the old `setcap` helper and its world-exec / DMA-BUF-fd-passing attack - surface, but it also means the pixel-format conversion / detile of an - untrusted framebuffer runs inside the `CAP_SYS_ADMIN` service, without a - seccomp cage around it. Mitigations: the device is realpath-gated to - `/dev/dri/`; the frame copy has format / stride / geometry and - integer-overflow guards (`drm_reader.rs`); non-32bpp scanouts are rejected - before the copy. + removes the old `setcap` helper and its world-exec attack surface. On the + **default (split) path** the root service does only a **metadata-only** parse + of the scanout descriptor and exports the DMA-BUF fd; the untrusted-framebuffer + detile / pixel-format conversion runs in the **unprivileged `--server`**, + outside `CAP_SYS_ADMIN`. Export-side validation is therefore metadata-only — + geometry bounded to `<= MAX_DIM` (16384) and `num_planes` in `1..=4` + (`drm_reader.rs` `grab_desc`); there is **no fourcc gate** on the export side, + because the format check is delegated to the unprivileged converter, which + handles every format `libdrmtap` supports (XRGB/ARGB8888, 10-bit XR30/AR30, + HDR, CCS-compressed). The exported fd is **read-only**: `libdrmtap` exports the + DMA-BUF via `drmPrimeHandleToFD` with `DRM_RDWR` dropped (`O_RDONLY`), and + `drm_reader` `dup()`s it — which shares the same open file description and so + preserves that access mode — so the unprivileged consumer can map the scanout + for reading but never write into the live framebuffer. On the **CPU fallback + path** the pixel-format conversion / detile instead runs inside the + `CAP_SYS_ADMIN` service without a seccomp cage; there the frame copy has + format / stride / geometry and integer-overflow guards (`drm_reader.rs` + `grab`), and non-32bpp scanouts are rejected before the copy. The device is + realpath-gated to `/dev/dri/` on both paths. - **`_drm` is a screen-content channel.** It is authorized per connection (see - above); without that authz any local process could read the screen. There is - no fd passing and no shared memory — frames cross as plain bytes over the - authorized socket. + above); without that authz any local process could read the screen. On the + **default (split) path** the channel carries the scanout DMA-BUF fd, passed to + the unprivileged `--server` over `SCM_RIGHTS` as a **read-only** descriptor + (the `--server` holds an import-once EGLImage cache, so a given scanout buffer + is imported once and re-imports are elided); the peer can map the scanout for + reading but cannot write it. The **CPU fallback path** instead carries plain + packed-BGRA bytes over the same authorized socket (no fd passing, no shared + memory). ## Deployment diff --git a/build.py b/build.py index 8131dbe57..4d10b9771 100755 --- a/build.py +++ b/build.py @@ -444,6 +444,14 @@ def finalize_deb(version, ships_so, so_basename=None): # rustdesk package. Then it writes the control, checksums, builds, and renames the .deb. if ships_so: system2(f'ln -sf {so_basename} tmpdeb/usr/lib/rustdesk/libdrmtap.so.0') + # TODO(drm, Debian Policy 10.2): dropping /usr/lib/rustdesk into the SYSTEM-WIDE + # linker search path (/etc/ld.so.conf.d) lets a privately-bundled library shadow a + # system library for EVERY binary on the host, which Debian Policy 10.2 forbids. + # The correct fix is to make the in-process dlopen resolve libdrmtap by ABSOLUTE + # path ("/usr/lib/rustdesk/libdrmtap.so.0") -- or link the rustdesk cdylib with an + # rpath of /usr/lib/rustdesk -- and then drop this ld.so.conf.d drop-in entirely. + # That change lives at the dlopen call site in src/ (drmtap_dl.rs), owned by + # another engineer, so it is out of scope for this packaging file; kept until then. system2('mkdir -p tmpdeb/etc/ld.so.conf.d') with open('tmpdeb/etc/ld.so.conf.d/rustdesk-unattended-wayland.conf', 'w') as f: f.write('/usr/lib/rustdesk\n') @@ -515,7 +523,7 @@ def build_flutter_deb(version, features): os.chdir("..") -def build_deb_from_folder(version, binary_folder): +def build_deb_from_folder(version, binary_folder, want_drm=False): os.chdir('flutter') system2('mkdir -p tmpdeb/usr/bin/') system2('mkdir -p tmpdeb/usr/share/rustdesk') @@ -544,6 +552,22 @@ def build_deb_from_folder(version, binary_folder): # private lib dir, then finalize the deb the same way build_flutter_deb does. bundled_glob = glob.glob('tmpdeb/usr/share/rustdesk/libdrmtap.so.0.*') ships_so = any(os.path.isfile(p) and not os.path.islink(p) for p in bundled_glob) + # The variant must be decided by the EXPLICIT --drm request, not merely by what happens + # to be staged. Cross-check the two and fail loudly on a mismatch: a drm binary staged + # WITHOUT its libdrmtap.so.0.* would otherwise be silently shipped as the stock + # `rustdesk` package (no drm deps, no ldconfig, a dlopen that can never resolve), and a + # bundle that DOES carry the .so would be shipped as the consent-bypass variant even + # when --drm was never asked for. + if want_drm and not ships_so: + raise Exception( + '--drm was requested but no real libdrmtap.so.0.* is staged under ' + 'usr/share/rustdesk/ in the bundle; refusing to package a drm binary as the ' + 'stock rustdesk package (it would ship without the capture library or its deps)') + if ships_so and not want_drm: + raise Exception( + 'the staged bundle carries libdrmtap.so.0.* but --drm was not passed; refusing ' + 'to silently ship the consent-bypass unattended-wayland variant (pass --drm to ' + 'build it deliberately)') so_basename = None if ships_so: so = _single_real_so(bundled_glob, 'the staged --drm bundle') @@ -641,7 +665,7 @@ def main(): portable = args.portable package = args.package if package: - build_deb_from_folder(version, package) + build_deb_from_folder(version, package, args.drm) return res_dir = 'resources' external_resources(flutter, args, res_dir) diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index c4e18cfa2..e4516cdef 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -232,9 +232,16 @@ impl DrmReader { /// The scanout `dma_buf_fd` is dup'd into an `OwnedFd` BEFORE the frame is /// released, so we keep an independently-owned reference to the buffer that /// survives `drmtap_frame_release` (the dma-buf refcount keeps the memory - /// alive while the peer also holds a reference). The descriptor is validated - /// on METADATA ONLY (no pixel access on the export side): the fourcc gate - /// (kept from `grab()`), `MAX_DIM`, and `num_planes` in `1..=4`. + /// alive while the peer also holds a reference). The exported fd is + /// READ-ONLY: libdrmtap exports the scanout via `drmPrimeHandleToFD` with + /// `DRM_RDWR` dropped (`O_RDONLY`), and `dup()` shares the same open file + /// description, so it preserves that access mode — the unprivileged + /// `--server` that receives the fd over `SCM_RIGHTS` can map the scanout for + /// reading but can never write into the live framebuffer. The descriptor is + /// validated on METADATA ONLY (no pixel access on the export side): + /// geometry `<= MAX_DIM` and `num_planes` in `1..=4`. There is deliberately + /// NO fourcc gate here (that is the CPU-mapped `grab()` fallback's job); the + /// format check is delegated to the unprivileged converter. /// /// Returns the owned fd + the validated descriptor with `dma_buf_fd` reset to /// `-1` (the `OwnedFd` owns the fd now; the descriptor's local int must never @@ -319,7 +326,10 @@ impl DrmReader { } // dup the fd into an OwnedFd BEFORE releasing the frame: after release // the library may recycle its handle, but our dup (an independent fd on - // the same open dma-buf) keeps the buffer alive for the peer. + // the same open dma-buf) keeps the buffer alive for the peer. dup(2) + // shares the same open file description, so it preserves the O_RDONLY + // access mode of libdrmtap's exported scanout fd (DRM_RDWR dropped) -- + // the peer's fd stays read-only and cannot write the live scanout. let dup_fd = hbb_common::libc::dup(raw_fd); if dup_fd < 0 { let e = io::Error::last_os_error(); diff --git a/src/platform/linux.rs b/src/platform/linux.rs index 4615489db..3bcdca3fa 100644 --- a/src/platform/linux.rs +++ b/src/platform/linux.rs @@ -365,7 +365,17 @@ pub fn get_cursor() -> ResultType> { #[cfg(feature = "drm")] if !is_x11() { if let Some(id) = crate::server::drm_capturer::drm_cursor_id() { - return Ok(Some(id)); + // In a mixed DRM + PipeWire session the DRM streams only cover the DRM-backed displays; + // when the pointer sits on a PipeWire-served display every DRM stream reports the hidden + // sentinel. Returning that sentinel here would hide the cursor globally, including on the + // PipeWire display where it is still visible, so only report a hidden DRM cursor when it + // is authoritative -- a pure-DRM session. A visible DRM cursor is always authoritative; + // otherwise fall through to the normal cursor path. + if id != scrap::drm_reader::HIDDEN_CURSOR_ID + || !crate::server::display_service::has_non_drm_backed_display() + { + return Ok(Some(id)); + } } } let mut res = None; @@ -392,14 +402,21 @@ pub fn get_cursor_data(hcursor: u64) -> ResultType { #[cfg(feature = "drm")] if !is_x11() { if let Some(c) = crate::server::drm_capturer::drm_cursor() { - let mut cd: CursorData = Default::default(); - cd.id = c.id; - cd.width = c.width; - cd.height = c.height; - cd.hotx = c.hotx; - cd.hoty = c.hoty; - cd.colors = c.colors.into(); - return Ok(cd); + // See get_cursor(): a hidden DRM sentinel is authoritative only in a pure-DRM session. In + // a mixed DRM + PipeWire session fall through so the PipeWire display's cursor is served + // by the normal path instead of being hidden everywhere. + if c.id != scrap::drm_reader::HIDDEN_CURSOR_ID + || !crate::server::display_service::has_non_drm_backed_display() + { + let mut cd: CursorData = Default::default(); + cd.id = c.id; + cd.width = c.width; + cd.height = c.height; + cd.hotx = c.hotx; + cd.hoty = c.hoty; + cd.colors = c.colors.into(); + return Ok(cd); + } } } let mut res = None; diff --git a/src/server/display_service.rs b/src/server/display_service.rs index 33bdad096..4a6ded0c3 100644 --- a/src/server/display_service.rs +++ b/src/server/display_service.rs @@ -444,6 +444,23 @@ pub(super) fn get_display_info(idx: usize) -> Option { SYNC_DISPLAYS.lock().unwrap().displays.get(idx).cloned() } +// True when at least one advertised (synced) display is NOT served by the DRM/KMS capture path, +// i.e. a mixed DRM + PipeWire session. The cursor service (platform::linux::get_cursor / +// get_cursor_data) uses this to decide whether a hidden DRM hardware-cursor sentinel is +// authoritative: in a pure-DRM session it is (the pointer is genuinely off every captured CRTC), +// but in a mixed session the sentinel only means the pointer moved onto a PipeWire-served display, +// whose cursor must come from the normal path instead of being hidden everywhere. +// +// When DRM capture is active the advertised list is enumerated from the DRM display list, so a DRM +// list shorter than the synced list means at least one advertised display is served by PipeWire. +#[cfg(all(target_os = "linux", feature = "drm"))] +pub fn has_non_drm_backed_display() -> bool { + match super::drm_capturer::get_display_infos() { + Some(drm) => drm.len() < SYNC_DISPLAYS.lock().unwrap().displays.len(), + None => false, + } +} + // Display to DisplayInfo // The DisplayInfo is be sent to the peer. pub(super) fn check_update_displays(all: &Vec) { @@ -545,7 +562,6 @@ pub fn get_primary_2(all: &Vec) -> usize { all.iter().position(|d| d.is_primary()).unwrap_or(0) } - #[inline] #[cfg(windows)] fn no_displays(displays: &Vec) -> bool {