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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list