feat(suidhelper): source device binaries from config, drop caller --bin

The unprivileged node used to pass `--bin <path>` to the setuid helper for
every losetup/dmsetup/blockdev op. Letting the caller name the binary the
helper escalates to run is a needless trust hole, even with SafeBin checks.

Move the paths into the helper-owned config (/etc/hyper/config.toml) with
sane defaults (/usr/sbin/{losetup,dmsetup,blockdev}); the helper validates
each as a SafeBin (absolute, root-owned, non-writable, exact basename) at
dispatch, as the real uid, before acquiring root. The `--bin` argument is
gone from both sides.

Also:
- Add a `dmsetup targets` op so the dm-target readiness probe runs through
  the helper (it opens /dev/mapper/control, which needs root) instead of
  shelling dmsetup directly as the BEAM user.
- An absent config file now falls back to the built-in defaults (trusted,
  compiled into the root-owned binary); a present-but-untrusted file stays
  fatal. Drop the Elixir-side *_path config and per-tool presence checks.
- Add :mix to the dialyzer PLT so the Mix tasks resolve Mix.raise/shell.

Author: markovejnovic

lib/hyper/config.ex

@@ -16,9 +16,6 @@ defmodule Hyper.Config do
   @parent_cgroup Application.compile_env(:hyper, :cgroup_parent, "hyper")
   @uid_gid_range Application.compile_env!(:hyper, :uid_gid_range)
   @layer_dir Application.compile_env!(:hyper, :layer_dir)
-  @losetup_path Application.compile_env(:hyper, :losetup_path, "losetup")
-  @dmsetup_path Application.compile_env(:hyper, :dmsetup_path, "dmsetup")
-  @blockdev_path Application.compile_env(:hyper, :blockdev_path, "blockdev")
   @skopeo_path Application.compile_env(:hyper, :skopeo_path, "skopeo")
   @umoci_path Application.compile_env(:hyper, :umoci_path, nil)
   @mke2fs_path Application.compile_env(:hyper, :mke2fs_path, "mke2fs")
@@ -111,15 +108,6 @@ defmodule Hyper.Config do
   @spec layer_dir :: Path.t()
   def layer_dir, do: @layer_dir
 
-  @doc "Path to the losetup binary."
-  def losetup_path, do: @losetup_path
-
-  @doc "Path to the dmsetup binary."
-  def dmsetup_path, do: @dmsetup_path
-
-  @doc "Path to the blockdev binary."
-  def blockdev_path, do: @blockdev_path
-
   @doc "Path to the skopeo binary (used by `Hyper.Img.OciLoader` to pull OCI images)."
   def skopeo_path, do: @skopeo_path
 
@@ -132,15 +120,20 @@ defmodule Hyper.Config do
   @doc "Path to the mke2fs binary (used by `Hyper.Img.OciLoader` to build the ext4 rootfs)."
   def mke2fs_path, do: @mke2fs_path
 
+  # Where `cargo xtask install` (via `mix suidhelper.install`) drops the helper.
+  @default_suid_helper "/usr/local/bin/hyper-suidhelper"
+
   @doc """
-  Path to the setuid-root device helper (`hyper-suidhelper`). Required: the node
-  runs unprivileged and routes every `losetup`/`dmsetup`/`blockdev` operation
-  through it.
+  Path to the setuid-root device helper (`hyper-suidhelper`). The node runs
+  unprivileged and routes every `losetup`/`dmsetup`/`blockdev` operation through
+  it.
 
-  Runtime config (host-specific), so it can be set per node without recompiling.
+  Defaults to `#{@default_suid_helper}`, the install path used by `mix
+  suidhelper.install`. Runtime config (host-specific), so an operator who
+  installs it elsewhere can override per node without recompiling.
   """
   @spec suid_helper :: Path.t()
