On Sat, Mar 13, 2021 at 08:40:03AM +0000, James Cook wrote:
> Currently, "vmctl create -i source.qcow2 dest.qcow" fails when
> source.qcow2 has a base image, because virtio_qcow2_get_base calls
> realpath which doesn't interact well with unveil.
> 
> The below patch fixes it by delaying the first call to unveil. Caveat:
> I have never worked with unveil or any of this code before, but the
> change seems simple enough.
> 
> I also documented a quirk of the "vmctl create" command: when the -b
> option is specified, the "disk" argument had better be in the current
> directory. (Otherwise for example vmd won't be able to open the image.)
> 
> The patch includes a regression test that demonstrates the existing
> problem.

Updated patch: I made a correction to my comment in vmctl.c.

- James


diff --git a/regress/usr.sbin/Makefile b/regress/usr.sbin/Makefile
index 60e2178d3c7..146f9c9f322 100644
--- a/regress/usr.sbin/Makefile
+++ b/regress/usr.sbin/Makefile
@@ -15,6 +15,7 @@ SUBDIR += rpki-client
 SUBDIR += snmpd
 SUBDIR += switchd
 SUBDIR += syslogd
+SUBDIR += vmctl
 
 .if ${MACHINE} == "amd64" || ${MACHINE} == "i386"
 SUBDIR += vmd
diff --git a/regress/usr.sbin/vmctl/Makefile b/regress/usr.sbin/vmctl/Makefile
new file mode 100644
index 00000000000..108ff0a43c4
--- /dev/null
+++ b/regress/usr.sbin/vmctl/Makefile
@@ -0,0 +1,18 @@
+# $OpenBSD$
+
+REGRESS_TARGETS=convert_with_absolute_base_path convert_with_local_base_paths
+
+convert_with_absolute_base_path:
+       rm *.qcow2
+       vmctl create -s 1m base.qcow2
+       vmctl create -b ${PWD}/base.qcow2 source.qcow2
+       vmctl create -i source.qcow2 dest.qcow2
+
+convert_with_local_base_paths:
+       rm *.qcow2
+       vmctl create -s 1m base0.qcow2
+       vmctl create -b base0.qcow2 base1.qcow2
+       vmctl create -b base1.qcow2 source.qcow2
+       vmctl create -i source.qcow2 dest.qcow2
+
+.include <bsd.regress.mk>
diff --git a/usr.sbin/vmctl/main.c b/usr.sbin/vmctl/main.c
index 249eaa3ded1..2d713d67274 100644
--- a/usr.sbin/vmctl/main.c
+++ b/usr.sbin/vmctl/main.c
@@ -575,8 +575,7 @@ ctl_create(struct parse_result *res, int argc, char *argv[])
                        break;
                case 'i':
                        input = optarg;
-                       if (unveil(input, "r") == -1)
-                               err(1, "unveil");
+                       /* Don't call unveil before ctl_convert. */
                        break;
                case 's':
                        if (parse_size(res, optarg) != 0)
@@ -597,8 +596,6 @@ ctl_create(struct parse_result *res, int argc, char *argv[])
 
        if (pledge("stdio rpath wpath cpath unveil", NULL) == -1)
                err(1, "pledge");
-       if (unveil(disk, "rwc") == -1)
-               err(1, "unveil");
 
        if (input) {
                if (base && input)
@@ -606,6 +603,8 @@ ctl_create(struct parse_result *res, int argc, char *argv[])
                return ctl_convert(input, disk, type, res->size);
        }
 
+       if (unveil(disk, "rwc") == -1)
+               err(1, "unveil");
        if (unveil(NULL, NULL))
                err(1, "unveil");
 
@@ -656,7 +655,10 @@ ctl_convert(const char *srcfile, const char *dstfile, int 
dsttype, size_t dstsiz
                goto done;
        }
 
-       /* We can only lock unveil after opening the disk and all base images */
+       /* We can't call unveil before open_imagefile, since it might call
+          virtio_qcow2_get_base. */
+       if (unveil(dst.disk, "rwc") == -1)
+               err(1, "unveil");
        if (unveil(NULL, NULL))
                err(1, "unveil");
 
diff --git a/usr.sbin/vmctl/vmctl.8 b/usr.sbin/vmctl/vmctl.8
index d36170e3e24..42cde19544b 100644
--- a/usr.sbin/vmctl/vmctl.8
+++ b/usr.sbin/vmctl/vmctl.8
@@ -77,7 +77,7 @@ connect to the console of the VM with the specified
 Create a VM disk image file with the specified
 .Ar disk
 path.
-.Bl -tag -width "-i input"
+.Bl -tag -width "-i disk"
 .It Fl b Ar base
 For
 .Sq qcow2 ,
@@ -85,7 +85,9 @@ a
 .Ar base
 image may be specified.
 The base image is not modified and the derived image contains only the
-changes written by the VM.
+changes written by the VM. When using this option,
+.Ar disk
+should be in the current directory.
 .It Fl i Ar disk
 Copy and convert the input
 .Ar disk
diff --git a/usr.sbin/vmctl/vmctl.c b/usr.sbin/vmctl/vmctl.c
index d8edbc062eb..6b21da08a69 100644
--- a/usr.sbin/vmctl/vmctl.c
+++ b/usr.sbin/vmctl/vmctl.c
@@ -847,6 +847,9 @@ vm_console(struct vmop_info_result *list, size_t ct)
  *
  * Open an imagefile with the specified type, path and size.
  *
+ * Do not call unveil before this function, in case virtio_qcow2_get_base
+ * needs to resolve base images.
+ *
  * Parameters:
  *  type        : format of the image file
  *  imgfile_path: path to the image file to create
@@ -885,14 +888,6 @@ open_imagefile(int type, const char *imgfile_path, int 
flags,
                        } else if (ret == 0)
                                break;
 
-                       /*
-                        * This might be called after unveil is already
-                        * locked but it is save to ignore the EPERM error
-                        * here as the subsequent open would fail as well.
-                        */
-                       if ((ret = unveil(path, "r")) != 0 &&
-                           (ret != EPERM))
-                               err(1, "unveil");
                        if ((basefd[i + 1] = open(path, O_RDONLY)) == -1) {
                                log_warn("%s: failed to open base %s",
                                    __func__, path);

Reply via email to