On 03/09/2016 08:20 PM, John Ferlan wrote:
[...]
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.
Right, we would go through the setuid path... but it would _succeed_, because we would be setuid($correct-file-uid) and not setuid($out-of-date-file-uid) This patch is infact still needed; consider the case when the cached uid is out of date on NFS: we will do setuid($out-of-date-file-uid) and fail in the same way as the original report. - Cole