-  def suid_helper, do: Application.fetch_env!(:hyper, :suid_helper)
+  def suid_helper, do: Application.get_env(:hyper, :suid_helper, @default_suid_helper)
 
   @doc """
   Directory for per-VM scratch (writable-layer COW) files. Must be node-local and

lib/hyper/suid_helper.ex

@@ -16,7 +16,7 @@ defmodule Hyper.SuidHelper do
   self-test and reports the base path it was compiled against.
   """
 
-  alias Hyper.SuidHelper.{Blockdev, Dmsetup, Expected, Losetup}
+  alias Hyper.SuidHelper.{Dmsetup, Expected}
 
   use OpenTelemetryDecorator
 
@@ -51,18 +51,20 @@ defmodule Hyper.SuidHelper do
   end
 
   @doc """
-  Check that the setuid helper and every tool it execs are usable on this
-  machine: the helper binary is present, is the build this release expects
-  (`verify_version/0`), then each tool submodule's own check.
+  Check that the setuid helper is usable on this machine: the helper binary is
+  present, is the build this release expects (`verify_version/0`), and the kernel
+  exposes the device-mapper targets we need (`Dmsetup.test_system/0`, which also
+  exercises the helper's configured `dmsetup` binary).
+
+  The `losetup`/`blockdev` binaries are validated by the helper the first time
+  each is used; their paths live in the helper's own config, not here.
   """
   @spec test_system() :: :ok | {:error, term()}
   @decorate with_span("Hyper.SuidHelper.test_system")
   def test_system do
     with :ok <- helper_present(),
-         :ok <- verify_version(),
-         :ok <- Losetup.test_system(),
-         :ok <- Dmsetup.test_system() do
-      Blockdev.test_system()
+         :ok <- verify_version() do
+      Dmsetup.test_system()
     end
   end
 

lib/hyper/suid_helper/blockdev.ex

@@ -11,18 +11,9 @@ defmodule Hyper.SuidHelper.Blockdev do
   @spec device_sectors(Path.t()) :: {:ok, pos_integer()} | {:error, err()}
   @decorate with_span("Hyper.SuidHelper.Blockdev.device_sectors", include: [:path])
   def device_sectors(path) do
-    case SuidHelper.exec(["blockdev", "--bin", Hyper.Config.blockdev_path(), "--getsz", path]) do
+    case SuidHelper.exec(["blockdev", "--getsz", path]) do
       {:ok, %{"sectors" => n}} -> {:ok, n}
       {:error, _} = err -> err
     end
   end
-
-  @doc "Check the blockdev binary is present."
-  @spec test_system() :: :ok | {:error, :blockdev_not_found}
-  @decorate with_span("Hyper.SuidHelper.Blockdev.test_system")
-  def test_system do
-    if System.find_executable(Hyper.Config.blockdev_path()),
-      do: :ok,
-      else: {:error, :blockdev_not_found}
-  end
 end

lib/hyper/suid_helper/dmsetup.ex

@@ -59,14 +59,7 @@ defmodule Hyper.SuidHelper.Dmsetup do
   @spec remove(String.t()) :: :ok | {:error, err()}
   @decorate with_span("Hyper.SuidHelper.Dmsetup.remove", include: [:name])
   def remove(name) do
-    case SuidHelper.exec([
-           "dmsetup",
-           "--bin",
-           Hyper.Config.dmsetup_path(),
-           "remove",
-           "--retry",
-           name
-         ]) do
+    case SuidHelper.exec(["dmsetup", "remove", "--retry", name]) do
       {:ok, _} -> :ok
       {:error, _} = err -> err
     end
@@ -76,39 +69,31 @@ defmodule Hyper.SuidHelper.Dmsetup do
   @spec message(String.t(), String.t()) :: :ok | {:error, err()}
   @decorate with_span("Hyper.SuidHelper.Dmsetup.message", include: [:name, :message])
   def message(name, message) do
