[libvirt] [PATCH 1/2] nvram: Fix permissions

Michal Privoznik mprivozn at redhat.com
Thu Sep 11 14:25:06 UTC 2014


On 11.09.2014 16:07, Laszlo Ersek wrote:
> On 09/11/14 14:09, Michal Privoznik wrote:
>> I've noticed two problem with the automatically created NVRAM varstore
>> file. The first, even though I run qemu as root:root for some reason I
>> get Permission denied when trying to open the _VARS.fd file. The
>> problem is, the upper directory misses execute permissions, which in
>> combination with us dropping some capabilities result in EPERM.
>>
>> The next thing is, that if I switch SELinux to enforcing mode, I get
>> another EPERM because the vars file is not labeled correctly. It is
>> passed to qemu as disk and hence should be labelled as disk. QEMU may
>> write to it eventually, so this is different to kernel or initrd.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   libvirt.spec.in                 | 2 +-
>>   src/security/security_selinux.c | 5 ++++-
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index a6a58cf..ecf160b 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -1938,7 +1938,7 @@ exit 0
>>   %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/
>>   %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/
>>   %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/target/
>> -%dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/nvram/
>> +%dir %attr(0711, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/nvram/
>>   %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/cache/libvirt/qemu/
>>   %{_datadir}/augeas/lenses/libvirtd_qemu.aug
>>   %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
>> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
>> index bf67fb5..3db2b27 100644
>> --- a/src/security/security_selinux.c
>> +++ b/src/security/security_selinux.c
>> @@ -2300,8 +2300,11 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
>>                                        mgr) < 0)
>>           return -1;
>>
>> +    /* This is different than kernel or initrd. The nvram store
>> +     * is really a disk, qemu can read and write to it. */
>>       if (def->os.loader && def->os.loader->nvram &&
>> -        virSecuritySELinuxSetFilecon(def->os.loader->nvram, data->content_context) < 0)
>> +        secdef && secdef->imagelabel &&
>> +        virSecuritySELinuxSetFilecon(def->os.loader->nvram, secdef->imagelabel) < 0)
>>           return -1;
>>
>>       if (def->os.kernel &&
>>
>
> Good detective work!
>
> Regarding the g+x,o+x change on
> %{_localstatedir}/lib/libvirt/qemu/nvram. This change theoretically
> allows a qemu instance to "probe" for the presence of "foreign" varstore
> files (it won't be able to open any, but eg. error codes for open()
> would differ between ENOENT vs. EACCES, and stat() would fail vs.
> succeed). However I think we can live with this, and anyway, it's simply
> impossible to open a file in directory D if directory D doesn't provide
> the user with search permission. So that looks like a must.

Indeed. Moreover, I was first surprised that even if was running qemu 
under root:root I got EACCES. Problem was, libvirt drops nearly all caps 
(even CAP_SYS_ADMIN) after fork and prior to executing qemu binary.

>
> Regarding the seclabel  / context, I agree that it should have a label
> consistent with other disk image files; for qemu it's just a -drive
> after all. The hunk in question looks consistent with the rest of
> "src/security/security_selinux.c".
>
> Acked-by: Laszlo Ersek <lersek at redhat.com>
>

Thanks, applied.

Michal




More information about the libvir-list mailing list