From 6212baaf828f530398723dfbc9ba1d1aa07368cb Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Sat, 13 Jun 2026 09:25:28 +0200 Subject: [PATCH 1/8] vmspawn: enforce an identifier grammar for QEMU config types/ids/keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit qemu_config_section()/qemu_config_key() write section types and keys unquoted ([type], key = …), but the type check only rejected '\n' while the structurally identical id/key checks rejected more, even though the unquoted type slot is the most sensitive. Replace the three ad-hoc denylists with one allowlist matching QEMU's identifier grammar ([A-Za-z0-9._-], non-empty). This is merely hardening, all callers pass constant input today. For the value check use string_is_safe() for now, which is a bit stricter than QEMUs own config validation. Add test-vmspawn-qemu-config covering both. Co-developed-by: Claude Opus 4.8 Signed-off-by: Paul Meyer --- src/vmspawn/meson.build | 6 +- src/vmspawn/test-vmspawn-qemu-config.c | 120 +++++++++++++++++++++++++ src/vmspawn/vmspawn-qemu-config.c | 33 ++++--- 3 files changed, 140 insertions(+), 19 deletions(-) create mode 100644 src/vmspawn/test-vmspawn-qemu-config.c diff --git a/src/vmspawn/meson.build b/src/vmspawn/meson.build index 6bc31c77c6..76617cc6d4 100644 --- a/src/vmspawn/meson.build +++ b/src/vmspawn/meson.build @@ -7,7 +7,6 @@ endif vmspawn_sources = files( 'vmspawn.c', 'vmspawn-bind-volume.c', - 'vmspawn-qemu-config.c', 'vmspawn-qmp.c', 'vmspawn-varlink.c', 'vmspawn-settings.c', @@ -16,6 +15,7 @@ vmspawn_sources = files( ) vmspawn_extract_sources = files( 'vmspawn-util.c', + 'vmspawn-qemu-config.c', ) executables += [ @@ -29,4 +29,8 @@ executables += [ 'sources' : files('test-vmspawn-util.c'), 'objects' : ['systemd-vmspawn'], }, + test_template + { + 'sources' : files('test-vmspawn-qemu-config.c'), + 'objects' : ['systemd-vmspawn'], + }, ] diff --git a/src/vmspawn/test-vmspawn-qemu-config.c b/src/vmspawn/test-vmspawn-qemu-config.c new file mode 100644 index 0000000000..001a431981 --- /dev/null +++ b/src/vmspawn/test-vmspawn-qemu-config.c @@ -0,0 +1,120 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "alloc-util.h" +#include "memstream-util.h" +#include "tests.h" +#include "vmspawn-qemu-config.h" + +/* Render a single "key = value" pair through qemu_config_key() and return both the function's return code + * and (on success, when ret is set) the rendered bytes. */ +static int render_key(char **ret, const char *key, const char *value) { + _cleanup_(memstream_done) MemStream m = {}; + FILE *f; + int r; + + assert_se(f = memstream_init(&m)); + + r = qemu_config_key(f, key, value); + if (r < 0) + return r; + + if (ret) + assert_se(memstream_finalize(&m, ret, NULL) >= 0); + + return r; +} + +static int render_section(char **ret, const char *type, const char *id) { + _cleanup_(memstream_done) MemStream m = {}; + FILE *f; + int r; + + assert_se(f = memstream_init(&m)); + + r = qemu_config_section(f, type, id); + if (r < 0) + return r; + + if (ret) + assert_se(memstream_finalize(&m, ret, NULL) >= 0); + + return r; +} + +TEST(qemu_config_key_valid) { + _cleanup_free_ char *out = NULL; + + ASSERT_OK(render_key(&out, "readonly", "on")); + ASSERT_STREQ(out, " readonly = \"on\"\n"); + + /* Keys may carry the full identifier charset. */ + out = mfree(out); + ASSERT_OK(render_key(&out, "confidential-guest-support", "snp0")); + ASSERT_STREQ(out, " confidential-guest-support = \"snp0\"\n"); +} + +TEST(qemu_config_value_permits_path_bytes) { + _cleanup_free_ char *out = NULL; + + /* Legitimate path bytes, including backslash, must pass through a quoted value verbatim. */ + ASSERT_OK(render_key(&out, "file", "/usr/share/edk2/ovmf/OVMF_CODE.fd")); + ASSERT_STREQ(out, " file = \"/usr/share/edk2/ovmf/OVMF_CODE.fd\"\n"); + + out = mfree(out); + ASSERT_OK(render_key(&out, "file", "/weird/but\\legal/path")); + ASSERT_STREQ(out, " file = \"/weird/but\\legal/path\"\n"); + + /* '=', spaces, brackets etc. are all fine inside a quoted value. */ + out = mfree(out); + ASSERT_OK(render_key(&out, "cpus", "1,sockets=1")); + ASSERT_STREQ(out, " cpus = \"1,sockets=1\"\n"); +} + +TEST(qemu_config_value_rejects_quote_and_newline) { + /* These two bytes are the only ones that can break out of the quoted token; both must be refused. */ + ASSERT_ERROR(render_key(NULL, "file", "ab\"cd"), EINVAL); + ASSERT_ERROR(render_key(NULL, "file", "ab\ncd"), EINVAL); +} + +TEST(qemu_config_key_name_rejects_structure) { + /* Key names are emitted unquoted, so they must be plain identifiers. */ + ASSERT_ERROR(render_key(NULL, "fo=o", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "fo\no", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "fo\"o", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "fo o", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "foo]", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "/foo", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "", "v"), EINVAL); +} + +TEST(qemu_config_section_valid) { + _cleanup_free_ char *out = NULL; + + ASSERT_OK(render_section(&out, "drive", "ovmf-code")); + ASSERT_STREQ(out, "\n[drive \"ovmf-code\"]\n"); + + out = mfree(out); + ASSERT_OK(render_section(&out, "smp-opts", NULL)); + ASSERT_STREQ(out, "\n[smp-opts]\n"); +} + +TEST(qemu_config_section_type_rejects_structure) { + /* The section type is emitted unquoted as "[type]" — a ']' or newline here would let a caller + * close the header early or open a new section. */ + ASSERT_ERROR(render_section(NULL, "drive]", "id"), EINVAL); + ASSERT_ERROR(render_section(NULL, "dr\nive", "id"), EINVAL); + ASSERT_ERROR(render_section(NULL, "dr ive", "id"), EINVAL); + ASSERT_ERROR(render_section(NULL, "dr\"ive", "id"), EINVAL); + ASSERT_ERROR(render_section(NULL, "", "id"), EINVAL); +} + +TEST(qemu_config_section_id_rejects_structure) { + /* The id is quoted, but ']' and backslash are still hardened against future runtime data. */ + ASSERT_ERROR(render_section(NULL, "drive", "i\"d"), EINVAL); + ASSERT_ERROR(render_section(NULL, "drive", "i\nd"), EINVAL); + ASSERT_ERROR(render_section(NULL, "drive", "i]d"), EINVAL); + ASSERT_ERROR(render_section(NULL, "drive", "i\\d"), EINVAL); + ASSERT_ERROR(render_section(NULL, "drive", ""), EINVAL); +} + +DEFINE_TEST_MAIN(LOG_DEBUG); diff --git a/src/vmspawn/vmspawn-qemu-config.c b/src/vmspawn/vmspawn-qemu-config.c index 08908ff294..d17e841e35 100644 --- a/src/vmspawn/vmspawn-qemu-config.c +++ b/src/vmspawn/vmspawn-qemu-config.c @@ -5,22 +5,19 @@ #include "alloc-util.h" #include "errno-util.h" #include "log.h" +#include "string-util.h" #include "vmspawn-qemu-config.h" -static bool qemu_config_type_valid(const char *type) { - return !strchr(type, '\n'); -} - -static bool qemu_config_id_valid(const char *id) { - return !strpbrk(id, "\"\n"); -} - -static bool qemu_config_key_name_valid(const char *key) { - return !strpbrk(key, "=\n"); +/* Enforce QEMU's identifier grammar, so runtime data can never inject config structure. */ +static bool qemu_config_identifier_valid(const char *s) { + return !isempty(s) && in_charset(s, ALPHANUMERICAL ".-_"); } +/* Values are written quoted ('key = "%s"') and QEMU reads them literally between the quotes (no escapes), + * only '"' and newline can break out. Nevertheless run the value through string_is_safe(), which is + * stricter than QEMU's own parser. Relax if a valid use case occurs. */ static bool qemu_config_value_valid(const char *value) { - return !strpbrk(value, "\"\n"); + return string_is_safe(value, STRING_ALLOW_EMPTY | STRING_ALLOW_BACKSLASHES | STRING_ALLOW_GLOBS); } int qemu_config_key(FILE *f, const char *key, const char *value) { @@ -28,10 +25,10 @@ int qemu_config_key(FILE *f, const char *key, const char *value) { assert(key); assert(value); - if (!qemu_config_key_name_valid(key)) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config key '%s' contains '=' or newline.", key); + if (!qemu_config_identifier_valid(key)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config key '%s' is not a valid identifier.", key); if (!qemu_config_value_valid(value)) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config value '%s' contains quote or newline.", value); + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config value '%s' contains unsafe characters.", value); if (fprintf(f, " %s = \"%s\"\n", key, value) < 0) return -errno_or_else(EIO); @@ -64,12 +61,12 @@ int qemu_config_section_impl(FILE *f, const char *type, const char *id, ...) { assert(f); assert(type); - if (!qemu_config_type_valid(type)) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config section type '%s' contains newline.", type); + if (!qemu_config_identifier_valid(type)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config section type '%s' is not a valid identifier.", type); if (id) { - if (!qemu_config_id_valid(id)) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config section id '%s' contains quote or newline.", id); + if (!qemu_config_identifier_valid(id)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config section id '%s' is not a valid identifier.", id); fprintf(f, "\n[%s \"%s\"]\n", type, id); } else fprintf(f, "\n[%s]\n", type); From 9f28938a842f8e936d2a59b3039c4db9c0e8077f Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Sat, 13 Jun 2026 09:58:53 +0200 Subject: [PATCH 2/8] vmspawn: don't leak prior arg_directory in ephemeral branch The ephemeral-snapshot branch did a bare arg_directory = strdup(...), orphaning the -D/--directory allocation from parse_argv. Use free_and_strdup(), matching the idiom already used elsewhere in the file. Co-developed-by: Claude Opus 4.8 Signed-off-by: Paul Meyer --- src/vmspawn/vmspawn.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index f7cbb9c72d..3b53283bd8 100644 --- a/src/vmspawn/vmspawn.c +++ b/src/vmspawn/vmspawn.c @@ -3172,8 +3172,8 @@ static int run_virtual_machine(int kvm_device_fd, int vhost_device_fd) { if (r < 0) return log_error_errno(r, "Failed to create ephemeral snapshot of '%s': %m", arg_directory); - arg_directory = strdup(snapshot_directory); - if (!arg_directory) + r = free_and_strdup(&arg_directory, snapshot_directory); + if (r < 0) return log_oom(); } From b10a938d8deeb880a1a481ddcb723a3bd42fd1ee Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Sat, 13 Jun 2026 10:01:27 +0200 Subject: [PATCH 3/8] vmspawn: reject --initrd= without --linux= for directory boot determine_kernel() ran boot-entry discovery under a !arg_linux guard only. With --initrd= but no --linux=, discovery overwrote arg_initrds with the discovered entry's own initrds (leaking the user's strv) and set arg_linux, so verify_arguments()'s "--initrd= needs --linux=" check, which runs afterwards, never fired: the user's --initrd= was silently discarded. Reject the combination before discovery. This also keeps the discovery store leak-free. Co-developed-by: Claude Opus 4.8 Signed-off-by: Paul Meyer --- src/vmspawn/vmspawn.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index 3b53283bd8..70dcc9914a 100644 --- a/src/vmspawn/vmspawn.c +++ b/src/vmspawn/vmspawn.c @@ -4022,7 +4022,14 @@ static int determine_kernel(void) { int r; if (!arg_linux && arg_directory) { - /* A kernel is required for directory type images so attempt to find one under /boot and /efi */ + /* A kernel is required for directory type images so attempt to find one under /boot and /efi. + * Reject a user --initrd= first: discovery would overwrite (and leak) it, and the + * --initrd=/--linux= consistency check in verify_arguments() is bypassed once discovery + * sets arg_linux. */ + if (!strv_isempty(arg_initrds)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "--initrd= cannot be used with --directory= unless --linux= is also specified."); + r = discover_boot_entry(arg_directory, &arg_linux, &arg_initrds); if (r < 0) return log_error_errno(r, "Failed to locate UKI in directory type image, please specify one with --linux=."); From c90e57241138a2fbafe5b64a52287cbbea834ee0 Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Sat, 13 Jun 2026 10:37:20 +0200 Subject: [PATCH 4/8] vmspawn: null freed fields and drain subscribers before bridge teardown vmspawn_varlink_context_free() discarded the sd_varlink_server_unref() and vmspawn_qmp_bridge_free() return values, leaving ctx->varlink_server and ctx->bridge dangling. No current handler reads those fields, but use the assign-back idiom so the fields are NULL during any synchronous callback regardless of future changes. Also drain subscribers before freeing the bridge, so subscriber teardown can't run against a half-freed bridge. Co-developed-by: Claude Opus 4.8 Signed-off-by: Paul Meyer --- src/vmspawn/vmspawn-varlink.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vmspawn/vmspawn-varlink.c b/src/vmspawn/vmspawn-varlink.c index ebfdd87876..57230c8e8f 100644 --- a/src/vmspawn/vmspawn-varlink.c +++ b/src/vmspawn/vmspawn-varlink.c @@ -577,10 +577,11 @@ VmspawnVarlinkContext* vmspawn_varlink_context_free(VmspawnVarlinkContext *ctx) if (!ctx) return NULL; - sd_varlink_server_unref(ctx->varlink_server); - vmspawn_qmp_bridge_free(ctx->bridge); + ctx->varlink_server = sd_varlink_server_unref(ctx->varlink_server); drain_event_subscribers(&ctx->subscribed); + ctx->bridge = vmspawn_qmp_bridge_free(ctx->bridge); + return mfree(ctx); } From 0fc82c6d139b6319664517750cecffb54b64f21c Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Sat, 13 Jun 2026 10:57:46 +0200 Subject: [PATCH 5/8] vmspawn: return negative errno from qemu_config_key()/_section_impl() errno_or_else() already returns a negative errno, so "return -errno_or_else(EIO)" returned a positive value on fprintf()/ferror() failure. Callers checking "r < 0" therefore missed the write error and treated it as success. Drop the spurious negation; these were the only two "-errno_or_else" uses in the tree. Co-developed-by: Claude Opus 4.8 Signed-off-by: Paul Meyer --- src/vmspawn/vmspawn-qemu-config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vmspawn/vmspawn-qemu-config.c b/src/vmspawn/vmspawn-qemu-config.c index d17e841e35..a58e1f141b 100644 --- a/src/vmspawn/vmspawn-qemu-config.c +++ b/src/vmspawn/vmspawn-qemu-config.c @@ -31,7 +31,7 @@ int qemu_config_key(FILE *f, const char *key, const char *value) { return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config value '%s' contains unsafe characters.", value); if (fprintf(f, " %s = \"%s\"\n", key, value) < 0) - return -errno_or_else(EIO); + return errno_or_else(EIO); return 0; } @@ -88,7 +88,7 @@ int qemu_config_section_impl(FILE *f, const char *type, const char *id, ...) { va_end(ap); if (ferror(f)) - return -errno_or_else(EIO); + return errno_or_else(EIO); return 0; } From 4f6ce566e16ead262f49b0ca18b87b933486cdc6 Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Sat, 13 Jun 2026 11:24:13 +0200 Subject: [PATCH 6/8] vmspawn: reject oversized fstab.extra credential before int-cast merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fstab.extra merge prepends the existing credential via asprintf("%.*s", (int) existing->size, …). MachineCredential.size is size_t, so for a credential >INT_MAX the (int) cast yields a negative precision, which C treats as omitted — turning %.*s into an unbounded read past the allocation. Reject such a credential up front with EFBIG; for all realistic sizes the merge is unchanged. Co-developed-by: Claude Opus 4.8 Signed-off-by: Paul Meyer --- src/vmspawn/vmspawn.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index 70dcc9914a..55c650af1b 100644 --- a/src/vmspawn/vmspawn.c +++ b/src/vmspawn/vmspawn.c @@ -3305,6 +3305,11 @@ static int run_virtual_machine(int kvm_device_fd, int vhost_device_fd) { if (existing) { _cleanup_free_ char *combined = NULL; + if (existing->size >= INT_MAX) + return log_error_errno(SYNTHETIC_ERRNO(EFBIG), + "Existing fstab.extra credential is too large (%zu bytes).", + existing->size); + if (existing->size > 0 && existing->data[existing->size - 1] != '\n') r = asprintf(&combined, "%.*s\n%s", (int) existing->size, existing->data, fstab_extra); else From ef7d64cd09bda6dbbe5b7a5534f41b0dfe78435e Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Sat, 13 Jun 2026 11:38:02 +0200 Subject: [PATCH 7/8] vmspawn: fix QEMU target names for ppc64/mips64el/mipsel hosts architecture_to_qemu_table mapped four 64-bit/LE architectures to wrong qemu-system- names: MIPS64_LE and MIPS_LE both to "mips" (the BE MIPS32 target), and PPC64/PPC64_LE to "ppc" (32-bit PowerPC). The string feeds both find_qemu_binary() ("qemu-system-" + name) and the firmware.json architecture match, so on those hosts vmspawn looked up a wrong or nonexistent binary and never matched a firmware descriptor. Map them to the real targets: mips64el, mipsel, and ppc64 (qemu-system-ppc64 runs LE pSeries guests; there is no qemu-system-ppc64le). Co-developed-by: Claude Opus 4.8 Signed-off-by: Paul Meyer --- src/vmspawn/vmspawn-util.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/vmspawn/vmspawn-util.c b/src/vmspawn/vmspawn-util.c index c5ab40a0d2..8ecd266000 100644 --- a/src/vmspawn/vmspawn-util.c +++ b/src/vmspawn/vmspawn-util.c @@ -32,11 +32,11 @@ static const char* const architecture_to_qemu_table[_ARCHITECTURE_MAX] = { [ARCHITECTURE_X86_64] = "x86_64", /* differs from our name */ [ARCHITECTURE_X86] = "i386", /* differs from our name */ [ARCHITECTURE_LOONGARCH64] = "loongarch64", - [ARCHITECTURE_MIPS64_LE] = "mips", /* differs from our name */ - [ARCHITECTURE_MIPS_LE] = "mips", /* differs from our name */ + [ARCHITECTURE_MIPS64_LE] = "mips64el", /* differs from our name */ + [ARCHITECTURE_MIPS_LE] = "mipsel", /* differs from our name */ [ARCHITECTURE_PARISC] = "hppa", /* differs from our name */ - [ARCHITECTURE_PPC64_LE] = "ppc", /* differs from our name */ - [ARCHITECTURE_PPC64] = "ppc", /* differs from our name */ + [ARCHITECTURE_PPC64_LE] = "ppc64", /* differs from our name; qemu-system-ppc64 runs LE guests too */ + [ARCHITECTURE_PPC64] = "ppc64", /* differs from our name */ [ARCHITECTURE_PPC] = "ppc", [ARCHITECTURE_RISCV32] = "riscv32", [ARCHITECTURE_RISCV64] = "riscv64", From ba1203562ef20f84ca746113ad182336c430d2c4 Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Sat, 13 Jun 2026 12:07:45 +0200 Subject: [PATCH 8/8] vmspawn: don't abort VM launch when an optional swtpm fails to start When start_tpm() failed under TPM autodetect (arg_tpm <= 0), the swtpm block logged "Failed to start tpm, ignoring" but then fell through to event_add_child_pidref() with the still-unset child PidRef. That returns -ESRCH (PIDREF_NULL fails its pidref_is_set() guard), and the VM launch fails. Register the child only on start_tpm() success. Co-developed-by: Claude Opus 4.8 Signed-off-by: Paul Meyer --- src/vmspawn/vmspawn.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index 55c650af1b..36966122f4 100644 --- a/src/vmspawn/vmspawn.c +++ b/src/vmspawn/vmspawn.c @@ -3383,15 +3383,15 @@ static int run_virtual_machine(int kvm_device_fd, int vhost_device_fd) { return log_error_errno(r, "Failed to start tpm: %m"); log_debug_errno(r, "Failed to start tpm, ignoring: %m"); + } else { + _cleanup_(sd_event_source_unrefp) sd_event_source *source = NULL; + r = event_add_child_pidref(event, &source, &child, WEXITED, on_child_exit, /* userdata= */ NULL); + if (r < 0) + return r; + + pidref_done(&child); + children[n_children++] = TAKE_PTR(source); } - - _cleanup_(sd_event_source_unrefp) sd_event_source *source = NULL; - r = event_add_child_pidref(event, &source, &child, WEXITED, on_child_exit, /* userdata= */ NULL); - if (r < 0) - return r; - - pidref_done(&child); - children[n_children++] = TAKE_PTR(source); } if (tpm_socket_address) {