-    argv =
-      ["dmsetup", "--bin", Hyper.Config.dmsetup_path(), "message", name, "--message", message]
-
-    case SuidHelper.exec(argv) do
+    case SuidHelper.exec(["dmsetup", "message", name, "--message", message]) do
       {:ok, _} -> :ok
       {:error, _} = err -> err
     end
   end
 
   @doc """
-  Check the dmsetup binary is present and the kernel exposes the dm targets we
-  use (snapshot, thin, thin-pool).
+  Verify the kernel exposes the dm targets we use (snapshot, thin, thin-pool).
+
+  Routes through the setuid helper: `dmsetup targets` opens `/dev/mapper/control`,
+  which needs root, and the BEAM runs unprivileged. The helper validates its
+  configured `dmsetup` binary before running it, so a missing or unsafe binary
+  surfaces here too.
   """
   @spec test_system() :: :ok | {:error, term()}
   @decorate with_span("Hyper.SuidHelper.Dmsetup.test_system")
   def test_system do
-    if System.find_executable(Hyper.Config.dmsetup_path()),
-      do: test_targets(),
-      else: {:error, :dmsetup_not_found}
-  end
-
-  @doc "Verify the kernel exposes the dm targets we use (snapshot, thin, thin-pool)."
-  @spec test_targets() :: :ok | {:error, term()}
-  @decorate with_span("Hyper.SuidHelper.Dmsetup.test_targets")
-  def test_targets do
-    case System.cmd(Hyper.Config.dmsetup_path(), ["targets"], stderr_to_stdout: true) do
-      {out, 0} ->
+    case SuidHelper.exec(["dmsetup", "targets"]) do
+      {:ok, %{"output" => out}} ->
         have = parse_targets(out)
         missing = Enum.reject(@required_targets, &MapSet.member?(have, &1))
         if missing == [], do: :ok, else: {:error, {:missing_dm_targets, missing}}
 
-      {out, code} ->
-        {:error, {:dmsetup_targets_failed, code, String.trim(out)}}
+      {:error, {code, msg}} ->
+        {:error, {:dmsetup_targets_failed, code, msg}}
     end
   end
 
@@ -145,9 +130,7 @@ defmodule Hyper.SuidHelper.Dmsetup do
   # create flags (e.g. `--readonly`). Returns the `/dev/mapper/<name>` path.
   @spec create(String.t(), String.t(), [String.t()]) :: {:ok, Path.t()} | {:error, err()}
   defp create(name, table, flags) do
-    argv =
-      ["dmsetup", "--bin", Hyper.Config.dmsetup_path(), "create", name] ++
-        flags ++ ["--table", table]
+    argv = ["dmsetup", "create", name] ++ flags ++ ["--table", table]
 
     case SuidHelper.exec(argv) do
       {:ok, %{"device" => dev}} -> {:ok, dev}

lib/hyper/suid_helper/losetup.ex

@@ -11,7 +11,7 @@ defmodule Hyper.SuidHelper.Losetup do
   @spec attach_ro(Path.t()) :: {:ok, Path.t()} | {:error, err()}
   @decorate with_span("Hyper.SuidHelper.Losetup.attach_ro", include: [:path])
   def attach_ro(path) do
-    case SuidHelper.exec(["losetup", "--bin", Hyper.Config.losetup_path(), "attach", path]) do
+    case SuidHelper.exec(["losetup", "attach", path]) do
       {:ok, %{"device" => dev}} -> {:ok, dev}
       {:error, _} = err -> err
     end
@@ -21,14 +21,7 @@ defmodule Hyper.SuidHelper.Losetup do
   @spec attach_rw(Path.t()) :: {:ok, Path.t()} | {:error, err()}
   @decorate with_span("Hyper.SuidHelper.Losetup.attach_rw", include: [:path])
   def attach_rw(path) do
-    case SuidHelper.exec([
-           "losetup",
-           "--bin",
-           Hyper.Config.losetup_path(),
-           "attach",
-           "--rw",
-           path
-         ]) do
+    case SuidHelper.exec(["losetup", "attach", "--rw", path]) do
       {:ok, %{"device" => dev}} -> {:ok, dev}
       {:error, _} = err -> err
     end
