[...]
>> OK - so a too long winded way to say - I don't think the following patch
>> matters as anything (including libvirt) could have changed the file's
>> ownership and/or protections, but not updated the volume XML. The
>> refresh updates the volume XML to match the file's protection and
>> furthermore only matters in the current virFileRemove if the change is
>> back to root:root and only because we compare vs. gete{uid|gid}().
>>
>
> I don't follow this conclusion really... refreshVol syncs the XML/volume cache
> with the current uid/gid/mode on disk, which is then passed down to
> virFileRemove. If the cached uid is 'qemu' but the on disk uid is 'cole', the
> internal cache is synced, virFileRemove will setuid(cole) and successfully
> remove the disk image. Without this change, libvirt would setuid(qemu) and
> fail to remove the disk.
>
> So I don't see how this patch is unneeded, or only works for root:root
>
I was considering the checks:
+ if (geteuid() != 0)
+ return false;
We're running as root...
+
+ /* uid/gid weren'd specified */
+ if ((uid == (uid_t) -1) && (gid == (gid_t) -1))
+ return false;
We've provided qemu:qemu...
+
+ /* already running as proper uid/gid */
+ if (uid == geteuid() && gid == getegid())
+ return false;
+
At this point geteuid() would return 0 (root)
Maybe I'm wrong... I suppose it cannot hurt, but without patch 3 we'd
go through the setuid path - I think. I could also be missing something
really obvious.
John
> - Cole
>
>> John
>>
>>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>>> index 1d96618..ded54c9 100644
>>> --- a/src/storage/storage_driver.c
>>> +++ b/src/storage/storage_driver.c
>>> @@ -1801,6 +1801,16 @@ storageVolDelete(virStorageVolPtr obj,
>>> goto cleanup;
>>> }
>>>
>>> + /* Need to ensure we are using up to date uid/gid for deletion */
>>> + if (backend->refreshVol &&
>>> + backend->refreshVol(obj->conn, pool, vol) < 0) {
>>> + /* The file may have been removed behind libvirt's back. Don't
>>> + error here, let the deletion fail or succeed instead */
>>> + VIR_INFO("Failed to refresh volume before deletion: %s",
>>> + virGetLastErrorMessage());
>>> + virResetLastError();
>>> + }
>>> +
>>> if (storageVolDeleteInternal(obj, backend, pool, vol, flags, true) < 0)
>>> goto cleanup;
>>>
>>>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list