[v1] qemuDomainSaveMemory: Don't enforce dynamicOwnership

[libvirt] [PATCH] qemuDomainSaveMemory: Don't enforce dynamicOwnership

Posted by Michal Privoznik 5 years, 10 months ago

https://bugzilla.redhat.com/show_bug.cgi?id=1589115

When doing a memory snapshot qemuOpenFile() is used. This means
that the file where memory is saved is firstly attempted to be
created under root:root (because that's what libvirtd is running
under) and if this fails the second attempt is done under
domain's uid:gid. This does not make much sense - qemu is given
opened FD so it does not need to access the file. Moreover, if
dynamicOwnership is set in qemu.conf and the file lives on a
squashed NFS this is deadly combination and very likely to fail.

The fix consists of using:

  qemuOpenFileAs(fallback_uid = cfg->user,
                 fallback_gid = cfg->group,
                 dynamicOwnership = false)

In other words, dynamicOwnership is turned off for memory
snapshot (chown() will still be attempted if the file does not
live on NFS) and instead of using domain DAC label, configured
user:group is set as fallback.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_driver.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 129bacdd34..6af7e6e171 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3223,6 +3223,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
                      unsigned int flags,
                      qemuDomainAsyncJob asyncJob)
 {
+    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     bool bypassSecurityDriver = false;
     bool needUnlink = false;
     int ret = -1;
@@ -3241,9 +3242,10 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
             goto cleanup;
         }
     }
-    fd = qemuOpenFile(driver, vm, path,
-                      O_WRONLY | O_TRUNC | O_CREAT | directFlag,
-                      &needUnlink, &bypassSecurityDriver);
+
+    fd = qemuOpenFileAs(cfg->user, cfg->group, false, path,
+                        O_WRONLY | O_TRUNC | O_CREAT | directFlag,
+                        &needUnlink, &bypassSecurityDriver);
     if (fd < 0)
         goto cleanup;
 
@@ -3283,6 +3285,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
  cleanup:
     VIR_FORCE_CLOSE(fd);
     virFileWrapperFdFree(wrapperFd);
+    virObjectUnref(cfg);
 
     if (ret < 0 && needUnlink)
         unlink(path);
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuDomainSaveMemory: Don't enforce dynamicOwnership

Posted by John Ferlan 5 years, 9 months ago

On 06/26/2018 09:57 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1589115
> 
> When doing a memory snapshot qemuOpenFile() is used. This means
> that the file where memory is saved is firstly attempted to be
> created under root:root (because that's what libvirtd is running
> under) and if this fails the second attempt is done under
> domain's uid:gid. This does not make much sense - qemu is given
> opened FD so it does not need to access the file. Moreover, if
> dynamicOwnership is set in qemu.conf and the file lives on a
> squashed NFS this is deadly combination and very likely to fail.
> 
> The fix consists of using:
> 
>   qemuOpenFileAs(fallback_uid = cfg->user,
>                  fallback_gid = cfg->group,
>                  dynamicOwnership = false)
> 
> In other words, dynamicOwnership is turned off for memory
> snapshot (chown() will still be attempted if the file does not
> live on NFS) and instead of using domain DAC label, configured
> user:group is set as fallback.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_driver.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 

I see from reading the bz in a way you hoped to provoke upstream
discussion on this, so...

It seems the primary motivation is to go with dynamicOwnership = false
is because that "fixes" the bz for the "corner case" where someone is
saving their snapshot to a root squashed NFS share. If the same scenario
was played out for core dump or managed save image, wouldn't the same
problem exist?  Although the latter would skip the O_CREAT check in
qemuOpenFlagsAs thus not setting VIR_FILE_OPEN_FORCE_OWNER, but could
perhaps theoretically fail as IIRC the root squash case only had one way
to resolve.

Since @path_shared == 1 (only way to get to the second attempt to
virFileOpenAs), then why doesn't this code try the VIR_FILE_OPEN_FORK?
It's been a while, but IIRC that's what allowed storageBackendCreateRaw
to be successful.

IIUC the proposed solution to clear dynamicOwnership is purely to avoid
setting VIR_FILE_OPEN_FORCE_OWNER since @path_shared == 1 in the O_CREAT
path.

On a secondary note, this is the only path that doesn't pass NULL for
bypassSecuritydriver... If you follow that bypass - all that really
happens is it can alter the returned pointer, but that value is never
checked in the code - so what is it's purpose?  Taking a quick look
through history, I wonder if 23087cfdb was the last real consumer.

