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

[Michal Prívozník]

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 at 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