The NVRAM label is set in qemuSecuritySetAllLabel(). There's no
need to set its label upfront. In fact, setting it twice creates
an imbalance because it's unset only once which mangles seclabel
remembering. However, plain removal of the
qemuSecurityDomainSetPathLabel() undoes the fix for the original
bug (when dynamic ownership is off then the NVRAM is not created
with cfg->user and cfg->group but as root:root). Therefore, we
have to switch to virFileOpenAs() and pass cfg->user and
cfg->group and VIR_FILE_OPEN_FORCE_OWNER flag. There's no need to
pass VIR_FILE_OPEN_FORCE_MODE because the file will be created
with the proper mode.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1969347
Fixes: bcdaa91a27b5b2d103535270a6a287efe6cd8bfb
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_process.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c37687f249..2b03b0ab98 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4538,16 +4538,19 @@ qemuPrepareNVRAM(virQEMUDriver *driver,
         goto cleanup;
     }
 
-    if ((dstFD = qemuDomainOpenFile(driver, vm, loader->nvram,
-                                    O_WRONLY | O_CREAT | O_EXCL,
-                                    NULL)) < 0)
+    if ((dstFD = virFileOpenAs(loader->nvram,
+                               O_WRONLY | O_CREAT | O_EXCL,
+                               S_IRUSR | S_IWUSR,
+                               cfg->user, cfg->group,
+                               VIR_FILE_OPEN_FORCE_OWNER)) < 0) {
+        virReportSystemError(-dstFD,
+                             _("Failed to create file '%s'"),
+                             loader->nvram);
         goto cleanup;
+    }
 
     created = true;
 
-    if (qemuSecurityDomainSetPathLabel(driver, vm, loader->nvram, false) < 0)
-        goto cleanup;
-
     do {
         char buf[1024];
 
-- 
2.31.1

On 6/9/21 12:19 PM, Michal Privoznik wrote:
> The NVRAM label is set in qemuSecuritySetAllLabel(). There's no
> need to set its label upfront. In fact, setting it twice creates
> an imbalance because it's unset only once which mangles seclabel
> remembering. However, plain removal of the
> qemuSecurityDomainSetPathLabel() undoes the fix for the original
> bug (when dynamic ownership is off then the NVRAM is not created
> with cfg->user and cfg->group but as root:root). Therefore, we
> have to switch to virFileOpenAs() and pass cfg->user and
> cfg->group and VIR_FILE_OPEN_FORCE_OWNER flag. There's no need to
> pass VIR_FILE_OPEN_FORCE_MODE because the file will be created
> with the proper mode.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1969347
> Fixes: bcdaa91a27b5b2d103535270a6a287efe6cd8bfb
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


>   src/qemu/qemu_process.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c37687f249..2b03b0ab98 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4538,16 +4538,19 @@ qemuPrepareNVRAM(virQEMUDriver *driver,
>           goto cleanup;
>       }
>   
> -    if ((dstFD = qemuDomainOpenFile(driver, vm, loader->nvram,
> -                                    O_WRONLY | O_CREAT | O_EXCL,
> -                                    NULL)) < 0)
> +    if ((dstFD = virFileOpenAs(loader->nvram,
> +                               O_WRONLY | O_CREAT | O_EXCL,
> +                               S_IRUSR | S_IWUSR,
> +                               cfg->user, cfg->group,
> +                               VIR_FILE_OPEN_FORCE_OWNER)) < 0) {
> +        virReportSystemError(-dstFD,
> +                             _("Failed to create file '%s'"),
> +                             loader->nvram);
>           goto cleanup;
> +    }
>   
>       created = true;
>   
> -    if (qemuSecurityDomainSetPathLabel(driver, vm, loader->nvram, false) < 0)
> -        goto cleanup;
> -
>       do {
>           char buf[1024];
>   
>