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..a58e1f141b 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,13 +25,13 @@ 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); + return errno_or_else(EIO); return 0; } @@ -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); @@ -91,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; } 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", 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); } diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index f7cbb9c72d..36966122f4 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(); } @@ -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 @@ -3378,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) { @@ -4022,7 +4027,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=.");