drm: close the round-10 review findings

- the /dev/dri gate returns the CANONICAL path instead of a bool, and
  both callers open that value. answering yes/no meant the caller
  handed the original string to libdrmtap, which re-resolved every
  symlink component after the check - a check-then-use window, in the
  root service. this is the whole point of the gate, so it should
  never have been able to hand back an unresolved path.
- `--package <folder> --drm` builds the capture library instead of
  demanding it inside the bundle. no build path puts libdrmtap in a
  bundle folder (the flutter deb builds it straight into the staged
  deb), so that check made the flag combination impossible to satisfy.
  the safety property it stood in for is now asserted directly and
  better: the staged BINARY must carry the drm dlopen path, so a stock
  binary can never be packaged under the consent-bypass name. a bundle
  that does carry a .so keeps its existing EGL assertion, and the
  variant naming keys on the explicit request rather than on what
  happened to be staged.
- the deb assert step globs into an array and asserts the count: under
  set -e `ls` aborted before its own `test -n` could report, and
  several matches produced a multi-line value whose mv failed with an
  unrelated error.
This commit is contained in:
Mariano Abad
2026-07-30 14:03:26 -03:00
parent a94420ba86
commit f524d41279
4 changed files with 87 additions and 50 deletions

View File

@@ -117,13 +117,20 @@ pub fn list_devices() -> Option<Vec<DrmDevice>> {
)
}
/// Returns true only if `path` canonicalizes to a node directly under /dev/dri/.
/// This is the realpath gate the libdrmtap helper applied but the in-process
/// (direct) path does not, so the service must apply it itself.
pub(super) fn device_under_dev_dri(path: &str) -> bool {
match std::fs::canonicalize(path) {
Ok(p) => p.parent().map_or(false, |d| d == std::path::Path::new("/dev/dri")),
Err(_) => false,
/// The CANONICAL path, when `path` canonicalizes to a node directly under /dev/dri/, else `None`.
/// This is the realpath gate the libdrmtap helper applied but the in-process (direct) path does
/// not, so the service must apply it itself.
///
/// Returning the resolved path rather than a bool is the point: a gate that answers yes/no leaves
/// the caller opening the ORIGINAL string, so every symlink component gets resolved a second time,
/// by the library, after the check -- a check-then-use window in which a component could be
/// repointed outside /dev/dri, in the root service. Callers must open the value this returns.
pub(super) fn device_under_dev_dri(path: &str) -> Option<std::path::PathBuf> {
let p = std::fs::canonicalize(path).ok()?;
if p.parent() == Some(std::path::Path::new("/dev/dri")) {
Some(p)
} else {
None
}
}
@@ -149,13 +156,17 @@ impl DrmReader {
let device_cstr = match device {
None => None,
Some(d) => {
if !device_under_dev_dri(d) {
// Open the CANONICAL path the gate resolved, never the caller's string: handing the
// original back would make libdrmtap re-walk the symlinks after the check.
let Some(canonical) = device_under_dev_dri(d) else {
log::warn!("DRM device {d:?} is not under /dev/dri; refusing to open");
return None;
}
match CString::new(d) {
Ok(c) => Some(c),
Err(_) => return None, // interior NUL
};
match canonical.to_str().and_then(|s| CString::new(s).ok()) {
Some(c) => Some(c),
// Non-UTF-8 or an interior NUL. /dev/dri node names are neither, so this is a
// path we do not need to serve.
None => return None,
}
}
};

View File

@@ -62,17 +62,16 @@ impl RenderConverter {
// opens whatever path it is handed is a needless widening.
let node_cstr = match node.filter(|n| !n.is_empty()) {
None => None,
Some(n) => {
if !super::drm_reader::device_under_dev_dri(n) {
// Open the CANONICAL path the gate resolved, not the string that arrived over IPC:
// opening the original would re-walk its symlinks after the check (see
// device_under_dev_dri).
Some(n) => match super::drm_reader::device_under_dev_dri(n) {
None => {
log::warn!("drm: render node {n:?} is not under /dev/dri; auto-selecting");
None
} else {
match CString::new(n) {
Ok(c) => Some(c),
Err(_) => None, // interior NUL
}
}
}
Some(canonical) => canonical.to_str().and_then(|s| CString::new(s).ok()),
},
};
// SAFETY: `open_render` is a resolved C entry point; `node_cstr` outlives the
// call, and NULL requests auto-selection of a render node.