Download raw body.
remove TOCTOU in vmd config and defer disk detection
Hi,
On Mon, Apr 13, 2026 at 10:06:21AM -0400, Dave Voutila wrote:
> vmd's config parsing opens disk images at parse time to attempt
> detection of disk type (raw vs. qcow2). This is a TOCTOU as vmd doesn't
> keep these files open and locked, so nothing prevents these files from
> changing before vm launch.
>
> For instance, if a vm owner has write-access to the disk image, nothing
> prevents them from converting the image from raw to qcow2 or the
> reverse. Nothing also prevents the file from being deleted. Nothing
> prevents the file permissions from changing. Etc. etc.
>
> realpath(3) is still used by parse.y so if the file *is* deleted, config
> validation will still fail.
>
> It would be excessive to keep the files open while vmd is running, so
> instead defer to detection to open time. This shifts to using an enum
> instead of ints with #DEFINEs to represent the 4 states:
> invalid/uninitialized, detection needed (auto), raw, and qcow2.
>
> I'm slowly chipping away at some fundamental issues in qcow2
> support. After this, I have a new vmd process to delegate file opening
> as the vm owner but may wait for after release for that type of change.
>
> ok?
I'm running this diff in my SEV setup and on my laptop. I'm not
seeing regressions. However, I'm mostly using qcow2 images (with
and without base images). Use of raw images is limited to a few
instances.
Diff is ok
> diff refs/heads/master refs/heads/vmd-parse_disk
> commit - 6d57bcee831e743a742d07d09e624416a482c89a
> commit + f69a11f3a0772e7b500e5ada30cb9bc522f060a8
> blob - 0f3958c20c8b1b9bf704e8dca8caa441fb7284ea
> blob + 9461759b25a9a96b1798f0d6d27a30510bc0782d
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -32,6 +32,7 @@
> #include <imsg.h>
>
> #include "proc.h"
> +#include "virtio.h"
> #include "vmd.h"
>
> /* Supported bridge types */
> @@ -190,10 +191,12 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
> int diskfds[VM_MAX_DISKS_PER_VM][VM_MAX_BASE_PER_DISK];
> struct vmd_if *vif;
> struct vmop_create_params *vmc = &vm->vm_params;
> + enum vm_disk_fmt type;
> unsigned int i, j;
> int fd = -1, cdromfd = -1, kernfd = -1;
> int *tapfds = NULL;
> - int n = 0, aflags, oflags, ret = -1;
> + int aflags, oflags, ret = -1;
> + ssize_t n = 0;
> char ifname[IF_NAMESIZE], *s;
> char path[PATH_MAX], base[PATH_MAX];
> unsigned int unit;
> @@ -330,7 +333,6 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
> oflags = O_RDWR | O_EXLOCK | O_NONBLOCK;
> aflags = R_OK | W_OK;
> for (j = 0; j < VM_MAX_BASE_PER_DISK; j++) {
> - /* Stat disk[i] to ensure it is a regular file */
> if ((diskfds[i][j] = open(path, oflags)) == -1) {
> log_warn("can't open disk %s",
> vmc->vmc_disks[i]);
> @@ -338,6 +340,10 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
> goto fail;
> }
>
> + /*
> + * Check if it's a regular file and accessible to
> + * the user starting the vm.
> + */
> if (vm_checkaccess(diskfds[i][j],
> vmc->vmc_checkaccess & VMOP_CREATE_DISK,
> uid, aflags) == -1) {
> @@ -347,6 +353,23 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
> goto fail;
> }
>
> + /* Identify the disk type if unknown. */
> + type = vmc->vmc_disktypes[i];
> + if (type == VMDF_AUTO) {
> + type = virtio_get_disktype(diskfds[i][j]);
> + vmc->vmc_disktypes[i] = type;
> + }
> + if (type != VMDF_RAW && type != VMDF_QCOW2) {
> + log_warnx("vm \"%s\" invalid disk format",
> + vmc->vmc_name);
> + ret = EINVAL;
> + goto fail;
> + }
> +
> + /* We're done if it's not a QCOW disk image. */
> + if (type != VMDF_QCOW2)
> + break;
> +
> /*
> * Clear the write and exclusive flags for base images.
> * All writes should go to the top image, allowing them
> @@ -354,16 +377,20 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, ui
> */
> oflags = O_RDONLY | O_NONBLOCK;
> aflags = R_OK;
> - n = virtio_get_base(diskfds[i][j], base, sizeof(base),
> - vmc->vmc_disktypes[i], path);
> - if (n == 0)
> - break;
> +
> + /* Resolve the path of the next base image, if any. */
> + n = virtio_qcow2_get_base(diskfds[i][j], base,
> + sizeof(base), path);
> if (n == -1) {
> log_warnx("vm \"%s\" unable to read "
> "base for disk %s", vmc->vmc_name,
> vmc->vmc_disks[i]);
> goto fail;
> }
> + /* Are we at the last base image layer? */
> + if (n == 0)
> + break;
> +
> (void)strlcpy(path, base, sizeof(path));
> }
> }
> blob - 2c03621a5123d7c656f23f545d2610d68e5a43af
> blob + 3faa030f0a9de0047e72d3b8adbde01486c6a97f
> --- usr.sbin/vmd/parse.y
> +++ usr.sbin/vmd/parse.y
> @@ -88,8 +88,8 @@ int symset(const char *, const char *, int);
> char *symget(const char *);
>
> ssize_t parse_size(char *, int64_t);
> -int parse_disk(char *, int);
> -unsigned int parse_format(const char *);
> +int parse_disk(char *, enum vm_disk_fmt);
> +enum vm_disk_fmt parse_format(const char *);
>
> static struct vmop_create_params vmc;
> static struct vmd_switch *vsw;
> @@ -104,6 +104,7 @@ extern const char *vmd_descsw[];
> typedef struct {
> union {
> uint8_t lladdr[ETHER_ADDR_LEN];
> + enum vm_disk_fmt disk_format;
> int64_t number;
> char *string;
> struct {
> @@ -128,7 +129,7 @@ typedef struct {
> %type <v.lladdr> lladdr
> %type <v.number> bootdevice
> %type <v.number> disable
> -%type <v.number> image_format
> +%type <v.disk_format> image_format
> %type <v.number> local
> %type <v.number> locked
> %type <v.number> updown
> @@ -642,10 +643,10 @@ agentxopts : /* none */
> ;
>
> image_format : /* none */ {
> - $$ = 0;
> + $$ = VMDF_AUTO;
> }
> | FORMAT string {
> - if (($$ = parse_format($2)) == 0) {
> + if (($$ = parse_format($2)) == VMDF_INVALID) {
> yyerror("unrecognized disk format %s", $2);
> free($2);
> YYERROR;
> @@ -1355,11 +1356,9 @@ parse_size(char *word, int64_t val)
> }
>
> int
> -parse_disk(char *word, int type)
> +parse_disk(char *word, enum vm_disk_fmt type)
> {
> - char buf[BUFSIZ], path[PATH_MAX];
> - int fd;
> - ssize_t len;
> + char path[PATH_MAX];
>
> if (vmc.vmc_ndisks >= VM_MAX_DISKS_PER_VM) {
> log_warnx("too many disks");
> @@ -1371,23 +1370,6 @@ parse_disk(char *word, int type)
> return (-1);
> }
>
> - if (!type) {
> - /* Use raw as the default format */
> - type = VMDF_RAW;
> -
> - /* Try to derive the format from the file signature */
> - if ((fd = open(path, O_RDONLY)) != -1) {
> - len = read(fd, buf, sizeof(buf));
> - close(fd);
> - if (len >= (ssize_t)strlen(VM_MAGIC_QCOW) &&
> - strncmp(buf, VM_MAGIC_QCOW,
> - strlen(VM_MAGIC_QCOW)) == 0) {
> - /* The qcow version will be checked later */
> - type = VMDF_QCOW2;
> - }
> - }
> - }
> -
> if (strlcpy(vmc.vmc_disks[vmc.vmc_ndisks], path,
> sizeof(vmc.vmc_disks[vmc.vmc_ndisks])) >=
> sizeof(vmc.vmc_disks[vmc.vmc_ndisks])) {
> @@ -1401,14 +1383,14 @@ parse_disk(char *word, int type)
> return (0);
> }
>
> -unsigned int
> +enum vm_disk_fmt
> parse_format(const char *word)
> {
> if (strcasecmp(word, "raw") == 0)
> return (VMDF_RAW);
> else if (strcasecmp(word, "qcow2") == 0)
> return (VMDF_QCOW2);
> - return (0);
> + return (VMDF_INVALID);
> }
>
> /*
> blob - d7bee8e377aa22a7225b6573182b361ca0f018ed
> blob + fed57a121fbb8251e811e980cbc40c9725c4f2bf
> --- usr.sbin/vmd/vioblk.c
> +++ usr.sbin/vmd/vioblk.c
> @@ -36,7 +36,7 @@
> extern struct vmd_vm *current_vm;
> struct iovec io_v[VIRTIO_QUEUE_SIZE_MAX];
>
> -static const char *disk_type(int);
> +static const char *disk_type(enum vm_disk_fmt);
> static uint32_t vioblk_read(struct virtio_dev *, struct viodev_msg *, int *);
> static int vioblk_write(struct virtio_dev *, struct viodev_msg *);
> static uint32_t vioblk_dev_read(struct virtio_dev *, struct viodev_msg *);
> @@ -49,11 +49,13 @@ static void dev_dispatch_vm(int, short, void *);
> static void handle_sync_io(int, short, void *);
>
> static const char *
> -disk_type(int type)
> +disk_type(enum vm_disk_fmt type)
> {
> switch (type) {
> + case VMDF_AUTO: return "auto";
> case VMDF_RAW: return "raw";
> case VMDF_QCOW2: return "qcow2";
> + case VMDF_INVALID: return "invalid";
> }
> return "unknown";
> }
> @@ -67,7 +69,8 @@ vioblk_main(int fd, int fd_vmm)
> struct vmd_vm vm;
> ssize_t sz;
> off_t szp = 0;
> - int i, ret, type;
> + enum vm_disk_fmt type;
> + int i, ret;
>
> /*
> * stdio - needed for read/write to disk fds and channels to the vm.
> blob - 662b74264004705438bd1ac464c6b5559c0544ec
> blob + 9f3db23e18bcc2563723b013b55cd723c77e154b
> --- usr.sbin/vmd/virtio.c
> +++ usr.sbin/vmd/virtio.c
> @@ -941,17 +941,18 @@ vmmci_io(int dir, uint16_t reg, uint32_t *data, uint8_
> return (0);
> }
>
> -int
> -virtio_get_base(int fd, char *path, size_t npath, int type, const char *dpath)
> +enum vm_disk_fmt
> +virtio_get_disktype(int fd)
> {
> - switch (type) {
> - case VMDF_RAW:
> - return 0;
> - case VMDF_QCOW2:
> - return virtio_qcow2_get_base(fd, path, npath, dpath);
> - }
> - log_warnx("%s: invalid disk format", __func__);
> - return -1;
> + char buf[sizeof(VM_MAGIC_QCOW) - 1];
> + ssize_t len;
> +
> + len = pread(fd, buf, sizeof(buf), 0);
> + if (len >= (ssize_t)sizeof(buf) &&
> + memcmp(buf, VM_MAGIC_QCOW, sizeof(buf)) == 0)
> + return (VMDF_QCOW2);
> +
> + return (VMDF_RAW);
> }
>
> static void
> blob - bfc32cec095e3a7ed218f4527058df284a467157
> blob + 2ebc0b7bd8d460fa594afe4d829345ad3d81cb5f
> --- usr.sbin/vmd/virtio.h
> +++ usr.sbin/vmd/virtio.h
> @@ -396,6 +396,7 @@ uint32_t virtio_io_cfg(struct virtio_dev *, int, uint8
> void virtio_update_qs(struct virtio_dev *);
> void virtio_update_qa(struct virtio_dev *);
>
> +enum vm_disk_fmt virtio_get_disktype(int);
> ssize_t virtio_qcow2_get_base(int, char *, size_t, const char *);
> int virtio_qcow2_create(const char *, const char *, uint64_t);
> int virtio_qcow2_init(struct virtio_backing *, off_t *, int*, size_t);
> blob - d6bcfd05da4b5d22130c39573c0078d60a2f528e
> blob + 3b3f5fb608dca3e8bcb44b201b2f8b7ef390d5e2
> --- usr.sbin/vmd/vmd.h
> +++ usr.sbin/vmd/vmd.h
> @@ -207,6 +207,13 @@ struct vmop_owner {
> int64_t gid;
> };
>
> +enum vm_disk_fmt {
> + VMDF_INVALID = 0,
> + VMDF_AUTO,
> + VMDF_RAW,
> + VMDF_QCOW2,
> +};
> +
> struct vmop_create_params {
> /* vm identifying information */
> uint32_t vmc_id;
> @@ -248,10 +255,8 @@ struct vmop_create_params {
> /* Emulated disk and cdrom drives */
> size_t vmc_ndisks;
> char vmc_disks[VM_MAX_DISKS_PER_VM][PATH_MAX];
> - unsigned int vmc_disktypes[VM_MAX_DISKS_PER_VM];
> + enum vm_disk_fmt vmc_disktypes[VM_MAX_DISKS_PER_VM];
> unsigned int vmc_diskbases[VM_MAX_DISKS_PER_VM];
> -#define VMDF_RAW 0x01
> -#define VMDF_QCOW2 0x02
> char vmc_cdrom[PATH_MAX];
>
> /* Emulated network devices */
> @@ -581,9 +586,6 @@ int cmdline_symset(char *);
> int parse_prefix4(const char *, struct local_prefix *, const char **);
> int parse_prefix6(const char *, struct local_prefix *, const char **);
>
> -/* virtio.c */
> -int virtio_get_base(int, char *, size_t, int, const char *);
> -
> /* vionet.c */
> __dead void vionet_main(int, int);
>
remove TOCTOU in vmd config and defer disk detection