diff --git a/DRM_CAPTURE_SECURITY.md b/docs/DRM_CAPTURE_SECURITY.md similarity index 88% rename from DRM_CAPTURE_SECURITY.md rename to docs/DRM_CAPTURE_SECURITY.md index ecdc33f53..0c2804529 100644 --- a/DRM_CAPTURE_SECURITY.md +++ b/docs/DRM_CAPTURE_SECURITY.md @@ -31,7 +31,7 @@ unprivileged one presents) but reuses RustDesk's own hardened IPC. 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.9, and one reporting a newer version without actually exporting + below 0.4.10, and one reporting a newer version without actually exporting `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 in-process convert, which in the root service means loading the vendor GL stack @@ -41,9 +41,21 @@ unprivileged one presents) but reuses RustDesk's own hardened IPC. the seat or the consumer. - The reader restricts the device it opens to a realpath under `/dev/dri/` (`drm_reader.rs`); RustDesk always runs libdrmtap in direct in-process mode - (`helper_path` is `NULL`), so no privileged child process is ever spawned and - none is built, shipped, or installed. There is no `drmtap-helper` binary, no - `setcap`, no capability-bearing file, and no capture group in this deployment. + (`helper_path` is `NULL`). **No `drmtap-helper` binary is built, shipped, or + installed by this package**: there is no `setcap`, no capability-bearing file, + and no capture group in this deployment. Being precise about what that does + and does not guarantee: an empty `helper_path` is not by itself a "helper + disabled" switch in the C. `find_helper` (`privilege_helper.c`) searches six + hardcoded paths, one of which is `/usr/lib/rustdesk/drmtap-helper`, the + directory this package installs into, and `fork`/`exec`s the first executable + it finds if the direct export ever returns `EACCES`/`EPERM`. Here that path is + unreachable for two independent reasons: the root service holds + `CAP_SYS_ADMIN` so the direct export succeeds, and the package builds only the + shared library, so no helper exists at any of those paths. They are all + root-writable-only, so a helper appearing there would not be an escalation + either, but the honest statement is "a privileged child is spawned only if a + helper binary exists at one of those fixed root-owned paths, and this package + never installs one", not "never". - The `_drm` socket lives beside the hardened `_service` socket (`/tmp/-service/ipc_drm`). It is `0666` so the unprivileged `--server` can connect, but every accepted peer is authorized in `handle_drm_conn` diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index 44bfd900b..e6ed9a5b1 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -170,8 +170,9 @@ impl DrmReader { /// the internal buffer. Returns (width, height). The returned slice is valid /// until the next grab. A non-32bpp scanout, an oversized/degenerate /// geometry, or a stride < w*4 is rejected with a hard error so the caller - /// falls back to PipeWire (see the codex format finding). Errno failures map - /// to WouldBlock (retry) or a hard error (tear down) as in the old path. + /// falls back to PipeWire rather than encoding whatever the bytes happen to + /// mean. Errno failures map to WouldBlock (retry) or a hard error (tear + /// down) as in the old path. pub fn grab(&mut self) -> io::Result<(&[u8], usize, usize)> { // SAFETY: self.ctx is a valid context; frame is zeroed before the call // and released on every path. diff --git a/libs/scrap/src/common/drm_render.rs b/libs/scrap/src/common/drm_render.rs index 84c041db6..817320d74 100644 --- a/libs/scrap/src/common/drm_render.rs +++ b/libs/scrap/src/common/drm_render.rs @@ -10,7 +10,8 @@ // import-once EGLImage cache it holds are THREAD-LOCAL inside libdrmtap: the // context MUST be created, used (`convert`), and closed (`drop`) on the SAME // thread (the consumer's `recv_thread`). Dropping it off-thread would strand the -// cached EGLImages — the exact leak class behind the 0.4.8 OOM regression. The raw +// cached EGLImages, which leaks a GPU context per capture session until the process is out of +// memory. The raw // ctx pointer makes `RenderConverter` !Send/!Sync, which enforces that at the type // level. @@ -54,7 +55,7 @@ impl RenderConverter { /// an empty/invalid path falls back to libdrmtap auto-selection. It opens no KMS /// card, spawns no helper, and needs no elevated capability. Returns `None` when /// libdrmtap is unavailable or too old to carry the split convert symbols (the - /// loader refuses a pre-0.4.9 `.so` outright), or when no render node could be + /// loader refuses anything below 0.4.10 outright), or when no render node could be /// opened (a locked-down seat with no `/dev/dri/renderD*` access) — the caller /// then degrades to the service-side CPU convert / PipeWire path. MUST be called /// on the thread that will later `convert()` and drop it. diff --git a/libs/scrap/src/common/drmtap_dl.rs b/libs/scrap/src/common/drmtap_dl.rs index a697f5e24..13e0bd445 100644 --- a/libs/scrap/src/common/drmtap_dl.rs +++ b/libs/scrap/src/common/drmtap_dl.rs @@ -148,7 +148,7 @@ type FnGrabMapped = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_frame_info type FnFrameRelease = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_frame_info); type FnGetCursor = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_cursor_info) -> c_int; type FnCursorRelease = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_cursor_info); -// Split-capture entry points (libdrmtap >= 0.4.9). REQUIRED (see below). +// Split-capture entry points (libdrmtap >= 0.4.10). REQUIRED (see below). // `grab_desc` runs on the privileged export side; `open_render`/`convert_dmabuf` // on the unprivileged converter side. type FnGrabDesc = @@ -176,7 +176,7 @@ pub struct DrmtapLib { pub frame_release: FnFrameRelease, pub get_cursor: FnGetCursor, pub cursor_release: FnCursorRelease, - // Split-capture symbols (libdrmtap >= 0.4.9). Not optional: a library that + // Split-capture symbols (libdrmtap >= 0.4.10). Not optional: a library that // cannot do the split is refused at load time (see `abi_accepted`), so these // are plain pointers and the type system carries the guarantee that no // caller can silently take an in-process-convert path instead. @@ -209,20 +209,20 @@ const DRMTAP_ABI_MAJOR: c_int = 0; // major alone bounds nothing: every release it has ever made reports major 0, // and comparing only that accepts a library from before the split existed. // -// 0.4.9 is where `drmtap_grab_desc` / `drmtap_open_render` / -// `drmtap_convert_dmabuf` landed, i.e. the oldest library that can serve the -// architecture this code implements: the privileged process exports the scanout -// dma-buf and NEVER converts, so it never loads libEGL/libGLESv2. An older .so -// has none of those entry points, and the only way to capture with it is the -// in-process convert, in the ROOT service. That is precisely the property the -// split exists to remove, so treat such a library as unusable and fall back to -// PipeWire/portal rather than quietly pulling the vendor GL stack into the +// 0.4.10 is the oldest release with the WHOLE split API: `drmtap_open_render` +// and `drmtap_convert_dmabuf` arrived in 0.4.9, `drmtap_grab_desc` in 0.4.10. +// That is the oldest library that can serve the architecture this code +// implements, where the privileged process exports the scanout dma-buf and NEVER +// converts, so it never loads libEGL/libGLESv2. Below it the only way to capture +// is the in-process convert, in the ROOT service, which is precisely the +// property the split exists to remove: treat such a library as unusable and fall +// back to PipeWire/portal rather than quietly pull the vendor GL stack into the // privileged process because a stale file happened to be on the load path. // // The mirrored `#[repr(C)]` layouts above are unchanged across 0.4.9..0.4.15 // (verified field by field against include/drmtap.h at both ends), so the floor // costs no compatibility that was real. -const DRMTAP_MIN_MINOR_PATCH: (c_int, c_int) = (4, 9); +const DRMTAP_MIN_MINOR_PATCH: (c_int, c_int) = (4, 10); /// Whether a library reporting `major.minor.patch` may be loaded. Pure, so the /// version rule is unit-testable without an .so to dlopen: the major must match @@ -420,8 +420,10 @@ mod tests { fn abi_gate_rejects_a_library_from_before_the_split() { // The releases that predate drmtap_grab_desc. Accepting any of these means the // privileged service has no export-only path and converts in-process, which is - // the whole thing the split was built to prevent. - for (minor, patch) in [(3, 3), (4, 0), (4, 8)] { + // the whole thing the split was built to prevent. 0.4.9 is in the list on + // purpose: it introduced the convert half of the split but not the export half, + // so it cannot serve the privileged side either. + for (minor, patch) in [(3, 3), (4, 0), (4, 8), (4, 9)] { assert!( !abi_accepted(DRMTAP_ABI_MAJOR, minor, patch), "v0.{minor}.{patch} predates the split-capture API and must be refused" diff --git a/src/ipc.rs b/src/ipc.rs index 0c33b2018..fde59ce1a 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -13,7 +13,9 @@ mod ipc_drm; // `crate::ipc::DrmDisplayInfo`) keep working, and so the `Data` variants can name the two // payload types. #[cfg(all(target_os = "linux", feature = "drm"))] -pub use ipc_drm::{start_drm, DmabufDesc, DrmConn, DrmDisplayInfo}; +pub use ipc_drm::{start_drm, DmabufDesc, DrmDisplayInfo}; +#[cfg(all(target_os = "linux", feature = "drm"))] +pub(crate) use ipc_drm::DrmConn; #[cfg(all(target_os = "linux", feature = "drm"))] pub(crate) use ipc_drm::connect_drm; diff --git a/src/ipc/auth.rs b/src/ipc/auth.rs index 56069f37d..89beef072 100644 --- a/src/ipc/auth.rs +++ b/src/ipc/auth.rs @@ -211,7 +211,9 @@ pub(crate) fn active_uid() -> Option { /// The active session uid read ONLY from the service-loop cache, never from a fresh (blocking) seat0 /// lookup. `None` on a cache miss. For hot, latency-sensitive, fail-closed re-auth on an async runtime /// thread (the `_drm` per-frame re-auth), where a blocking `loginctl` per frame would stall the stream. -#[cfg(target_os = "linux")] +// Gated with the feature, not just the OS: the `_drm` per-frame re-auth is its only caller, so a +// drm-off Linux build would carry it as dead code and warn about it. +#[cfg(all(target_os = "linux", feature = "drm"))] #[inline] pub(crate) fn active_uid_cached() -> Option { crate::platform::linux::get_active_userid_cached() diff --git a/src/ipc/drm.rs b/src/ipc/drm.rs index 1521cc570..7e409967a 100644 --- a/src/ipc/drm.rs +++ b/src/ipc/drm.rs @@ -106,7 +106,7 @@ pub(crate) async fn connect_drm(ms_timeout: u64) -> ResultType { // The producer MUST be root. DRM/KMS scanout export is a root-service capability, and the DRM // path outranks PipeWire (an available DRM stream suppresses the portal consent prompt), so a // non-root peer that won a socket-path race must not be trusted to supply the display list, - // frames and an arbitrary dma-buf fd (review 4.1). The producer direction is authorized in + // frames and an arbitrary dma-buf fd. The producer direction is authorized in // handle_drm_conn; this closes the same gap on the consumer direction. if peer_uid_from_fd(stream.as_raw_fd()) != Some(0) { bail!("drm: _drm producer is not root; refusing to consume"); @@ -119,7 +119,7 @@ pub(crate) async fn connect_drm(ms_timeout: u64) -> ResultType { /// drm-off build needs no hbb_common change. The socket is 0666 (world-connectable) so the /// unprivileged `--server` can reach it; every accepted peer is still authorized in /// `handle_drm_conn` (root or the active session uid + exe identity), so connectable != authorized. -async fn new_drm_listener() -> ResultType { +fn new_drm_listener() -> ResultType { let path = drm_ipc_path(); // Ensure the shared service dir exists at its hardened (0711) mode. Passing the `_service` // postfix reuses hbb_common's expected mode for that directory; it only creates/chmods the @@ -144,10 +144,8 @@ enum DrmProducerMsg { /// Enumerated displays, sent once before any frame so the task can answer the handshake. Displays(Vec), /// A captured frame (split/zero-copy path): the serializable dma-buf descriptor plus the (owned) - /// scanout fd to hand to the peer via SCM_RIGHTS. The worker always produces a real `fd` here; the - /// async task's `ExportLedger` decides whether to actually attach it (`desc.has_fd`) or elide it as - /// an import-once cache hit. The `OwnedFd` is closed once the send has dup'd it into the peer (or - /// immediately, when elided). + /// scanout fd to hand to the peer via SCM_RIGHTS. The `OwnedFd` is closed once the send has dup'd + /// it into the peer. Frame { desc: DmabufDesc, fd: Option, @@ -182,85 +180,6 @@ impl Drop for DrmStopGuard { } } -/// Producer-side fd-elision ledger (root `--service`, one per `_drm` connection). Decides, per -/// exported frame, whether the scanout dma-buf fd must ride an SCM_RIGHTS cmsg (`has_fd = true`) or -/// can be elided as an import-once cache hit (`has_fd = false`) because the peer's converter already -/// imported that `fb_id`. Keyed by `fb_id -> (modifier, dims)`; a change in any of those (a resize, -/// a modifier/tiling change, or a recycled fb_id that also changed geometry) forces a real fd, and a -/// modeset/hotplug that invalidates the CRTC ends the connection (so a reconnect starts with a fresh, -/// empty ledger — matching the peer's fresh, empty converter cache). -/// -/// SAFETY / CORRECTNESS: eliding relies solely on `(fb_id, modifier, dims)` uniquely identifying a -/// buffer, but the kernel can recycle an `fb_id` onto a *different* buffer with identical geometry -/// and modifier; eliding then would serve a stale EGLImage. libdrmtap's own import cache keys on -/// `fb_id + dma-buf inode` and can re-import ONLY when it is handed a real fd. Because always sending -/// the fd is cheap (the converter still imports once per `fb_id` and closes the surplus fd) and is -/// strictly safe, `DRM_FD_ELISION` defaults to `false` for v1 (always send). The ledger's `epoch` -/// tracks `DRM_DISPLAY_GENERATION` (bumped by the udev listener on a connector-topology change), so a -/// hotplug/modeset invalidates every cached buffer and forces a real fd; but the ledger still cannot -/// see the dma-buf inode, so a recycled fb_id within the SAME generation (identical geometry + -/// modifier) would elide onto a stale EGLImage. Enabling elision needs that inode case validated -/// first. -const DRM_FD_ELISION: bool = false; - -struct SeenBuf { - modifier: u64, - dims: (u32, u32), - epoch: u64, -} - -struct ExportLedger { - seen: HashMap, - order: std::collections::VecDeque, // insertion order, for evict-oldest - epoch: u64, -} - -impl ExportLedger { - // Grow-once, hard-capped (preallocated model): a hostile/buggy peer or a fb_id churn cannot grow - // this unbounded; oldest keys are evicted so a real fd is simply re-sent for them later. - const MAX_LEDGER: usize = 32; - - fn new() -> Self { - Self { - seen: HashMap::new(), - order: std::collections::VecDeque::new(), - epoch: 0, - } - } - - /// Returns true if this frame's fd must be attached (new/changed/recycled buffer, caching - /// disabled, or elision off), false if the converter already holds `fb_id` imported. - fn should_send_fd(&mut self, desc: &DmabufDesc) -> bool { - // fb_id == 0 disables caching for that frame; elision-off always sends. - if !DRM_FD_ELISION || desc.fb_id == 0 { - return true; - } - let ident = SeenBuf { - modifier: desc.modifier, - dims: (desc.width, desc.height), - epoch: self.epoch, - }; - if let Some(prev) = self.seen.get(&desc.fb_id) { - if prev.modifier == ident.modifier - && prev.dims == ident.dims - && prev.epoch == ident.epoch - { - return false; // import-once cache hit: elide the fd - } - } else { - // New key: record insertion order and evict the oldest if at capacity. - if self.order.len() >= Self::MAX_LEDGER { - if let Some(old) = self.order.pop_front() { - self.seen.remove(&old); - } - } - self.order.push_back(desc.fb_id); - } - self.seen.insert(desc.fb_id, ident); - true - } -} - /// Build a [`DrmConn`] from an already-authorized `_drm` `Connection` (root `--service` side). The /// parity `Connection` wraps a tokio `UnixStream` but exposes no way to move it out, so we `dup()` /// its fd into a fresh, independently-owned tokio `UnixStream` for the bespoke SCM_RIGHTS framing. @@ -589,7 +508,7 @@ fn drm_prewarm() { /// thread while the workers capture in parallel. #[tokio::main(flavor = "current_thread")] pub async fn start_drm() { - match new_drm_listener().await { + 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. @@ -644,7 +563,7 @@ fn drm_conn_admitted(prev_count: usize) -> bool { prev_count < MAX_DRM_CONNS } -/// Whether a `_drm` peer may keep receiving frames (review 3.3): root (uid 0) always, any other peer +/// Whether a `_drm` peer may keep receiving frames: root (uid 0) always, any other peer /// only while it still matches the active-session uid, and an unknown peer never (fail closed). Pure, /// so the per-frame re-authorization decision is unit-testable without a live logind session. fn drm_peer_authorized(peer_uid: Option, active_uid: Option) -> bool { @@ -707,7 +626,7 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { // physical scanout of a CRTC regardless of which session currently owns the display. So a stream // authorized for one session must stop the moment the active session changes, or the outgoing // user's --server keeps receiving the incoming user's screen (and the greeter in between) until - // the socket dies (review 3.3). `peer_uid` is the --server's fixed uid. + // the socket dies. `peer_uid` is the --server's fixed uid. let peer_uid = stream.peer_uid(); // Move the authorized `_drm` stream onto the bespoke SCM_RIGHTS framing (see `DrmConn`). ALL @@ -792,17 +711,14 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { } // Forward frames + cursor updates until the worker ends or the client disconnects (a wire send - // error on a dropped client propagates out and tears the worker down via the guard). The - // per-connection `ExportLedger` decides, for the zero-copy path, whether each frame's fd must ride - // an SCM_RIGHTS cmsg or can be elided as an import-once cache hit. - let mut ledger = ExportLedger::new(); + // error on a dropped client propagates out and tears the worker down via the guard). // Live hotplug: the udev listener bumps DRM_DISPLAY_GENERATION when the connector topology changes. // Seed from the value current at handshake (the list already sent reflects it) and, whenever it // moves, push the fresh list to this consumer. Piggybacked on the frame cadence so it costs only one // atomic load per frame; a genuinely idle stream tears down after MAX_STALLED and the consumer // reconnects to a fresh list anyway. let mut seen_gen = DRM_DISPLAY_GENERATION.load(Ordering::Acquire); - // Flow control (review P1-2): allow at most DRM_FRAME_CREDIT frames in flight on the socket. The + // Flow control: allow at most DRM_FRAME_CREDIT frames in flight on the socket. The // consumer acks each converted frame (send_frame_ack) and the producer only sends while it has // credit, so a slow convert bounds the socket FIFO to a couple of frames instead of accumulating // seconds of stale descriptors (a permanently-behind desktop). Backpressure on the capture @@ -875,7 +791,7 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { None => break, } }; - // Re-authorize per frame (review 3.3): root (0) is always allowed; any other peer must still be + // Re-authorize per frame: root (0) is always allowed; any other peer must still be // the active-session uid. Use the CACHE-ONLY active uid (never a blocking loginctl lookup): this // runs on the single-threaded `_drm` runtime, so a per-frame seat0 subprocess -- which is // exactly what a fresh lookup does during a session switch, when the cache is momentarily empty @@ -889,10 +805,6 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { break; } let gen = DRM_DISPLAY_GENERATION.load(Ordering::Acquire); - // Keep the ledger's epoch at the live generation so a hotplug/modeset (which may recycle an - // fb_id onto a new buffer) invalidates every cached buffer and forces a real fd on the next - // frame. Cheap (one field write) and only observable when DRM_FD_ELISION is enabled. - ledger.epoch = gen; if gen != seen_gen { seen_gen = gen; let fresh = DRM_DISPLAY_CACHE.lock().unwrap().clone(); @@ -901,7 +813,7 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { // advertising the removed displays indefinitely. conn.send_msg(&Data::DrmDisplaysChanged(fresh), None).await?; } - // Coalesce to latest-wins at the source (review 4.8). The `_drm` socket is a FIFO, so a + // Coalesce to latest-wins at the source. The `_drm` socket is a FIFO, so a // consumer that drains slower than we produce (a 4K convert on a modest GPU) would fall // seconds behind stale frames. Drain everything already queued without blocking and forward // only the NEWEST frame; each replaced frame drops here, closing its OwnedFd (zero-copy path) @@ -954,8 +866,14 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { } match latest_frame { Some(DrmProducerMsg::Frame { mut desc, fd }) => { - // The worker always supplies a real fd; the ledger decides whether to attach it. - let send_fd = fd.is_some() && ledger.should_send_fd(&desc); + // Every exported frame carries its fd. Eliding it on an fb_id the converter has + // already imported looks free, but the kernel can recycle an fb_id onto a different + // buffer with identical geometry and modifier, and this side cannot see the dma-buf + // inode that would tell the difference, so an elision can serve a stale EGLImage. + // Sending it is cheap: the converter imports once per buffer and closes the surplus + // fd. libdrmtap's own import cache keys on fb_id AND inode, and can only re-import + // when it is handed a real fd. + let send_fd = fd.is_some(); desc.has_fd = send_fd; let borrowed = if send_fd { fd.as_ref().map(|f| f.as_fd()) } else { None }; conn.send_msg(&Data::DrmFrameDmabuf(desc), borrowed).await?; @@ -1009,7 +927,7 @@ fn drm_capture_worker( drm_enumerate_all_displays() }; // Send even an empty list: the consumer treats "0 displays" as Unavailable and falls back - // promptly (zhou's empty-topology finding), rather than waiting out repeated probe failures. + // promptly, rather than waiting out repeated probe failures. if frame_tx .blocking_send(DrmProducerMsg::Displays(displays)) .is_err() @@ -1102,7 +1020,7 @@ fn drm_capture_worker( pitches: d.pitches, hdr_eotf: d.hdr_eotf, hdr_max_nits: d.hdr_max_nits, - has_fd: true, // the async task's ExportLedger may downgrade this + has_fd: true, // every exported frame carries its fd; see the send below }, fd: Some(fd), }), @@ -1200,7 +1118,7 @@ fn drm_capture_worker( /// SCM_RIGHTS cmsg bound to the frame's first (prefix) byte, so reading the prefix with a control /// buffer reliably collects it (`MSG_CTRUNC` is rejected). Reads use exact-length loops so they never /// cross a frame boundary and thus never discard a following frame's ancillary fd. -pub struct DrmConn { +pub(crate) struct DrmConn { /// The raw stream. Obtained from `connect_drm` (client) or the accepted `_drm` listener stream /// (service). All framing is done by hand on this fd; there is no `Framed` codec. stream: tokio::net::UnixStream, @@ -1593,7 +1511,7 @@ impl DrmConn { } } -// Pure-userspace coverage for the bespoke `_drm` SCM_RIGHTS framing (review 6). The wire format is +// Pure-userspace coverage for the bespoke `_drm` SCM_RIGHTS framing. The wire format is // hand-rolled (length prefix + an fd bound to the frame's first byte) because `Framed`/`BytesCodec` // cannot carry ancillary data, so it gets direct tests over a socketpair instead of only live runs. #[cfg(test)] @@ -1824,7 +1742,7 @@ mod drm_conn_tests { assert_eq!(peer_uid_from_fd(a.as_raw_fd()), Some(euid)); } - // Per-frame _drm re-auth decision (review 3.3): root always passes; a non-root peer passes only + // Per-frame _drm re-auth decision: root always passes; a non-root peer passes only // while it still equals the active-session uid; an unknown peer or active session fails closed. #[test] fn drm_peer_authorized_matrix() { @@ -1842,7 +1760,7 @@ mod drm_conn_tests { assert!(!drm_peer_authorized(None, None)); } - // _drm admission bound (review 6): admit strictly below MAX_DRM_CONNS, reject at and above it. + // _drm admission bound: admit strictly below MAX_DRM_CONNS, reject at and above it. // `prev_count` is the live count taken before this connection (what fetch_add returns). #[test] fn drm_conn_admission_bound() { diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index bbf9783d5..2cd760a82 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -22,7 +22,7 @@ // thread-local, so both convert and close must run on the same thread. use crate::ipc::{connect_drm, Data, DrmDisplayInfo}; -use hbb_common::{anyhow::anyhow, log, message_proto::DisplayInfo, tokio, ResultType}; +use hbb_common::{anyhow::anyhow, bail, log, message_proto::DisplayInfo, tokio, ResultType}; use scrap::drm_render::RenderConverter; use scrap::drmtap_dl::drmtap_dmabuf_desc; use scrap::{Frame, Pixfmt, PixelBuffer, TraitCapturer}; @@ -239,7 +239,7 @@ impl IpcDrmCapturer { // with no owning capturer (our Drop never runs — the capturer was never built), so // signal it to stop before giving up. stop.store(true, Ordering::SeqCst); - return Err(anyhow!("drm capture handshake timed out")); + bail!("drm capture handshake timed out"); } }; Ok(( @@ -420,10 +420,10 @@ async fn recv_thread( .get(display.max(0) as usize) .map(|d| (d.device.clone(), d.crtc_id)); let our_key = displays.get(display.max(0) as usize).map(connector_key); - // Open the unprivileged render-node convert context ONCE, on THIS thread, BEFORE the handshake; it - // is dropped on this same thread when the loop exits (its EGL state + import-once cache are - // thread-local). `None` means no usable render node (a locked-down seat, or an old `.so` without - // the split symbols): we then ask the service for the CPU-converted `DrmFrame` path via + // Open the unprivileged render-node convert context ONCE, on THIS thread, before we answer the + // display list with DrmStart; it is dropped on this same thread when the loop exits (its EGL + // state + import-once cache are thread-local). `None` means no usable render node (a locked-down + // seat with no /dev/dri/renderD* access): we then ask the service for the CPU-converted `DrmFrame` path via // `need_cpu`, so a render-node-less seat still captures instead of the service streaming a dma-buf // fd we cannot detile (which would lose the stream and force a PipeWire fallback nobody may be // present to approve on an unattended seat). @@ -448,7 +448,7 @@ async fn recv_thread( // SUCCEEDS and yields corrupted pixels, so there is no convert error for the prefer-cpu bit above // to learn from - the stream just looks broken. The node is empty when the service ran against a // libdrmtap without `drmtap_render_node` (we dlopen by soname, so the runtime .so can be older - // than the one this was built against, anywhere in 0.4.9..0.4.14 -- below that it does not load + // than the one this was built against, anywhere in 0.4.10..0.4.14 -- below that it does not load // at all). Ask for the CPU path instead: the service converts on the // device it already has open, which is correct by construction. Single-render-node hosts (the // common case) keep the dma-buf fast path untouched. @@ -530,7 +530,10 @@ async fn recv_thread( format: desc.format, modifier: desc.modifier, fb_id: desc.fb_id, - num_planes: desc.num_planes, + // Clamped although the producer already normalizes it and must be root: this + // value indexes offsets/pitches inside libdrmtap, and the wire is the one place + // it arrives from another process. + num_planes: desc.num_planes.clamp(1, 4), offsets: desc.offsets, pitches: desc.pitches, hdr_eotf: desc.hdr_eotf, @@ -797,27 +800,32 @@ fn remove_drm_cursor(display: i32, epoch: u64) { } } -// Pick the cursor to present: prefer the visible one (the pointer is over exactly one captured CRTC -// at a time), else fall back to any (hidden) entry so the client still gets the hidden sentinel when -// the pointer is off every captured monitor. `None` only when no stream is active. -fn pick_drm_cursor() -> Option { +// Which cursor to present: prefer the visible one (the pointer is over exactly one captured CRTC at +// a time), else fall back to any (hidden) entry so the client still gets the hidden sentinel when the +// pointer is off every captured monitor. Returns what `f` extracts from it, so a caller that only +// wants the id does not pay for a clone of the pixels. `None` only when no stream is active. +fn with_drm_cursor(f: impl Fn(&DrmCursorData) -> T) -> Option { let map = DRM_CURSOR.lock().unwrap(); map.values() .map(|(_, c)| c) .find(|c| c.id != scrap::drm_reader::HIDDEN_CURSOR_ID) .or_else(|| map.values().map(|(_, c)| c).next()) - .cloned() + .map(f) } /// The id of the current DRM hardware cursor (None if no stream). The cursor service polls this to -/// detect shape changes (a change triggers a `get_cursor_data` fetch). +/// detect shape changes (a change triggers a `get_cursor_data` fetch), so it runs at frame cadence +/// and deliberately reads the id WITHOUT copying the pixels: a 256x256 cursor is 256 KiB, and +/// cloning that 30 times a second to look at 8 bytes of it is pure waste. pub fn drm_cursor_id() -> Option { - pick_drm_cursor().map(|c| c.id) + with_drm_cursor(|c| c.id) } -/// The current DRM hardware-cursor snapshot (RGBA), or None. +/// The current DRM hardware-cursor snapshot (RGBA), or None. The pixels are premultiplied ARGB and +/// are passed through as-is, which is exactly what the XFixes path does (`platform/linux.rs` +/// `get_cursor_data`), so the client sees one cursor format whichever backend produced it. pub fn drm_cursor() -> Option { - pick_drm_cursor() + with_drm_cursor(|c| c.clone()) } // --------------------------------------------------------------------------- @@ -1100,6 +1108,12 @@ fn refresh_available_async() { /// retries (the "connects on the Nth try" symptom). Probes with a short retry budget and only caches /// the positive result; a genuinely DRM-less host just falls through to the lazy `is_available()`. pub(super) fn warm_availability() { + // Nothing on X11 can consume a DRM stream, and probing makes the ROOT service open DRM readers, + // so an X11 host running a drm build would pay that at every startup for a path it can never + // take. The lazy probe behind is_available is reached only from the Wayland paths already. + if crate::platform::linux::is_x11() { + return; + } for _ in 0..10 { if matches!(&*DRM_STATE.lock().unwrap(), ProbeState::Available(..)) { return; @@ -1126,7 +1140,7 @@ pub(super) fn get_display_infos() -> Option> { }; let multi = list.len() > 1; let mut infos = augment_with_wayland_geometry(&list); - // review 4.5: on a multi-monitor host a display demoted to PipeWire has no geometry-consistent + // On a multi-monitor host a display demoted to PipeWire has no geometry-consistent // per-connector stream to fall through to -- the portal exposes a single whole-desktop stream, so // serving it for one connector would stretch the frame and offset all input. Advertise such a // display OFFLINE while keeping its list position, so the index space stays aligned with @@ -1369,9 +1383,9 @@ pub(super) fn get_capturer_info( if since.elapsed() >= demote_cooldown(demotes) { map.insert(key.clone(), (0, Instant::now(), demotes)); } else { - return Err(anyhow!( + bail!( "drm capture for display {display_idx} repeatedly produced no frame; using PipeWire" - )); + ); } } } @@ -1408,9 +1422,7 @@ pub(super) fn get_capturer_info( e.1 = Instant::now(); e.2 += 1; } - return Err(anyhow!( - "drm capture for display {display_idx} is flapping; using PipeWire" - )); + bail!("drm capture for display {display_idx} is flapping; using PipeWire"); } } let ndisplay = displays.len(); diff --git a/src/server/input_service.rs b/src/server/input_service.rs index 2b13d59bd..59a3555ac 100644 --- a/src/server/input_service.rs +++ b/src/server/input_service.rs @@ -400,17 +400,17 @@ fn run_cursor(sp: MouseCursorService, state: &mut StateCursor) -> ResultType<()> // requested `hcursor` (it returns the latest hardware cursor); file it in the cache AND // record state.hcursor under the id ACTUALLY served, so a later reappearance of that exact // shape dedupes correctly instead of being suppressed. Everything below is fully - // `#[cfg(feature = "drm")]`-gated so the drm-off build stays byte-identical to upstream. - #[cfg(feature = "drm")] + // gated on the drm feature, so the drm-off build stays byte-identical to upstream. + #[cfg(all(target_os = "linux", feature = "drm"))] let mut drm_served_id = hcursor; if let Some(cached) = state.cached_cursor_data.get(&hcursor) { super::log::trace!("Cursor data cached, hcursor: {}", hcursor); msg = cached.clone(); } else { let mut data = crate::get_cursor_data(hcursor)?; - #[cfg(feature = "drm")] + #[cfg(all(target_os = "linux", feature = "drm"))] let hcursor = data.id; - #[cfg(feature = "drm")] + #[cfg(all(target_os = "linux", feature = "drm"))] { drm_served_id = hcursor; } @@ -421,11 +421,11 @@ fn run_cursor(sp: MouseCursorService, state: &mut StateCursor) -> ResultType<()> state.cached_cursor_data.insert(hcursor, msg.clone()); super::log::trace!("Cursor data updated, hcursor: {}", hcursor); } - #[cfg(not(feature = "drm"))] + #[cfg(not(all(target_os = "linux", feature = "drm")))] { state.hcursor = hcursor; } - #[cfg(feature = "drm")] + #[cfg(all(target_os = "linux", feature = "drm"))] { state.hcursor = drm_served_id; } diff --git a/src/server/wayland.rs b/src/server/wayland.rs index b363ad135..bbc9f80a7 100644 --- a/src/server/wayland.rs +++ b/src/server/wayland.rs @@ -426,8 +426,8 @@ pub(super) fn get_capturer_for_display( unsafe { let cap_display_info = &*cap_display_info; let rect = cap_display_info.rects[cap_display_info.current]; - // review 4.5: reaching here with DRM active means get_capturer_info bailed (a demoted - // display) and we fell through to PipeWire. Serve this stream ONLY if its rect matches the + // Reaching here with DRM active means get_capturer_info bailed (a demoted display) and + // we fell through to PipeWire. Serve this stream ONLY if its rect matches the // geometry we advertised for this index. The portal typically exposes one whole-desktop // stream, so on a multi-monitor host that rect is the FULL desktop while the advertised DRM // geometry is a single connector -> serving it would stretch the frame and offset all