@@ -38,18 +31,9 @@ defmodule Hyper.SuidHelper.Losetup do
   @spec detach(Path.t()) :: :ok | {:error, err()}
   @decorate with_span("Hyper.SuidHelper.Losetup.detach", include: [:dev])
   def detach(dev) do
-    case SuidHelper.exec(["losetup", "--bin", Hyper.Config.losetup_path(), "detach", dev]) do
+    case SuidHelper.exec(["losetup", "detach", dev]) do
       {:ok, _} -> :ok
       {:error, _} = err -> err
     end
   end
-
-  @doc "Check the losetup binary is present."
-  @spec test_system() :: :ok | {:error, :losetup_not_found}
-  @decorate with_span("Hyper.SuidHelper.Losetup.test_system")
-  def test_system do
-    if System.find_executable(Hyper.Config.losetup_path()),
-      do: :ok,
-      else: {:error, :losetup_not_found}
-  end
 end

mix.exs

@@ -24,6 +24,10 @@ defmodule Hyper.MixProject do
         # Cache the PLTs in a stable, gitignored dir so CI can cache them.
         plt_local_path: "priv/plts",
         plt_core_path: "priv/plts",
+        # `:mix` is needed so the Mix tasks under `lib/mix/tasks` (which call
+        # `Mix.raise/1`, `Mix.shell/0`, and implement the `Mix.Task` behaviour)
+        # resolve instead of tripping `unknown_function`.
+        plt_add_apps: [:mix],
         # Verify @specs against actual returns, and flag ignored return values.
         flags: [:unmatched_returns, :extra_return, :missing_return]
       ]

native/suidhelper/src/config.rs

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: AGPL-3.0-only
 //! Runtime host configuration, read from a single root-owned TOML file.
 
+use crate::util::safe_bin::{self, SafeBin};
 use crate::util::safe_file::{self, IsRegularFile, OnlyRootWritable, RootOwner, SafeFile};
 use crate::util::safe_path::{self, IsAbsolute, SafePath, StrictComponents};
 use nix::fcntl::OFlag;
@@ -41,16 +42,57 @@ fn config_path() -> PathBuf {
 }
 
 /// Hyper's /etc/hyper/config.toml file format.
+///
+/// The device-tool paths are read from here (never from the unprivileged
+/// caller, which is why there is no `--bin` argument): the helper alone decides
+/// which binary it escalates to run. Each defaults to its usual location and is
+/// validated as a [`SafeBin`] before use.
 #[derive(Debug, Clone, Deserialize)]
 pub struct Config {
     work_dir: PathBuf,
+    #[serde(default = "default_dmsetup")]
+    dmsetup: PathBuf,
+    #[serde(default = "default_losetup")]
+    losetup: PathBuf,
+    #[serde(default = "default_blockdev")]
+    blockdev: PathBuf,
+}
+
+// The default data root. Must match the Elixir node's `@dev_work_dir`, which it
+// uses when the same config file is absent, so both sides agree (see
+// `Hyper.Node.check_helper_base`).
+fn default_work_dir() -> PathBuf {
+    PathBuf::from("/srv/hyper")
+}
+
+fn default_dmsetup() -> PathBuf {
+    PathBuf::from("/usr/sbin/dmsetup")
+}
+
+fn default_losetup() -> PathBuf {
+    PathBuf::from("/usr/sbin/losetup")
+}
+
+fn default_blockdev() -> PathBuf {
+    PathBuf::from("/usr/sbin/blockdev")
+}
+
+impl Default for Config {
+    fn default() -> Self {
+        Self {
+            work_dir: default_work_dir(),
+            dmsetup: default_dmsetup(),
+            losetup: default_losetup(),
+            blockdev: default_blockdev(),
+        }
+    }
 }
 
 impl Config {
     /// The process-wide config, loaded once (and forced unprivileged by
-    /// [`Config::init`]). A load failure is fatal: the helper cannot safely
-    /// operate without a trusted data root, so it prints the error and exits
-    /// rather than guessing a default.
+    /// [`Config::init`]). An absent file yields the built-in defaults; a
+    /// *present but untrusted* file (wrong owner/mode, malformed) is fatal -
+    /// the helper prints the error and exits rather than trusting it.
     pub fn get() -> &'static Config {
         LazyLock::force(&CONFIG)
     }
