This problem is reproducible only with secret driver. When
starting a domain via virt-qemu-run and both secret and
(nonexistent) root directory specified this is what happens:

1) virt-qemu-run opens "secret:///embed?root=$rootdir"
   connection, which results in the secret driver initialization
   (done in secretStateInitialize()). During this process, the
   driver creates it's own configDir (derived from $rootdir)
   including those parents which don't exists yet. This is all
   done with the mode S_IRWXU and thus results in the $rootdir
   being created with very restrictive mode (specifically, +x is
   missing for group and others).

2) now, virt-qemu-run-opens "qemu:///embed?root=$rootdir" and
   calls virDomainCreateXML(). This results in the master-key.aes
   being written somewhere under the $rootdir and telling qemu
   where to find it.

But because the secret driver created $rootdir with too
restrictive mode, qemu can't access the file (even though it
knows the full path) and fails to start.

It looks like the best solution is to pre-create the root
directory before opening any connection (letting any driver
initialize itself) and set its mode to something less
restrictive.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1859873
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_shim.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_shim.c b/src/qemu/qemu_shim.c
index 21a24abade..a08bdcac6a 100644
--- a/src/qemu/qemu_shim.c
+++ b/src/qemu/qemu_shim.c
@@ -213,11 +213,16 @@ int main(int argc, char **argv)
         }
         tmproot = true;
 
-        if (chmod(root, 0755) < 0) {
-            g_printerr("%s: cannot chown temporary dir: %s\n",
-                       argv[0], g_strerror(errno));
-            goto cleanup;
-        }
+    } else if (g_mkdir_with_parents(root, 0755) < 0) {
+        g_printerr("%s: cannot create dir: %s\n",
+                   argv[0], g_strerror(errno));
+        goto cleanup;
+    }
+
+    if (chmod(root, 0755) < 0) {
+        g_printerr("%s: cannot chmod temporary dir: %s\n",
+                   argv[0], g_strerror(errno));
+        goto cleanup;
     }
 
     escaped = g_uri_escape_string(root, NULL, true);
-- 
2.26.2

On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
> This problem is reproducible only with secret driver. When
> starting a domain via virt-qemu-run and both secret and
> (nonexistent) root directory specified this is what happens:
> 
> 1) virt-qemu-run opens "secret:///embed?root=$rootdir"
>    connection, which results in the secret driver initialization
>    (done in secretStateInitialize()). During this process, the
>    driver creates it's own configDir (derived from $rootdir)

s/it's own/its own/

>    including those parents which don't exists yet. This is all
>    done with the mode S_IRWXU and thus results in the $rootdir
>    being created with very restrictive mode (specifically, +x is
>    missing for group and others).
> 
> 2) now, virt-qemu-run-opens "qemu:///embed?root=$rootdir" and

s/run-opens/run opens/

> +++ b/src/qemu/qemu_shim.c
> @@ -213,11 +213,16 @@ int main(int argc, char **argv)
>          }
>          tmproot = true;
>  
> -        if (chmod(root, 0755) < 0) {
> -            g_printerr("%s: cannot chown temporary dir: %s\n",
> -                       argv[0], g_strerror(errno));
> -            goto cleanup;
> -        }
> +    } else if (g_mkdir_with_parents(root, 0755) < 0) {
> +        g_printerr("%s: cannot create dir: %s\n",
> +                   argv[0], g_strerror(errno));
> +        goto cleanup;
> +    }
> +
> +    if (chmod(root, 0755) < 0) {
> +        g_printerr("%s: cannot chmod temporary dir: %s\n",
> +                   argv[0], g_strerror(errno));
> +        goto cleanup;
>      }

Wouldn't it make sense to leave the chmod() bit where it was?
g_mkdir_with_parents() already accepts the mode as a parameter, so
calling chmod() again seems unnecessary.

With that changed and the commit message fixed,

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

On 3/12/21 11:51 AM, Andrea Bolognani wrote:
> On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
>> This problem is reproducible only with secret driver. When
>> starting a domain via virt-qemu-run and both secret and
>> (nonexistent) root directory specified this is what happens:
>>
>> 1) virt-qemu-run opens "secret:///embed?root=$rootdir"
>>     connection, which results in the secret driver initialization
>>     (done in secretStateInitialize()). During this process, the
>>     driver creates it's own configDir (derived from $rootdir)
> 
> s/it's own/its own/
> 
>>     including those parents which don't exists yet. This is all
>>     done with the mode S_IRWXU and thus results in the $rootdir
>>     being created with very restrictive mode (specifically, +x is
>>     missing for group and others).
>>
>> 2) now, virt-qemu-run-opens "qemu:///embed?root=$rootdir" and
> 
> s/run-opens/run opens/
> 
>> +++ b/src/qemu/qemu_shim.c
>> @@ -213,11 +213,16 @@ int main(int argc, char **argv)
>>           }
>>           tmproot = true;
>>   
>> -        if (chmod(root, 0755) < 0) {
>> -            g_printerr("%s: cannot chown temporary dir: %s\n",
>> -                       argv[0], g_strerror(errno));
>> -            goto cleanup;
>> -        }
>> +    } else if (g_mkdir_with_parents(root, 0755) < 0) {
>> +        g_printerr("%s: cannot create dir: %s\n",
>> +                   argv[0], g_strerror(errno));
>> +        goto cleanup;
>> +    }
>> +
>> +    if (chmod(root, 0755) < 0) {
>> +        g_printerr("%s: cannot chmod temporary dir: %s\n",
>> +                   argv[0], g_strerror(errno));
>> +        goto cleanup;
>>       }
> 
> Wouldn't it make sense to leave the chmod() bit where it was?
> g_mkdir_with_parents() already accepts the mode as a parameter, so
> calling chmod() again seems unnecessary.

Well, if the dir exists but doesn't have right perms then 
g_mkdir_with_parents() does nothing and we need that explicit chmod().

Michal

On Fri, 2021-03-12 at 15:38 +0100, Michal Privoznik wrote:
> On 3/12/21 11:51 AM, Andrea Bolognani wrote:
> > On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
> > > @@ -213,11 +213,16 @@ int main(int argc, char **argv)
> > >           }
> > >           tmproot = true;
> > >   
> > > -        if (chmod(root, 0755) < 0) {
> > > -            g_printerr("%s: cannot chown temporary dir: %s\n",
> > > -                       argv[0], g_strerror(errno));
> > > -            goto cleanup;
> > > -        }
> > > +    } else if (g_mkdir_with_parents(root, 0755) < 0) {
> > > +        g_printerr("%s: cannot create dir: %s\n",
> > > +                   argv[0], g_strerror(errno));
> > > +        goto cleanup;
> > > +    }
> > > +
> > > +    if (chmod(root, 0755) < 0) {
> > > +        g_printerr("%s: cannot chmod temporary dir: %s\n",
> > > +                   argv[0], g_strerror(errno));
> > > +        goto cleanup;
> > >       }
> > 
> > Wouldn't it make sense to leave the chmod() bit where it was?
> > g_mkdir_with_parents() already accepts the mode as a parameter, so
> > calling chmod() again seems unnecessary.
> 
> Well, if the dir exists but doesn't have right perms then 
> g_mkdir_with_parents() does nothing and we need that explicit chmod().

Fair point, I hadn't considered that :)

-- 
Andrea Bolognani / Red Hat / Virtualization