From 64621975517bddf7d82bc38fcda03dc4e586b2f9 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Sun, 6 Sep 2026 20:05:22 -0700 Subject: [PATCH 1/3] feat(linux): own udev rules setup and diagnose unopenable ports (#1424) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Linux serial nodes are root:dialout 0660 and the invoking user is usually not in `dialout`. `fbuild deploy` then flashes successfully and cannot reopen the port it just flashed. Nothing in the toolchain set this up: fbuild had no udev/setup/doctor path, and FastLED only *diagnoses* (`ci/compiler/pio.py::check_usb_permissions`) on the legacy PlatformIO path that the fbuild deploy path never calls. PlatformIO used to ship `99-platformio-udev.rules` for manual install; fbuild replaced pio for build and deploy but not that setup step. The expensive part was the misdirection. fbuild found the port, called it `health healthy`, printed `Permission denied`, and then advised cables, BOOTSEL and RESET — a remedy unrelated to the cause. On an unattended bench that reads as a hardware fault. `port doctor` now reports it. A port can be attached, healthy, and still impossible to open; that combination rendered as "attached and healthy" with an empty remedy. The new verdict is checked before presence and names the fix. It also says a one-shot chmod will not hold: deploy re-enumerates the board (BOOTSEL -> application) and udev recreates the node before fbuild reopens it, so only a vendor-keyed rule survives. `fbuild port udev` prints rules for every vendor in the registry. - Vendors come from the ingested FastLED/boards catalogue via the new `usb::online_vendor_vids()`, never a local table — a hand-maintained copy would drift the moment a vendor is ingested, which is what the VID/PID source-of-truth rule exists to prevent. 36 vendors today. - Prints, never installs. On NixOS `/etc` is generated from declarative config, so a written file there is out-of-band and gets clobbered; those users need the content for `services.udev.extraRules`. Elsewhere it keeps the privileged write explicit. - An empty registry is an error, not an empty file: one that looks configured while granting nothing is worse than none. - Defaults to `plugdev`, which unlike `dialout` does not also confer modem/PPP access. Rules render as lowercase zero-padded 4-digit hex because udev compares ATTRS{idVendor} as a string against sysfs — "0x2E8A" or "2E8A" simply never match, silently. Tests pin that, the padding, sort/dedup, the group override, and the empty-registry refusal. Verified: cargo check --all-targets clean; `cargo test -p fbuild-cli` 315 passed / 0 failed, including 6 new udev tests and 4 new port_doctor tests (the existing 27 still pass, so the absent/failing-board verdicts this command was written for are undisturbed). `fbuild port udev` emits 36 rules from the live registry, every VID exactly 4 lowercase hex digits, including 2e8a and 303a. Found while an RP2350 bench run kept failing post-deploy port reopen (FastLED#3899). --- crates/fbuild-cli/src/cli/mod.rs | 1 + crates/fbuild-cli/src/cli/port_doctor.rs | 98 ++++++++++++++++++ crates/fbuild-cli/src/cli/port_scan.rs | 46 +++++++++ crates/fbuild-cli/src/cli/udev.rs | 123 +++++++++++++++++++++++ crates/fbuild-core/src/usb/data.rs | 25 +++++ crates/fbuild-core/src/usb/mod.rs | 3 +- 6 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 crates/fbuild-cli/src/cli/udev.rs diff --git a/crates/fbuild-cli/src/cli/mod.rs b/crates/fbuild-cli/src/cli/mod.rs index 52c9b672..c485fb22 100644 --- a/crates/fbuild-cli/src/cli/mod.rs +++ b/crates/fbuild-cli/src/cli/mod.rs @@ -34,6 +34,7 @@ pub mod monitor_parse; pub mod pio; pub mod plotter; pub mod port_doctor; +pub mod udev; pub mod port_doctor_fix; pub mod port_scan; pub mod purge; diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index 579eed10..60a4b281 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -32,6 +32,13 @@ pub struct PortDiagnosis { /// a single-port query — see `query_last_seen_secs` for why. #[serde(skip_serializing_if = "Option::is_none")] pub last_seen_secs_ago: Option, + /// Whether this process can actually open the port. + /// + /// `None` when not probed. Never silently `true`: a port the host lists + /// as healthy can still be unopenable, and reporting that as fine is the + /// failure this field exists to stop (FastLED/fbuild#1424). + #[serde(skip_serializing_if = "Option::is_none")] + pub openable: Option, } /// What the diagnosis means and what to do about it. @@ -68,10 +75,51 @@ pub fn diagnose(port: &DetectedPort, power_rows: &[(String, bool)]) -> PortDiagn parent_instance_id: port.parent_instance_id.clone(), suspend_allowed: suspend_for_ancestors(power_rows, &port.ancestor_instance_ids), last_seen_secs_ago: None, + openable: probe_openable(&port.info.port_name), + } +} + +/// Whether the current process can open `port`, or `None` where the question +/// is not meaningful. +/// +/// Linux only. Elsewhere serial access is not group-gated the same way and a +/// speculative open would be a side effect in a command documented as +/// strictly read-only. Opening for read is enough to surface `EACCES` and +/// does not disturb a device: no DTR/RTS assertion, no write. +#[cfg(target_os = "linux")] +pub fn probe_openable(port: &str) -> Option { + use std::io::ErrorKind; + match std::fs::OpenOptions::new().read(true).open(port) { + Ok(_) => Some(true), + Err(e) if e.kind() == ErrorKind::PermissionDenied => Some(false), + // Busy, absent, or anything else is a different question that the + // presence/problem-code verdict already covers. Claiming "not + // openable" here would blame permissions for an unrelated fault. + Err(_) => None, } } +#[cfg(not(target_os = "linux"))] +pub fn probe_openable(_port: &str) -> Option { + None +} + pub fn verdict(diagnosis: &PortDiagnosis) -> Verdict { + // Checked before presence/problem-code: a port can be attached, healthy, + // and still impossible to open. That combination previously rendered as + // "attached and healthy" with an empty remedy, so `fbuild deploy` would + // flash successfully and then time out reopening the port it had just + // reported healthy — with recovery advice about cables and BOOTSEL that + // had nothing to do with the cause (FastLED/fbuild#1424). + if diagnosis.openable == Some(false) { + return Verdict { + summary: "attached, but this process cannot open the port (permission denied)" + .to_string(), + remedy: "serial nodes are typically root:dialout 0660 and your user is not in that group. Generate rules with `fbuild port udev`, install them as /etc/udev/rules.d/99-fbuild.rules, then `sudo udevadm control --reload-rules && sudo udevadm trigger`. Adding your user to the group works too, but needs a fresh login. A one-shot chmod does not hold: deploy re-enumerates the board and udev recreates the node" + .to_string(), + needs_hands: true, + }; + } match (diagnosis.presence, diagnosis.problem_code) { // The case this command exists for. A phantom record is not a fault. (Some(false), _) => Verdict { @@ -593,9 +641,59 @@ mod tests { parent_instance_id: None, suspend_allowed: None, last_seen_secs_ago: None, + openable: None, } } + fn diag_openable(presence: Option, openable: Option) -> PortDiagnosis { + PortDiagnosis { + openable, + ..diag(presence, None) + } + } + + /// The bug this branch exists for: a port the host lists as healthy but + /// that cannot be opened used to render as "attached and healthy" with an + /// empty remedy, so deploy flashed fine and then timed out reopening it. + #[test] + fn unopenable_port_is_not_reported_as_healthy() { + let v = verdict(&diag_openable(Some(true), Some(false))); + assert!(v.summary.contains("cannot open"), "got: {}", v.summary); + assert!(v.summary.contains("permission denied"), "got: {}", v.summary); + assert!(!v.summary.contains("attached and healthy"), "got: {}", v.summary); + assert!(!v.remedy.is_empty(), "an unopenable port must carry a remedy"); + assert!(v.needs_hands); + } + + /// The remedy has to name a fix that survives re-enumeration. A one-shot + /// chmod is wiped when deploy cycles the board through BOOTSEL. + #[test] + fn permission_remedy_points_at_udev_not_a_chmod() { + let v = verdict(&diag_openable(Some(true), Some(false))); + assert!(v.remedy.contains("udev"), "got: {}", v.remedy); + assert!(v.remedy.contains("fbuild port udev"), "got: {}", v.remedy); + assert!(!v.remedy.contains("cable"), "cables are unrelated: {}", v.remedy); + assert!(!v.remedy.contains("BOOTSEL"), "BOOTSEL is unrelated: {}", v.remedy); + } + + /// Permission state must not mask the absent-board verdict, which is the + /// case this whole command was written for. + #[test] + fn openable_port_still_reports_the_presence_verdict() { + let v = verdict(&diag_openable(Some(true), Some(true))); + assert!(v.summary.contains("attached and healthy"), "got: {}", v.summary); + + let absent = verdict(&diag_openable(Some(false), None)); + assert!(absent.summary.contains("not attached"), "got: {}", absent.summary); + } + + /// An unprobed port must fall through untouched — `None` is "unknown", + /// never "fine". + #[test] + fn unprobed_openability_changes_nothing() { + assert_eq!(verdict(&diag_openable(Some(true), None)), verdict(&diag(Some(true), None))); + } + /// The headline: an absent board must be called out as absent, and the /// remedy must be "plug it in" — never a recovery procedure. #[test] diff --git a/crates/fbuild-cli/src/cli/port_scan.rs b/crates/fbuild-cli/src/cli/port_scan.rs index 648bd93c..9a0618f3 100644 --- a/crates/fbuild-cli/src/cli/port_scan.rs +++ b/crates/fbuild-cli/src/cli/port_scan.rs @@ -72,6 +72,20 @@ pub enum PortAction { #[arg(long, conflicts_with_all = ["port", "fix", "dry_run"])] hub: Option, }, + /// Print udev rules granting serial access for every vendor in the + /// FastLED/boards registry (FastLED/fbuild#1424). + /// + /// Prints rather than installs. On NixOS `/etc` is generated from + /// declarative config, so a written file there is out-of-band and liable + /// to be clobbered -- those users need the content for + /// `services.udev.extraRules`, not a mutation. Everywhere else, redirect + /// it yourself so the privileged write stays explicit. + Udev { + /// Group to grant access to. Defaults to `plugdev`, which unlike + /// `dialout` does not also confer modem/PPP access. + #[arg(long)] + group: Option, + }, } /// Top-level entry — dispatcher calls this. @@ -94,6 +108,38 @@ pub fn run_port(action: PortAction) -> Result<()> { super::port_doctor::run(port.as_deref(), hub.as_deref(), json) } } + PortAction::Udev { group } => run_udev(group.as_deref()), + } +} + +/// Emit udev rules derived from the ingested registry. +fn run_udev(group: Option<&str>) -> Result<()> { + use super::udev::{DEFAULT_UDEV_GROUP, UDEV_RULES_FILENAME, render_udev_rules}; + + // Refresh first, exactly as `scan` does: rules generated from a stale or + // absent overlay would silently omit vendors the user has plugged in. + populate_online_overlay(); + + let vids = fbuild_core::usb::online_vendor_vids(); + let group = group.unwrap_or(DEFAULT_UDEV_GROUP); + match render_udev_rules(&vids, group) { + Some(rules) => { + print!("{rules}"); + eprintln!( + "# {} vendor rule(s) from the FastLED/boards registry. \ + Install as /etc/udev/rules.d/{UDEV_RULES_FILENAME}.", + vids.len() + ); + Ok(()) + } + // Refuse rather than emit an empty file: one that looks configured + // and grants nothing is worse than none at all. + None => Err(FbuildError::Other( + "USB vendor registry is empty — cannot generate udev rules. Run \ + `fbuild port scan` once with network access to populate the \ + FastLED/boards cache, then retry." + .to_string(), + )), } } diff --git a/crates/fbuild-cli/src/cli/udev.rs b/crates/fbuild-cli/src/cli/udev.rs new file mode 100644 index 00000000..f615320f --- /dev/null +++ b/crates/fbuild-cli/src/cli/udev.rs @@ -0,0 +1,123 @@ +//! udev rule generation for Linux serial access (FastLED/fbuild#1424). +//! +//! On Linux, serial device nodes are `root:dialout 0660` and the invoking +//! user is usually not in `dialout`. `fbuild deploy` then flashes +//! successfully and cannot reopen the port it just flashed, which surfaces +//! as a post-deploy timeout rather than a permissions error. +//! +//! A one-shot `chmod` does not hold: the deploy cycle re-enumerates the board +//! (BOOTSEL -> application), and udev recreates the node with default +//! ownership before fbuild reopens it. Only a rule keyed on vendor ID +//! survives re-enumeration, which is why this generates rules rather than +//! touching device nodes. +//! +//! The vendor list comes from the ingested FastLED/boards registry, never a +//! local table -- see the USB VID/PID source-of-truth rule. A hand-written +//! copy would drift the moment a vendor is ingested. +//! +//! This module only *renders* text. Installing is deliberately left to the +//! caller: on NixOS `/etc` is generated from declarative config, so a written +//! file there is both out-of-band and liable to be clobbered. What those users +//! need is the content for `services.udev.extraRules`, not a mutation. + +/// Group granted access by generated rules. `plugdev` is the conventional +/// choice for pluggable non-storage hardware and, unlike `dialout`, does not +/// also confer modem/PPP access. +pub const DEFAULT_UDEV_GROUP: &str = "plugdev"; + +/// Filename callers should install as, if they install at all. The `99-` +/// prefix keeps it last so it overrides earlier distro rules. +pub const UDEV_RULES_FILENAME: &str = "99-fbuild.rules"; + +/// Render udev rules granting `group` access to every vendor in `vids`. +/// +/// Returns `None` when `vids` is empty. That is a real case -- the registry +/// overlay may not be installed -- and emitting a header with no rules would +/// hand back a file that looks configured and grants nothing. +pub fn render_udev_rules(vids: &[u16], group: &str) -> Option { + if vids.is_empty() { + return None; + } + let mut sorted: Vec = vids.to_vec(); + sorted.sort_unstable(); + sorted.dedup(); + + let mut out = String::new(); + out.push_str("# Generated by `fbuild port udev`. Do not edit by hand.\n"); + out.push_str("#\n"); + out.push_str("# Grants the listed group access to serial ports for every USB vendor\n"); + out.push_str("# in the FastLED/boards registry, so `fbuild deploy` can reopen a port\n"); + out.push_str("# after the board re-enumerates. Regenerate after upgrading fbuild to\n"); + out.push_str("# pick up newly ingested vendors.\n"); + out.push_str("#\n"); + out.push_str(&format!("# Install as /etc/udev/rules.d/{UDEV_RULES_FILENAME}, then:\n")); + out.push_str("# sudo udevadm control --reload-rules && sudo udevadm trigger\n"); + out.push_str(&format!("# Ensure your user is in the '{group}' group, then log out and back in.\n")); + out.push_str("#\n"); + out.push_str("# On NixOS, paste the rules below into services.udev.extraRules instead;\n"); + out.push_str("# /etc there is generated and a hand-written file will not persist.\n"); + out.push('\n'); + + for vid in sorted { + // Lowercase hex without an 0x prefix: udev compares ATTRS{idVendor} + // as a string against sysfs, which renders it exactly this way. An + // uppercase or 0x-prefixed value silently never matches. + out.push_str(&format!( + "SUBSYSTEM==\"tty\", ATTRS{{idVendor}}==\"{vid:04x}\", GROUP=\"{group}\", MODE=\"0660\"\n" + )); + } + Some(out) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn empty_registry_yields_no_rules() { + // Not an empty file: a header with no rules would look configured + // while granting nothing. + assert!(render_udev_rules(&[], DEFAULT_UDEV_GROUP).is_none()); + } + + #[test] + fn renders_lowercase_four_digit_hex() { + // udev matches ATTRS{idVendor} as a string against sysfs. "0x2E8A" + // or "2E8A" never match, and the failure is silent. + let out = render_udev_rules(&[0x2e8a, 0x303a], DEFAULT_UDEV_GROUP).unwrap(); + assert!(out.contains(r#"ATTRS{idVendor}=="2e8a""#)); + assert!(out.contains(r#"ATTRS{idVendor}=="303a""#)); + assert!(!out.contains("0x2e8a")); + assert!(!out.contains("2E8A")); + } + + #[test] + fn pads_short_vids_to_four_digits() { + let out = render_udev_rules(&[0x403], DEFAULT_UDEV_GROUP).unwrap(); + assert!(out.contains(r#"ATTRS{idVendor}=="0403""#), "{out}"); + } + + #[test] + fn sorts_and_dedups() { + let out = render_udev_rules(&[0x303a, 0x2e8a, 0x303a], DEFAULT_UDEV_GROUP).unwrap(); + let rules: Vec<&str> = out.lines().filter(|l| l.starts_with("SUBSYSTEM==")).collect(); + assert_eq!(rules.len(), 2, "duplicate VID must collapse: {out}"); + assert!(rules[0].contains("2e8a"), "not sorted: {out}"); + assert!(rules[1].contains("303a"), "not sorted: {out}"); + } + + #[test] + fn honors_the_requested_group() { + let out = render_udev_rules(&[0x2e8a], "wheel").unwrap(); + assert!(out.contains(r#"GROUP="wheel""#)); + assert!(!out.contains(r#"GROUP="plugdev""#)); + } + + #[test] + fn every_vid_gets_exactly_one_rule() { + let vids: Vec = vec![0x2e8a, 0x303a, 0x0403, 0x1a86, 0x10c4, 0x16c0]; + let out = render_udev_rules(&vids, DEFAULT_UDEV_GROUP).unwrap(); + let rules = out.lines().filter(|l| l.starts_with("SUBSYSTEM==")).count(); + assert_eq!(rules, vids.len(), "{out}"); + } +} diff --git a/crates/fbuild-core/src/usb/data.rs b/crates/fbuild-core/src/usb/data.rs index e64c241d..6cbacbb8 100644 --- a/crates/fbuild-core/src/usb/data.rs +++ b/crates/fbuild-core/src/usb/data.rs @@ -457,6 +457,31 @@ pub(crate) fn install_online_cache_map(map: HashMap) { *guard = Some(map); } +/// Every vendor VID present in the runtime overlay, ascending and deduped. +/// +/// Exists so callers that must enumerate the whole supported device set -- +/// udev rule generation is the motivating one (FastLED/fbuild#1424) -- can +/// derive it from the published FastLED/boards registry instead of carrying +/// their own vendor list. A hand-maintained copy would drift the moment a +/// vendor is ingested, which is precisely what the VID/PID source-of-truth +/// rule exists to prevent. +/// +/// Empty when no overlay is installed. Callers must treat that as "unknown", +/// never as "no devices are supported" -- emitting rules from an empty +/// registry would silently produce a file that grants nothing. +pub fn online_vendor_vids() -> Vec { + let Ok(guard) = ONLINE_MAP.read() else { + return Vec::new(); + }; + let Some(map) = guard.as_ref() else { + return Vec::new(); + }; + let mut vids: Vec = map.keys().map(|key| (key >> 16) as u16).collect(); + vids.sort_unstable(); + vids.dedup(); + vids +} + /// Runtime online overlay only (freshest, curated at workflow time). pub(crate) fn online_lookup(vid: u16, pid: u16) -> Option { let key = pack(vid, pid); diff --git a/crates/fbuild-core/src/usb/mod.rs b/crates/fbuild-core/src/usb/mod.rs index 2c235953..6ad8016a 100644 --- a/crates/fbuild-core/src/usb/mod.rs +++ b/crates/fbuild-core/src/usb/mod.rs @@ -25,7 +25,8 @@ pub mod resolver; pub use data::{ MANIFEST_URL, ONLINE_CACHE_TTL_SECS, USB_VID_JSON_URL, USB_VIDS_PROTO_ZSTD_URL, - install_online_cache, install_online_cache_proto_zstd, populate_online_cache_from_paths, + install_online_cache, install_online_cache_proto_zstd, online_vendor_vids, + populate_online_cache_from_paths, populate_online_cache_from_paths_and_urls, try_install_online_cache, try_install_online_cache_proto_zstd, }; From 0867aac36228998814129e76674aed6a2373854d Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Mon, 7 Sep 2026 05:41:37 -0700 Subject: [PATCH 2/3] fix(cli): open the port probe non-blocking, and fix the remedy spacing (#1424) Addresses CodeRabbit on #1425. Both findings were valid; the second was a bug I introduced. 1. `OpenOptions::new().read(true).open(port)` set neither O_NONBLOCK nor O_NOCTTY. On Linux a terminal with CLOCAL clear blocks until carrier detect, which would hang a command whose own docs promise a strictly read-only diagnostic; without O_NOCTTY the probe could also acquire a controlling terminal, so signals sent there would reach fbuild. Not theoretical: an open on a contended port measured 13.3 s on the bench that motivated this issue. Now uses `custom_flags(libc::O_NONBLOCK | libc::O_NOCTTY)`, with libc added to fbuild-cli under a `cfg(unix)` target since it was a workspace dependency but not a crate one. 2. The permission remedy carried literal runs of ~20 spaces: "...your user is not in that group..." The source was generated through a heredoc that consumed the `\` line-continuations as continuations of the *generating* language, joining the lines while keeping their indentation. Rebuilt from `concat!()` of separate literals, which cannot reproduce that. cargo check -p fbuild-cli --all-targets clean; `cargo test -p fbuild-cli port_doctor` 34 passed / 0 failed, including the four verdict tests that assert on the remedy text. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KkufoNxfnNRU9psT3R9F51 --- crates/fbuild-cli/Cargo.toml | 5 +++++ crates/fbuild-cli/src/cli/port_doctor.rs | 25 +++++++++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/crates/fbuild-cli/Cargo.toml b/crates/fbuild-cli/Cargo.toml index 1eddbd3e..cb07fe55 100644 --- a/crates/fbuild-cli/Cargo.toml +++ b/crates/fbuild-cli/Cargo.toml @@ -39,4 +39,9 @@ sha2 = { workspace = true } tempfile = { workspace = true } walkdir = { workspace = true } owo-colors = { workspace = true } + +# O_NONBLOCK / O_NOCTTY for the read-only serial probe in `port doctor` +# (FastLED/fbuild#1424). Unix-only: the probe itself is cfg'd to Linux. +[target.'cfg(unix)'.dependencies] +libc = { workspace = true } semver = { workspace = true } diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index 60a4b281..c58e63ab 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -89,7 +89,18 @@ pub fn diagnose(port: &DetectedPort, power_rows: &[(String, bool)]) -> PortDiagn #[cfg(target_os = "linux")] pub fn probe_openable(port: &str) -> Option { use std::io::ErrorKind; - match std::fs::OpenOptions::new().read(true).open(port) { + use std::os::unix::fs::OpenOptionsExt; + // O_NONBLOCK: without CLOCAL set, a terminal open blocks until carrier + // detect is asserted, which would hang a command documented as a quick + // read-only diagnostic. O_NOCTTY: never let the probe acquire a + // controlling terminal -- signals delivered to that terminal would then + // reach fbuild. Both matter here: an open on a contended port was + // measured at 13.3 s on the bench that motivated FastLED/fbuild#1424. + match std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NONBLOCK | libc::O_NOCTTY) + .open(port) + { Ok(_) => Some(true), Err(e) if e.kind() == ErrorKind::PermissionDenied => Some(false), // Busy, absent, or anything else is a different question that the @@ -115,8 +126,16 @@ pub fn verdict(diagnosis: &PortDiagnosis) -> Verdict { return Verdict { summary: "attached, but this process cannot open the port (permission denied)" .to_string(), - remedy: "serial nodes are typically root:dialout 0660 and your user is not in that group. Generate rules with `fbuild port udev`, install them as /etc/udev/rules.d/99-fbuild.rules, then `sudo udevadm control --reload-rules && sudo udevadm trigger`. Adding your user to the group works too, but needs a fresh login. A one-shot chmod does not hold: deploy re-enumerates the board and udev recreates the node" - .to_string(), + remedy: concat!( + "serial nodes are typically root:dialout 0660 and your user ", + "is not in that group. Generate rules with `fbuild port udev`, ", + "install them as /etc/udev/rules.d/99-fbuild.rules, then ", + "`sudo udevadm control --reload-rules && sudo udevadm trigger`. ", + "Adding your user to the group works too, but needs a fresh ", + "login. A one-shot chmod does not hold: deploy re-enumerates ", + "the board and udev recreates the node", + ) + .to_string(), needs_hands: true, }; } From 3b6d292970533a2d64b78e1409b1249d0f68635f Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Mon, 7 Sep 2026 12:10:21 -0700 Subject: [PATCH 3/3] fix(cli): validate --group before rendering it into a udev rule (#1424) Two findings from the review that I had missed by reading only the first comment rather than enumerating all of them. 1. Injection (CWE-74). `--group` reached the quoted udev value with no validation. The rendered file is one an operator installs as root, so --group 'plugdev\nSUBSYSTEM=="tty", MODE="0666"' would close the quote and append a world-writable rule for every serial device. There is no encoding that makes this safe: udev only unescapes \" inside a standard quoted string, and "\n" stays literal, so nothing neutralises an embedded newline. Reject instead. `is_valid_group_name` mirrors useradd(8)'s NAME_REGEX -- an initial alphanumeric or underscore, then alphanumerics, underscore, hyphen or dot, up to 32 characters -- and `render_udev_rules` returns None for anything else, alongside the existing empty-registry refusal. 2. rustfmt. I had never run `soldr cargo fmt`, so the formatting pipeline was failing on this branch. Tests cover quote injection, newline injection, empty, leading hyphen, leading dot, embedded space, semicolon and slash, plus the ordinary names that must keep working and the 32/33 character boundary. soldr cargo test -p fbuild-cli: 318 passed / 0 failed. soldr cargo fmt --all -- --check: clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KkufoNxfnNRU9psT3R9F51 --- crates/fbuild-cli/src/cli/mod.rs | 2 +- crates/fbuild-cli/src/cli/port_doctor.rs | 46 +++++++++++--- crates/fbuild-cli/src/cli/udev.rs | 77 +++++++++++++++++++++--- crates/fbuild-core/src/usb/mod.rs | 5 +- 4 files changed, 111 insertions(+), 19 deletions(-) diff --git a/crates/fbuild-cli/src/cli/mod.rs b/crates/fbuild-cli/src/cli/mod.rs index c485fb22..d3cfa822 100644 --- a/crates/fbuild-cli/src/cli/mod.rs +++ b/crates/fbuild-cli/src/cli/mod.rs @@ -34,7 +34,6 @@ pub mod monitor_parse; pub mod pio; pub mod plotter; pub mod port_doctor; -pub mod udev; pub mod port_doctor_fix; pub mod port_scan; pub mod purge; @@ -43,6 +42,7 @@ pub mod serial_probe; pub mod show; pub mod symbols_cmd; pub mod sync_cmd; +pub mod udev; // #1148 supplies the helper foundation; #1147 is deliberately its first // production consumer once RP2040 deployment can emit a typed request. #[allow( diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index c58e63ab..37a20169 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -678,9 +678,20 @@ mod tests { fn unopenable_port_is_not_reported_as_healthy() { let v = verdict(&diag_openable(Some(true), Some(false))); assert!(v.summary.contains("cannot open"), "got: {}", v.summary); - assert!(v.summary.contains("permission denied"), "got: {}", v.summary); - assert!(!v.summary.contains("attached and healthy"), "got: {}", v.summary); - assert!(!v.remedy.is_empty(), "an unopenable port must carry a remedy"); + assert!( + v.summary.contains("permission denied"), + "got: {}", + v.summary + ); + assert!( + !v.summary.contains("attached and healthy"), + "got: {}", + v.summary + ); + assert!( + !v.remedy.is_empty(), + "an unopenable port must carry a remedy" + ); assert!(v.needs_hands); } @@ -691,8 +702,16 @@ mod tests { let v = verdict(&diag_openable(Some(true), Some(false))); assert!(v.remedy.contains("udev"), "got: {}", v.remedy); assert!(v.remedy.contains("fbuild port udev"), "got: {}", v.remedy); - assert!(!v.remedy.contains("cable"), "cables are unrelated: {}", v.remedy); - assert!(!v.remedy.contains("BOOTSEL"), "BOOTSEL is unrelated: {}", v.remedy); + assert!( + !v.remedy.contains("cable"), + "cables are unrelated: {}", + v.remedy + ); + assert!( + !v.remedy.contains("BOOTSEL"), + "BOOTSEL is unrelated: {}", + v.remedy + ); } /// Permission state must not mask the absent-board verdict, which is the @@ -700,17 +719,28 @@ mod tests { #[test] fn openable_port_still_reports_the_presence_verdict() { let v = verdict(&diag_openable(Some(true), Some(true))); - assert!(v.summary.contains("attached and healthy"), "got: {}", v.summary); + assert!( + v.summary.contains("attached and healthy"), + "got: {}", + v.summary + ); let absent = verdict(&diag_openable(Some(false), None)); - assert!(absent.summary.contains("not attached"), "got: {}", absent.summary); + assert!( + absent.summary.contains("not attached"), + "got: {}", + absent.summary + ); } /// An unprobed port must fall through untouched — `None` is "unknown", /// never "fine". #[test] fn unprobed_openability_changes_nothing() { - assert_eq!(verdict(&diag_openable(Some(true), None)), verdict(&diag(Some(true), None))); + assert_eq!( + verdict(&diag_openable(Some(true), None)), + verdict(&diag(Some(true), None)) + ); } /// The headline: an absent board must be called out as absent, and the diff --git a/crates/fbuild-cli/src/cli/udev.rs b/crates/fbuild-cli/src/cli/udev.rs index f615320f..040d07f1 100644 --- a/crates/fbuild-cli/src/cli/udev.rs +++ b/crates/fbuild-cli/src/cli/udev.rs @@ -29,13 +29,35 @@ pub const DEFAULT_UDEV_GROUP: &str = "plugdev"; /// prefix keeps it last so it overrides earlier distro rules. pub const UDEV_RULES_FILENAME: &str = "99-fbuild.rules"; +/// Whether `group` is a plausible Unix group name. +/// +/// The rendered value lands inside a quoted udev field in a file an operator +/// installs as root, so a `"` or a newline here could close the quote and +/// append rules of the caller's choosing. udev only unescapes `\\"` inside a +/// standard quoted string, so there is no encoding that makes arbitrary input +/// safe -- reject it instead. Mirrors the useradd(8) NAME_REGEX: an initial +/// alphanumeric or underscore, then alphanumerics, underscore, hyphen or dot. +pub fn is_valid_group_name(group: &str) -> bool { + if group.is_empty() || group.len() > 32 { + return false; + } + let mut chars = group.chars(); + let first = chars.next().unwrap_or('\0'); + if !(first.is_ascii_alphanumeric() || first == '_') { + return false; + } + chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.') +} + /// Render udev rules granting `group` access to every vendor in `vids`. /// -/// Returns `None` when `vids` is empty. That is a real case -- the registry -/// overlay may not be installed -- and emitting a header with no rules would -/// hand back a file that looks configured and grants nothing. +/// Returns `None` when `vids` is empty, or when `group` is not a valid group +/// name. Both are real cases -- the registry overlay may not be installed, and +/// `--group` is caller-supplied -- and emitting a header with no rules, or a +/// rule built from an unvalidated group, would hand back a file that looks +/// configured while granting nothing or granting the wrong thing. pub fn render_udev_rules(vids: &[u16], group: &str) -> Option { - if vids.is_empty() { + if vids.is_empty() || !is_valid_group_name(group) { return None; } let mut sorted: Vec = vids.to_vec(); @@ -50,9 +72,13 @@ pub fn render_udev_rules(vids: &[u16], group: &str) -> Option { out.push_str("# after the board re-enumerates. Regenerate after upgrading fbuild to\n"); out.push_str("# pick up newly ingested vendors.\n"); out.push_str("#\n"); - out.push_str(&format!("# Install as /etc/udev/rules.d/{UDEV_RULES_FILENAME}, then:\n")); + out.push_str(&format!( + "# Install as /etc/udev/rules.d/{UDEV_RULES_FILENAME}, then:\n" + )); out.push_str("# sudo udevadm control --reload-rules && sudo udevadm trigger\n"); - out.push_str(&format!("# Ensure your user is in the '{group}' group, then log out and back in.\n")); + out.push_str(&format!( + "# Ensure your user is in the '{group}' group, then log out and back in.\n" + )); out.push_str("#\n"); out.push_str("# On NixOS, paste the rules below into services.udev.extraRules instead;\n"); out.push_str("# /etc there is generated and a hand-written file will not persist.\n"); @@ -100,12 +126,49 @@ mod tests { #[test] fn sorts_and_dedups() { let out = render_udev_rules(&[0x303a, 0x2e8a, 0x303a], DEFAULT_UDEV_GROUP).unwrap(); - let rules: Vec<&str> = out.lines().filter(|l| l.starts_with("SUBSYSTEM==")).collect(); + let rules: Vec<&str> = out + .lines() + .filter(|l| l.starts_with("SUBSYSTEM==")) + .collect(); assert_eq!(rules.len(), 2, "duplicate VID must collapse: {out}"); assert!(rules[0].contains("2e8a"), "not sorted: {out}"); assert!(rules[1].contains("303a"), "not sorted: {out}"); } + #[test] + fn rejects_a_group_that_could_break_out_of_the_quoted_value() { + // The rendered value sits inside a quoted udev field in a file an + // operator installs as root; a quote or newline could append rules. + for bad in [ + "plug\"dev", + "plugdev\nSUBSYSTEM==\"tty\", MODE=\"0666\"", + "", + "-leading-hyphen", + ".leading-dot", + "has space", + "semi;colon", + "sl/ash", + ] { + assert!( + render_udev_rules(&[0x2e8a], bad).is_none(), + "must reject {bad:?}" + ); + } + } + + #[test] + fn accepts_ordinary_group_names() { + for good in ["plugdev", "dialout", "users", "_svc", "grp.1", "a-b_c.d"] { + assert!(is_valid_group_name(good), "must accept {good:?}"); + } + } + + #[test] + fn rejects_an_overlong_group_name() { + assert!(!is_valid_group_name(&"a".repeat(33))); + assert!(is_valid_group_name(&"a".repeat(32))); + } + #[test] fn honors_the_requested_group() { let out = render_udev_rules(&[0x2e8a], "wheel").unwrap(); diff --git a/crates/fbuild-core/src/usb/mod.rs b/crates/fbuild-core/src/usb/mod.rs index 6ad8016a..27c47873 100644 --- a/crates/fbuild-core/src/usb/mod.rs +++ b/crates/fbuild-core/src/usb/mod.rs @@ -26,9 +26,8 @@ pub mod resolver; pub use data::{ MANIFEST_URL, ONLINE_CACHE_TTL_SECS, USB_VID_JSON_URL, USB_VIDS_PROTO_ZSTD_URL, install_online_cache, install_online_cache_proto_zstd, online_vendor_vids, - populate_online_cache_from_paths, - populate_online_cache_from_paths_and_urls, try_install_online_cache, - try_install_online_cache_proto_zstd, + populate_online_cache_from_paths, populate_online_cache_from_paths_and_urls, + try_install_online_cache, try_install_online_cache_proto_zstd, }; #[cfg(test)] pub use embedded::vendor_name as embedded_vendor_name;