@@ -74,6 +116,21 @@ impl Config {
         self.work_dir.join("jails")
     }
 
+    /// The validated `dmsetup` binary the helper will run.
+    pub fn dmsetup(&self) -> Result<SafeBin<"dmsetup">, safe_bin::Error> {
+        SafeBin::from_path(&self.dmsetup)
+    }
+
+    /// The validated `losetup` binary the helper will run.
+    pub fn losetup(&self) -> Result<SafeBin<"losetup">, safe_bin::Error> {
+        SafeBin::from_path(&self.losetup)
+    }
+
+    /// The validated `blockdev` binary the helper will run.
+    pub fn blockdev(&self) -> Result<SafeBin<"blockdev">, safe_bin::Error> {
+        SafeBin::from_path(&self.blockdev)
+    }
+
     /// Read, ownership-check, parse, and validate the config file. See the module
     /// docs for the trust model.
     pub fn safe_load() -> Result<Self, LoadingError> {
@@ -82,7 +139,18 @@ impl Config {
         let safe_path: SafePath<IsAbsolute, StrictComponents> = path.clone().try_into()?;
 
         let file: SafeFile<IsRegularFile, RootOwner, OnlyRootWritable> =
-            SafeFile::open(&safe_path, OFlag::O_RDONLY)?;
+            match SafeFile::open(&safe_path, OFlag::O_RDONLY) {
+                Ok(file) => file,
+                // A genuinely-absent file means "use the built-in defaults": those
+                // are compiled into this root-owned binary, so they are trusted. Any
+                // OTHER failure - a present but wrong-owner/mode file, an I/O error -
+                // stays fatal, because it is a signal (someone put an untrusted file
+                // there), not an absence.
+                Err(safe_file::ValidationError::Open(nix::errno::Errno::ENOENT)) => {
+                    return Ok(Self::default())
+                }
+                Err(e) => return Err(e.into()),
+            };
 
         let body = std::io::read_to_string(std::fs::File::from(file.into_owned_fd()))
             .map_err(|_| LoadingError::Unreadable(path.clone()))?;

native/suidhelper/src/tools/dmsetup/mod.rs

@@ -52,6 +52,9 @@ enum DmOp {
         #[arg(long)]
         message: ThinMessage,
     },
+    /// List the target types the kernel device-mapper exposes. Read-only, but
+    /// still needs root: it opens `/dev/mapper/control`.
+    Targets,
 }
 
 #[derive(Serialize)]
@@ -60,6 +63,7 @@ pub enum DmsetupOut {
     Created { device: PathBuf },
     Removed,
     Messaged,
+    Targets { output: String },
 }
 
 pub struct Dmsetup {
@@ -105,6 +109,9 @@ impl IsTool for Dmsetup {
                     .arg("0")
                     .arg(message.to_string());
             }
+            DmOp::Targets => {
+                cmd.arg("targets");
+            }
         }
 
         cmd.env_clear().output()
@@ -124,6 +131,9 @@ impl IsTool for Dmsetup {
             },
             DmOp::Remove { .. } => DmsetupOut::Removed,
             DmOp::Message { .. } => DmsetupOut::Messaged,
+            DmOp::Targets => DmsetupOut::Targets {
+                output: String::from_utf8_lossy(&out.stdout).into_owned(),
+            },
         })
     }
 }