James Cook writes:
>> > Another thought --- is it necessary to call realpath at all?
>>
>> Yes, vmd(8) will break if you remove it.
>>
>> The caveat here is vmd(8) uses realpath and works with realpath, so the
>> issue is really vmctl(8). I'm guessing in your test run you ran vmd from
>> the directory with the disk image.
>
> No, I ran it with rcctl stop vmd / rcctl start vmd.
>
> I set up my vm.conf so that the path to the disk image includes a
> symlink. It still worked.
>
> vm "leth" {
> owner falsifian
> disable
> memory 4G
> disk "/home/falsifian/vm/leth/disk.qcow2"
> local interface
> }
>
> falsifian moth ~ $ ls -l /home/falsifian/vm/leth
> lrwxr-xr-x 1 falsifian falsifian 9 Mar 14 04:37 /home/falsifian/vm/leth ->
> leth.orig
>
> And just to be sure I'm really using my changes to vmd, I tried adding
> an "&& 0" to the "if (path[0] != '/') {" condition, and sure enough,
> vmd fails to start my instance with
> vmctl: could not open disk image(s)
>
> I also tried adding a second base image: disk.qcow2 is based on
> disk_base.qcow2 which is based on disk_snapshot_2021-02-07.qcow2. vmd
> still works, with my no-realpath patch.
>
> Looking at the code in usr.sbin/vmd, I can't figure out why calling
> realpath could matter, but I might have missed something. I think
> virtio_qcow2_get_base is only called by virtio_get_base, which is only
> called by config_setvm, and I think the output is only passed into an
> "open" system call. From then on, the function only keeps the file
> descriptor from open. But there's a lot going on in config_setvm and I
> haven't read every line.
>
I've gone through the history of the qcow2 support [1] from when Ori
Bernstein and Reyk worked on importing it. I can't find any explicit
reason for using realpath(3).
To your point, it seems superflous. While it will validate the target
can be read and error if it can't, the open(2) call should fail.
I've been testing your proposed change and I haven't been able to break
it. :-)
The only thing I can think of is dirname(3) can return NULL if the given
path ends up resolving to something longer than PATH_MAX, but since
there's the guard of strlcpy(3) I don't think it can be triggered.
Maybe start a new tech@ thread with your diff for vioqcow2.c to increase
chance someone's paying attention besides us at this point. Given it's a
single function diff it has a better chance getting reviewed and
imported vs. our initial efforts.
[1]: https://marc.info/?l=openbsd-tech&m=153833570931405&w=2
>> The following diff lets vmctl(8) use a slightly different approach
>> (similar to what you were getting at hitting vmd/vioqcow2.c with a
>> hammer in your other email).
>>
>> - If the base image path looks like an absolute path, i.e. starts with
>> '/', just try using it.
>> - Otherwise, try the simplistic use of making it relative to the input
>> image file using a call to dirname(3).
>>
>> This continues to let vmctl(8) use unveil(2) without having to taint the
>> vmd(8) codebase with conditional unveil logic.
>
> Your patch works for me, except the below hunk was rejected for some
> reason.
>
> To test: I briefly tried vmd, and I tried out vmctl create commands
> with a symlink involved.
>
>> @@ -729,7 +734,7 @@ virtio_qcow2_create(const char *imgfile_
>> if (ftruncate(fd, (off_t)initsz + clustersz) == -1)
>> goto error;
>>
>> - /*
>> + /*
>> * Paranoia: if our disk image takes more than one cluster
>> * to refcount the initial image, fail.
>> */
--
-Dave Voutila
