nvme: parse untrusted input in a sandbox by dwsuse · Pull Request #4183 · linux-nvme/nvme-cli

dwsuse · 2026-10-14T18:50:28Z

Fifth step of privilege separation (#3879), on top of #4182 (review the last seven commits): the parsers of untrusted input run in a sandboxed child process, whether or not nvme-cli runs privileged. Rebased onto the new argconfig parser.

Behind a new sandbox meson option (disabled by default for now, Linux and FreeBSD). NVME_SANDBOX=0 disables the sandbox, e.g. for a debugger; NVME_SANDBOX=1 enables it in any build.

One primitive, one code path: shr_sandbox_run(fn, arg, regions, nr) runs the parser in a forked child and copies the memory regions it wrote back to the same addresses, which a fork has in common with the child. Without a sandbox (disabled, unsupported, valgrind, Windows) it calls the parser in process. The parsers and their callers are the same in both modes; each caller only names the regions and checks the result.

The sandbox: the child keeps only stdio and the result pipe, so it cannot reach the privsep helper socket. Linux: a seccomp filter allowing memory management, write, the terminal queries of stdio (TCGETS, TCGETS2, TIOCGWINSZ), getrandom, signals to itself and exit; other system calls fail with EPERM so libc falls back gracefully. FreeBSD: descriptor rights limited, then Capsicum. Only a child which did not enter the sandbox may report that it found none; any other failure is an error and leaves the regions unchanged.

Parsers:

the option parser of argconfig_parse() and argconfig_parse_global(): the new parser no longer permutes argv, so the regions are the position, the operands, --help/--version, and the values and seen flags of the options including included tables and the global options. This also covers the global options in front of the command, which main() parses before it decides on the privileges, so the separate sandboxed privsep lookup of the previous version is gone.
the result is kept only if the parser can produce it: position and operands within the arguments, booleans, only values of options which were given, strings into the arguments or the value table, numbers in the range of the option or one of its named values, enums one of their names, lists their bits. A taken-over parser can hand back neither pointers nor numbers the commands do not expect.
nvme-cli.conf: argconfig_sandbox_run() runs it the same way: the regions are the global options and the file buffer (nvme: split command lookup and config parsing for privsep #4185), strings have to point into the file
--set-options: the split buffer and the option list (nvme: split command lookup and config parsing for privsep #4185); errors are reported after parsing, so they also reach the JSON output

Not covered: string conversions commands do themselves after option parsing (CSV lists, UUIDs etc.).

Testing:

sandbox unit test: regions and other memory, in-process mode, denied open, closed descriptors, terminal queries on a pty, other ioctls, tgkill of another process, data after the result, abort, exit without a result
argconfig, plugin dispatch and config file unit tests run in both modes; the comparison with the frozen getopt parser includes the sandboxed parser for the two-word command lines; a test feeds the result check states a parser of the command line or of a text can and cannot produce (each rule mutation-checked)
CLI test comparing output with and without the sandbox and checking that it is used; the mock CLI tests also run with NVME_SANDBOX=1
full test suite with the sandbox enabled and disabled (gcc and clang, --werror), every commit on its own
FreeBSD with Capsicum (tests in both modes, smoke test on a real device), Windows (x86_64, aarch64) builds
the real-root checks of nvme: drop root privileges through the privsep helper #4182 on a VM with the sandbox enabled and disabled

igaw · 2026-10-15T07:37:16Z

+	r->quiet = nvme_args.quiet;
+	r->dry_run = nvme_args.dry_run;
+	r->no_retries = nvme_args.no_retries;
+	r->no_ioctl_probing = nvme_args.no_ioctl_probing;


is this serializing/desiarizling really necessary? can't we just transfer nvme_args?

Yes, done that way: nvme_args itself and the file buffer are the regions (25f524b). For that the file is parsed in place and the strings point into the buffer instead of being strdup()ed (#4185).

Copilot

🟡 Changes recommended

Unresolved confinement gaps, fail-open setup handling and lost JSON diagnostics prevent approval.

5 open findings

What changed in this PR

Adds optional parser sandboxing as the next privilege-separation step, building on #4182.

Changes:

Runs argument, configuration and --set-options parsers in confined children with parent-side report validation.
Adds Linux seccomp, FreeBSD Capsicum and runtime sandbox controls.
Expands unit and CLI tests for sandbox behavior and parsing equivalence.

File	Description
tests/unit/c/test-plugin.c	Tests command lookup and sandboxed dispatch.
tests/unit/c/test-global-config.c	Runs configuration tests in both modes.
tests/unit/c/test-argconfig-report.c	Tests rejection of malformed reports.
tests/unit/c/test-argconfig-parse.c	Compares sandboxed and direct parsing.
tests/unit/c/meson.build	Registers parser tests and dependencies.
tests/cli/nvme_sandbox_test.py	Tests output equivalence and sandbox use.
tests/cli/nvme_privsep_test.py	Tests privileged-helper integration.
tests/cli/meson.build	Adds sandbox and helper test variants.
src/sandbox.h	Declares CLI sandbox controls.
src/sandbox.c	Implements sandbox enablement policy.
src/privsep.h	Declares privilege-separation interfaces.
src/privsep.c	Sandboxes privilege-drop decisions.
src/plugin.h	Adds command privilege metadata.
src/plugin.c	Implements command lookup and privilege checks.
src/nvme.c	Initializes privilege separation early.
src/nvme-cmds-fabrics.c	Marks commands requiring retained privileges.
src/nvme-cmds-enumerate.c	Marks enumeration privilege requirements.
src/nvme-cmds-deprecated.c	Marks deprecated key-command privilege requirements.
src/meson.build	Includes sandbox and privilege-separation sources.
src/global-ctx.c	Sandboxes `--set-options` parsing.
src/global-config.c	Sandboxes configuration parsing and validates reports.
src/argconfig.c	Serializes and validates sandboxed parser results.
shared/tests/test-sandbox-util.c	Tests confinement and result handling.
shared/tests/test-ini-util.c	Tests separate configuration-file reading.
shared/tests/meson.build	Registers sandbox utility tests.
shared/sandbox-util.h	Defines the shared sandbox interface.
shared/sandbox-util.c	Implements confinement and child-result transport.
shared/sandbox-util-win.c	Provides Windows unsupported fallback.
shared/meson.build	Selects platform sandbox implementation.
shared/ini-util.h	Declares separate INI-file reading.
shared/ini-util.c	Separates file reading from parsing.
plugins/zns/zns.c	Marks listing privilege requirements.
plugins/wdc/wdc-nvme.c	Marks telemetry privilege requirements.
plugins/utils/utils.c	Marks metadata-command privilege policy.
plugins/solidigm/solidigm-nvme.c	Marks version-command privilege policy.
plugins/seagate/seagate-nvme.c	Marks version-command privilege policy.
plugins/sandisk/sandisk-nvme.c	Marks telemetry privilege requirements.
plugins/registry/registry-nvme.c	Marks registry-list privilege requirements.
plugins/netapp/netapp-nvme.c	Marks enumeration privilege requirements.
plugins/nbft/nbft-plugin.c	Marks NBFT privilege requirements.
plugins/micron/micron-nvme.c	Marks privileged and no-device commands.
plugins/keys/keys-plugin.c	Marks key-management privilege requirements.
plugins/huawei/huawei-nvme.c	Marks listing privilege requirements.
plugins/exclusion/exclusion-nvme.c	Marks exclusion-management privilege requirements.
plugins/config/config-nvme.c	Marks configuration-command privilege requirements.
meson.build	Configures sandbox support.
meson_options.txt	Adds the disabled-by-default sandbox option.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

dwsuse · 2026-10-15T09:09:15Z

+
+static int sandbox_enter(void)
+{
+	return cap_enter() ? -errno : 0;


Fixed in f1a2ca6: the child keeps only stdio and the result pipe, and on FreeBSD limits their rights to write, fstat and the terminal ioctls before cap_enter().

dwsuse · 2026-10-15T09:09:18Z

+		close(fds[0]);
+		sandbox_child(fds[1], fn, arg, size);


Fixed in f1a2ca6: the child closes every descriptor but stdio and the result pipe before entering the sandbox, so the helper socket is gone. The sandbox test checks that an inherited descriptor cannot be written.

Split handle_plugin() into plugin_lookup(), which parses the global options and finds what to do for a command line, and plugin_call_run(), which does it. The lookup runs nothing and prints only the usage errors of the global options. "nvme <plugin>-<command>" no longer modifies the arguments of the caller. No functional change. This allows to decide on the command before running it, e.g. whether it needs root privileges, with the same code which runs it. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Add shr_ini_read_file(), which reads and checks a file the way shr_ini_parse_file() does and hands back its content, and shr_ini_parse_inplace(), which parses a buffer without copying it, so that the strings passed to the callback point into the buffer. A caller can then keep the strings of a configuration file without copying each of them, and read the file and parse its content in different places. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Parse nvme-cli.conf in place and let the strings of nvme_args point into the file buffer instead of duplicating each value. The buffer of every loaded file is kept, as strings of nvme_args may point into it. No functional change. The settings of the file are then held by nvme_args and the file buffer alone. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Parse --set-options into a list of options up to the first invalid one, and then apply the list to the global context and report the error. The parser no longer prints anything. No functional change: the valid options in front of an invalid one are still applied before the error is reported. Signed-off-by: Daniel Wagner <dwagner@suse.de>

main() looks up the command, including the global options in front of it, before it loads nvme-cli.conf, and runs what it found afterwards. The file skips the global options given on the command line, so the command line still wins over the file; a count such as --verbose still adds up as if the file came first. Registering the global options forgets which ones were given, for the fuzz tests which run main() more than once. No functional change. This allows to decide on the command before anything else runs, e.g. whether it needs root privileges. Signed-off-by: Daniel Wagner <dwagner@suse.de>

libnvme_get_nsid(), libnvme_reset_subsystem() and libnvme_rescan_ns() return a negative error code. Only for a direct handle errno happens to hold the same value; a privsep handle gets the error code from the helper and leaves errno alone. Check the return value instead. With a privsep handle, get-feature, set-feature and fdp set-events failed on a controller device instead of falling back to all namespaces, and ns get-id and format could report success after a failure. Signed-off-by: Daniel Wagner <dwagner@suse.de>

A handle opened through the privsep helper drives a local device with the ioctl interface, only in the helper. Report it as direct, so that callers like nvme format still rescan the namespaces, update the block size or reset the controller after a format. Signed-off-by: Daniel Wagner <dwagner@suse.de>

libnvme_set_ioctl_probing() only changed the context of the client; the helper still probed for the 64 bit passthru ioctls, e.g. for nvme --no-ioctl-probing. Send the setting with OPEN, in the reserved word now called options, and open the handle in a helper context without probing. The helper rejects unknown options. Signed-off-by: Daniel Wagner <dwagner@suse.de>

With privilege separation, nvme-cli drops its root privileges and opens the devices through the libnvme helper. The helper forwards the device commands only, so the commands which do anything else must keep root for now: the commands without a device argument (fabrics, enumeration, keys, exclusions, configuration, registry, NBFT) and the device commands which also access the PCI config space, the kernel log or root owned paths (Micron vs-pcie-stats, clear-pcie-correctable-errors and vs-internal-log, WDC and Sandisk reason-identifier and vs-telemetry-controller-option, registry update and delete). Add an explicit needs_root flag to struct command and set it for them. The list is what needs to be addressed before nvme-cli can drop the privileges for all commands. Nothing uses the flag yet. Signed-off-by: Daniel Wagner <dwagner@suse.de>

When root runs nvme-cli on behalf of a user and the command does not need root, start the libnvme privsep helper, drop the privileges and open the devices through the helper. The decision runs first in main(), on the command plugin_lookup() found, before the configuration file or the options of the command are parsed. The privileges are dropped to SUDO_UID/SUDO_GID or to NVME_PRIVSEP_UID/NVME_PRIVSEP_GID. The environment is trusted only when nvme-cli runs as root, and setuid is not supported: a setuid nvme-cli keeps running as it was started. Without any of the variables root keeps its privileges, which leaves scripts and services unchanged. The supplementary groups become the target group only; looking up the ones of the target user would run NSS as root. If the helper cannot be started or the privileges cannot be dropped, nvme-cli fails instead of running privileged. Commands with an MI device (mctp:<net>,<eid>) keep root, as MI uses the MCTP sockets directly. A command which needs root does not run once the privileges are dropped, should a deprecated command forward to one. The code is the same without privilege separation support, except that it never starts a helper. For the tests, NVME_PRIVSEP_TEST_HELPER names a helper which an unprivileged nvme-cli uses as well; it is ignored as root and in secure-execution mode. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Add a test for the privilege separation of nvme-cli: device commands start the helper and the helper opens the devices, while commands which keep root, MI devices, help and version don't. The test needs NVME_PRIVSEP_TEST_HELPER and is skipped when run as root, which ignores it. In privsep builds, also run the mock CLI tests (print, pif-sts, init-pi-tags, dry-run, sel, host-metadata, discovery-logs, sed) a second time with the commands going through the helper. Signed-off-by: Daniel Wagner <dwagner@suse.de>

shr_read_all() reads text: it NUL-terminates and discards what does not fit. Add shr_read_full(), which reads binary data up to a length or to the end of the file and returns how much it read. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Add shr_sandbox_run(), which runs a parser in a forked child process that can only allocate memory, write to stdout and stderr and exit. The child then copies the memory regions the parser wrote back to the same addresses in the caller, which a fork has in common with the child. Without a sandbox, i.e. when disabled, on systems without support or under valgrind, the parser runs in the caller. Either way the caller sees the same result, so it has a single code path. The child keeps only stdio and the result pipe open; in particular it cannot use the privsep helper socket. On Linux it installs a seccomp filter: memory management, write, the terminal queries of stdio and word wrapping (TCGETS, TCGETS2, TIOCGWINSZ), getrandom, signals to itself for abort(), what sanitizer runtimes need, and exit. Other system calls fail with EPERM, so libc falls back gracefully, e.g. gettext(). A filter rather than strict mode, which cannot be entered under an existing filter (systemd, containers). On FreeBSD the child limits the rights of its descriptors and enters Capsicum capability mode. Windows has no fork(), so the parsers run in process there. Only a child which did not enter the sandbox may report that it found none; any other failure of the child is an error and leaves the regions unchanged. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Add the sandbox meson option (Linux and FreeBSD, disabled for now) and enable the parser sandbox in main() accordingly. NVME_SANDBOX=0 disables it, e.g. for a debugger; NVME_SANDBOX=1 enables it in any build. Nothing uses the sandbox yet. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Run the option parser of argconfig_parse() and argconfig_parse_global() through shr_sandbox_run(). The regions are what the parser writes: the position in the arguments, the operands, whether --help or --version was given, and the values and seen flags of the options, including those of the included tables and the global options. The parser itself is unchanged and the same in both modes; the help, the usage errors and the operands are still handled by the caller. This also covers the global options in front of the command, which main() parses before it decides on the privileges. Afterwards argconfig checks that the result is one the parser can produce: the position and the operands within the arguments, booleans, and only values of options which were given. A string has to point into the arguments or the value table, a number has to be in the range of the option or one of its named values, an enum one of its names, a list of its bits. A sandboxed parser taken over by its input thus can hand back neither pointers nor numbers the commands do not expect. The argconfig and plugin dispatch unit tests run in both modes, the comparison with the getopt based parser includes the sandboxed parser for the two-word command lines, and a new test checks the validation of the result. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Parse the configuration file through argconfig_sandbox_run(), which runs a parser other than the one of the command line the same way: the regions are the values of the options the parser sets and the text it parses in place, and afterwards the values are checked like those of the command line, except that strings have to point into the text. The parser of nvme-cli.conf sets the global options through their table, so argconfig knows them all. The unit tests of the configuration file run in both modes, and the test of the result validation covers a parser of a text. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Parse --set-options through shr_sandbox_run(). The regions are the split buffer, the option list and its count and errors; all pointers must point into the buffer. The errors are reported after parsing, so they also reach the JSON output in the sandbox. Signed-off-by: Daniel Wagner <dwagner@suse.de>

Add a test which compares the output of nvme-cli with and without the parser sandbox for options, global options and --set-options, and which checks that the sandbox is used. Also run the mock CLI tests a second time with NVME_SANDBOX=1. Signed-off-by: Daniel Wagner <dwagner@suse.de>

igaw reviewed Oct 15, 2026

View reviewed changes

Comment thread shared/sandbox-util.c Outdated

igaw reviewed Oct 15, 2026

View reviewed changes

Comment thread src/sandbox.h Outdated

igaw reviewed Oct 15, 2026

View reviewed changes

Comment thread src/argconfig.c

igaw reviewed Oct 15, 2026

View reviewed changes

igaw requested a balanced review from Copilot October 15, 2026 07:42

Copilot started reviewing on behalf of igaw October 15, 2026 07:42

View session

Copilot AI reviewed Oct 15, 2026

View reviewed changes

dwsuse force-pushed the privsep-sandbox branch from cf6a32c to 6a4e29b Compare October 15, 2026 09:07

dwsuse mentioned this pull request Oct 15, 2026

nvme: split command lookup and config parsing for privsep #4185

Draft

dwsuse added 18 commits October 17, 2026 15:04

shared: add shr_read_full()

1e8f62b

shr_read_all() reads text: it NUL-terminates and discards what does not fit. Add shr_read_full(), which reads binary data up to a length or to the end of the file and returns how much it read. Signed-off-by: Daniel Wagner <dwagner@suse.de>

dwsuse force-pushed the privsep-sandbox branch from 6a4e29b to b3e9236 Compare October 17, 2026 15:49

Navigation Menu

Repository navigation

nvme: parse untrusted input in a sandbox - #4183

nvme: parse untrusted input in a sandbox#4183
dwsuse wants to merge 18 commits into
linux-nvme:masterfrom
dwsuse:privsep-sandbox

dwsuse commented Oct 14, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igaw Oct 15, 2026

Uh oh!

dwsuse Oct 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

dwsuse Oct 15, 2026

Uh oh!

Uh oh!

dwsuse Oct 15, 2026

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dwsuse commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igaw Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

dwsuse Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

dwsuse Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dwsuse Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dwsuse commented Oct 14, 2026 •

edited

Loading