John

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 129bacdd34..6af7e6e171 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3223,6 +3223,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>                       unsigned int flags,
>                       qemuDomainAsyncJob asyncJob)
>  {
> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      bool bypassSecurityDriver = false;
>      bool needUnlink = false;
>      int ret = -1;
> @@ -3241,9 +3242,10 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>              goto cleanup;
>          }
>      }
> -    fd = qemuOpenFile(driver, vm, path,
> -                      O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> -                      &needUnlink, &bypassSecurityDriver);
> +
> +    fd = qemuOpenFileAs(cfg->user, cfg->group, false, path,
> +                        O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> +                        &needUnlink, &bypassSecurityDriver);
>      if (fd < 0)
>          goto cleanup;
>  
> @@ -3283,6 +3285,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>   cleanup:
>      VIR_FORCE_CLOSE(fd);
>      virFileWrapperFdFree(wrapperFd);
> +    virObjectUnref(cfg);
>  
>      if (ret < 0 && needUnlink)
>          unlink(path);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuDomainSaveMemory: Don't enforce dynamicOwnership

Posted by Michal Prívozník 5 years, 9 months ago

On 07/02/2018 04:49 PM, John Ferlan wrote:
> 
> 
> On 06/26/2018 09:57 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1589115
>>
>> When doing a memory snapshot qemuOpenFile() is used. This means
>> that the file where memory is saved is firstly attempted to be
>> created under root:root (because that's what libvirtd is running
>> under) and if this fails the second attempt is done under
>> domain's uid:gid. This does not make much sense - qemu is given
>> opened FD so it does not need to access the file. Moreover, if
>> dynamicOwnership is set in qemu.conf and the file lives on a
>> squashed NFS this is deadly combination and very likely to fail.
>>
>> The fix consists of using:
>>
>>   qemuOpenFileAs(fallback_uid = cfg->user,
>>                  fallback_gid = cfg->group,
>>                  dynamicOwnership = false)
>>
>> In other words, dynamicOwnership is turned off for memory
>> snapshot (chown() will still be attempted if the file does not
>> live on NFS) and instead of using domain DAC label, configured
>> user:group is set as fallback.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_driver.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
> 
> I see from reading the bz in a way you hoped to provoke upstream
> discussion on this, so...
> 
> It seems the primary motivation is to go with dynamicOwnership = false
> is because that "fixes" the bz for the "corner case" where someone is
> saving their snapshot to a root squashed NFS share. 

Yes.

> If the same scenario
> was played out for core dump or managed save image, wouldn't the same
> problem exist?

Ah, good catch. So managed save uses qemuDomainSaveMemory() which I'm
fixing here. But coredump uses doCoreDump() which needs the same treatment.

>  Although the latter would skip the O_CREAT check in
> qemuOpenFlagsAs thus not setting VIR_FILE_OPEN_FORCE_OWNER, but could
> perhaps theoretically fail as IIRC the root squash case only had one way
> to resolve.
> 
> Since @path_shared == 1 (only way to get to the second attempt to
> virFileOpenAs), then why doesn't this code try the VIR_FILE_OPEN_FORK?
> It's been a while, but IIRC that's what allowed storageBackendCreateRaw
> to be successful.

It does, but with fallback UID:GID (which is currently whatever DAC
label domain has). But at the same time chown() to the fallback UID:GID
pair is enforced because dynamicOwnership is set. And it is actually the
chown() that fails.

Now, as absurd as it may sound, we don't need the memory image to be the
same UID:GID as the running domain. Qemu is given a FD to migrate to (at
which point perms are not checked anymore), so for instance even an
unprivileged qemu process can write into a file owned by root:root.

> 
> IIUC the proposed solution to clear dynamicOwnership is purely to avoid
> setting VIR_FILE_OPEN_FORCE_OWNER since @path_shared == 1 in the O_CREAT
> path.

Exactly.

> 
> On a secondary note, this is the only path that doesn't pass NULL for
> bypassSecuritydriver... If you follow that bypass - all that really
> happens is it can alter the returned pointer, but that value is never
> checked in the code - so what is it's purpose?  Taking a quick look
> through history, I wonder if 23087cfdb was the last real consumer.

Ah, good point. I'll post a patch to remove it.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list