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);
