[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3] storage: refresh volume before deletion



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]