From d7ff2af0c26532233e4861c8342abc331cc1d648 Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Thu, 30 Jul 2026 12:44:59 -0300 Subject: [PATCH] drm: close the round-9 review findings - strict mode on the remaining two assert steps of the drm workflow (the deb-contents assert and the glibc-floor measurement): same masking pattern as the .so contract step fixed last round - without set -e only the last command's status counts and the mid-script checks were decorative. the floor extraction gets an explicit rescue so a no-match grep still reaches the `test -n` reporter. - the security doc states the whole accepted version window (exactly the pinned minor with a patch floor; a NEWER minor is refused too, because the mirrored struct layouts are only verified against the pinned one), and the auditing section carries the command matching its leftover-object comment. - the uinput-missing warning literal lost the embedded space runs a reflow had left in it (it is the sole, once-per-process diagnostic for that failure and it read as a run-on line with gaps). - the geometry-mismatch path in frame() hands the taken buffer back to the recycler before erroring; dropping it made every rebuild cycle re-allocate a scanout-sized buffer. --- .github/workflows/drm-capture.yml | 9 ++++++++- docs/DRM_CAPTURE_SECURITY.md | 16 ++++++++++++---- src/ipc/drm.rs | 5 ++++- src/server/drm_capturer.rs | 5 +++++ 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/.github/workflows/drm-capture.yml b/.github/workflows/drm-capture.yml index 9706c7ae1..c574db3cb 100644 --- a/.github/workflows/drm-capture.yml +++ b/.github/workflows/drm-capture.yml @@ -320,6 +320,9 @@ jobs: - name: Assert the deb is a real drm build shell: bash run: | + # Strict mode so the mid-script checks can fail the step (without it only the LAST + # command's status counts and the greps above it are decorative). + set -euo pipefail deb="$(ls rustdesk-unattended-wayland-*.deb)" test -n "$deb" echo "::notice::built $deb ($(stat -c %s "$deb") bytes)" @@ -335,10 +338,14 @@ jobs: id: floor shell: bash run: | + # Strict mode for the same reason as the assert step above. The floor extraction gets an + # explicit rescue so a no-match grep reaches the `test -n` reporter instead of dying as a + # bare pipeline failure. + set -euo pipefail deb="$(ls rustdesk-unattended-wayland-*-x86_64.deb)" rm -rf /tmp/debfloor && dpkg-deb -R "$deb" /tmp/debfloor floor="$(objdump -T /tmp/debfloor/usr/share/rustdesk/lib/librustdesk.so \ - | grep -oE 'GLIBC_2\.[0-9]+' | sort -uV | tail -1)" + | grep -oE 'GLIBC_2\.[0-9]+' | sort -uV | tail -1 || true)" test -n "$floor" echo "floor=${floor#GLIBC_}" >> "$GITHUB_OUTPUT" echo "::notice::deb requires ${floor} or newer (built on the runner, not the ubuntu18.04 release container)" diff --git a/docs/DRM_CAPTURE_SECURITY.md b/docs/DRM_CAPTURE_SECURITY.md index 61306ee04..50397c6db 100644 --- a/docs/DRM_CAPTURE_SECURITY.md +++ b/docs/DRM_CAPTURE_SECURITY.md @@ -30,10 +30,17 @@ 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 caller falls back to the PipeWire/portal path. -- The loader also **refuses a library that cannot do the split**: one reporting - below 0.4.10, and one reporting a newer version without actually exporting +- The loader also **refuses a library that cannot do the split** — and, more + broadly, any version outside the vetted window. Accepted is exactly the pinned + minor with a patch floor (currently `0.4.x`, `x >= 10`): an older minor + predates the split entry points, and a **newer minor is refused too** (`0.5.x` + included), because the loader mirrors C struct layouts that are only + field-by-field verified against the pinned minor; widening the window is a + deliberate act done together with re-verifying the layouts and moving the + build pin. Independently of the version report, a library that does not + actually export `drmtap_grab_desc` / `drmtap_open_render` / `drmtap_convert_dmabuf` (a stale or - pre-release build). The only way to capture with such a library is the + pre-release build) is refused as well. The only way to capture with such a library is the in-process convert, which in the root service means loading the vendor GL stack there, so it is refused and the caller falls back to PipeWire/portal. The privileged process therefore never loads GL because of which file happened to @@ -151,7 +158,8 @@ ls -l /usr/lib/rustdesk/libdrmtap.so.0* # the dlopen names the symlink by absolute path, so what matters is where the symlink points: readlink /usr/lib/rustdesk/libdrmtap.so.0 # expect: the versioned object shipped by the package # and there should be no other object left beside it (a leftover is not loaded on its own, but it -# is what a stray ldconfig over this directory would repoint the symlink to) +# is what a stray ldconfig over this directory would repoint the symlink to): +ls /usr/lib/rustdesk/libdrmtap.so.0.* # expect: exactly one versioned object ls /etc/ld.so.conf.d/ | grep -i rustdesk # expect: no output (none is shipped) # confirm no privileged helper is present (there should be none) getcap -r /usr/lib/rustdesk 2>/dev/null # expect: no output diff --git a/src/ipc/drm.rs b/src/ipc/drm.rs index 5d62b5cf0..911d8b8fa 100644 --- a/src/ipc/drm.rs +++ b/src/ipc/drm.rs @@ -507,7 +507,10 @@ fn drm_wake_displays(reason: &str) -> bool { // would log the same failure forever. DRM_WAKE_UNAVAILABLE.store(true, Ordering::Relaxed); log::warn!( - "drm: cannot wake displays ({reason}): no uinput device ({err}). A compositor that disabled its outputs will keep them disabled, so there is no scanout to capture until something else generates input. Note input injection needs uinput too, so this session cannot control the host either." + "drm: cannot wake displays ({reason}): no uinput device ({err}). A compositor that \ + disabled its outputs will keep them disabled, so there is no scanout to capture \ + until something else generates input. Note input injection needs uinput too, so \ + this session cannot control the host either." ); return false; } diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index d0355522f..acfaad303 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -454,6 +454,11 @@ impl TraitCapturer for IpcDrmCapturer { // would leave only the rapid-rebuild guard to catch it, several seconds later and // under a message about a change that never happened. if self.session_size.is_some_and(|(sw, sh)| (w, h) != (sw, sh)) { + // Hand the buffer back for recycling before erroring out: the receive path is + // still alive until it observes this session ending, and this is a + // scanout-sized allocation the recycler exists to keep. Dropping it here made + // every rebuild cycle re-allocate one. + self.shared.slot.lock().unwrap().free = Some(buf); if !self.got_frame { self.note_session_without_frame(); }