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@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. 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@redhat.com>

On 09/11/2014 04:25 PM, Michal Privoznik wrote:

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@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.

I believe CAP_DAC_OVERRIDE is the capability. Jan

On 12.09.2014 00:20, Laszlo Ersek wrote:

On 09/11/14 16:25, Michal Privoznik wrote:

...

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@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@redhat.com>

Thanks, applied.

... Unfortunately the patch is insufficient. Commit 742b08e3 added directory

%{_localstatedir}/lib/libvirt/qemu/nvram/

to two sub-packages:

%files daemon %files daemon-driver-qemu

I assume that was a correct thing to do (it is consistent with the rest of the directories being listed for both subpackages).

However, this recent patch (commit 37d8c75f) changes the 0750 permission bits to 0711 only in the "daemon" subpackage, and the directory in the daemon-driver-qemu subpackage remains with 0750. In my testing, the daemon package's modification doesn't take effect at all. In fact the daemon RPM doesn't even seem to contain the nvram directory.

Ah, thanks for catching that. I'm pushing the obvious trivial fix. Thanks! Michal

some comments below On 09/11/14 14:09, Michal Privoznik wrote:

When a domain is undefined, there are options to remove it's managed save state or snapshots. However, there's another file that libvirt creates per domain: the NVRAM variable store file. Make sure that the file is not left behind if the domain is undefined.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt.h.in | 2 ++ src/qemu/qemu_driver.c | 19 ++++++++++++++++++- tools/virsh-domain.c | 15 ++++++++++++--- tools/virsh.pod | 6 +++++- 4 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 94b942c..3c2a51a 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2257,6 +2257,8 @@ typedef enum { VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA = (1 << 1), /* If last use of domain, then also remove any snapshot metadata */ + VIR_DOMAIN_UNDEFINE_NVRAM = (1 << 2), /* Also remove any + nvram file */

/* Future undefine control flags should come here. */ } virDomainUndefineFlagsValues; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a8cda43..7f17fee 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6403,7 +6403,8 @@ qemuDomainUndefineFlags(virDomainPtr dom, virQEMUDriverConfigPtr cfg = NULL;

virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | - VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | + VIR_DOMAIN_UNDEFINE_NVRAM, -1);

if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -6452,6 +6453,22 @@ qemuDomainUndefineFlags(virDomainPtr dom, } }

Seems okay and consistent with the rest of the code thus far. OK.

+ if (!virDomainObjIsActive(vm) && + vm->def->os.loader && vm->def->os.loader->nvram) { + if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot delete inactive domain with nvram")); + goto cleanup; + } + + if (unlink(vm->def->os.loader->nvram) < 0) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; + } + } +

Again, this seems consistent with the rest of the code, eg. with the "cannot delete inactive domain with %d snapshots". I slightly wonder what happens when you succeed in the above, bute then the following fails:

if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0) goto cleanup;

I guess the full undefine operation will fail in this case. And if the user retries the undefine, then even the unlink() will fail (because the nvram file won't exist any longer). (1) After staring a bit longer at qemuDomainUndefineFlags(): can you please *further* restrict this code with a check for virFileExists()? Namely, the VIR_DOMAIN_UNDEFINE_MANAGED_SAVE flag does something very similar (including an unlink()), and that one is protected with a virFileExists(). I'll leave it up to you if you add the && to the top IF, or if you just short-circuit the unlink() with it (as in, (!virFileExists() && unlink() < 0)). I think it would be more useful to restrict the outer condition with it -- there's no point in complaining about the lack of VIR_DOMAIN_UNDEFINE_NVRAM of the nvram file doesn't exist anyway.

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6aa8631..db52271 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3248,6 +3248,10 @@ static const vshCmdOptDef opts_undefine[] = { .type = VSH_OT_BOOL, .help = N_("remove all domain snapshot metadata, if inactive") }, + {.name = "nvram", + .type = VSH_OT_BOOL, + .help = N_("remove nvram file, if inactive") + }, {.name = NULL} };

@@ -3270,6 +3274,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) bool snapshots_metadata = vshCommandOptBool(cmd, "snapshots-metadata"); bool wipe_storage = vshCommandOptBool(cmd, "wipe-storage"); bool remove_all_storage = vshCommandOptBool(cmd, "remove-all-storage"); + bool nvram = vshCommandOptBool(cmd, "nvram"); /* Positive if these items exist. */ int has_managed_save = 0; int has_snapshots_metadata = 0; @@ -3313,6 +3318,9 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA; snapshots_safe = true; } + if (nvram) { + flags |= VIR_DOMAIN_UNDEFINE_NVRAM; + }

if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false;

Seems reasonable. OK.

@@ -3504,10 +3512,11 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA flag was not present * until 0.9.5; skip to piecewise emulation if we couldn't prove * above that the new API is safe. */ - if (managed_save_safe && snapshots_safe) { + if ((managed_save_safe && snapshots_safe) || nvram) { rc = virDomainUndefineFlags(dom, flags); - if (rc == 0 || (last_error->code != VIR_ERR_NO_SUPPORT && - last_error->code != VIR_ERR_INVALID_ARG)) + if (rc == 0 || nvram || + (last_error->code != VIR_ERR_NO_SUPPORT && + last_error->code != VIR_ERR_INVALID_ARG)) goto out; vshResetLibvirtError(); }

If I understand correctly, if the user requests virsh to undefine the nvram too, then that's grounds enough to try the "new API" first. Okay. Then, pre-patch, the "new API" used to be considered *sufficient* if: - it succeeded, - or it failed, but the error code was different from both NO_SUPPORT and INVALID_ARG Post-patch, the "new API" is considered sufficient if: - it succeeded, - or failed, but the error code was different from both NO_SUPPORT and INVALID_ARG, - or it did fail with either NO_SUPPORT or INVALID_ARG, but the user requested to undefine the nvram. Namely, in this case there's no "old API" to fall back to. Good.

diff --git a/tools/virsh.pod b/tools/virsh.pod index 60ee515..ae396d2 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2083,7 +2083,7 @@ Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1.

=item B<undefine> I<domain> [I<--managed-save>] [I<--snapshots-metadata>] -[ {I<--storage> B<volumes> | I<--remove-all-storage>} I<--wipe-storage>] +[I<--nvram>} [ {I<--storage> B<volumes> | I<--remove-all-storage>} I<--wipe-storage>] ^ | (2) I think this is a small typo -- didn't you mean "|" (pipe symbol, meaning "alternatively")?

Undefine a domain. If the domain is running, this converts it to a transient domain, without stopping it. If the domain is inactive, @@ -2099,6 +2099,10 @@ domain. Without the flag, attempts to undefine an inactive domain with snapshot metadata will fail. If the domain is active, this flag is ignored.

+The I<--nvram> flag ensures no nvram (/domain/os/nvram/) file is +left behind. If the domain has a nvram file and the flag is

(3) "an nvram" (well it depends on how people read it out -- "an ENN VEE RAM" vs. "a non-volatile RAM").

+omitted the undefine will fail.

(4) please add a comma before "the undefine".

+ The I<--storage> flag takes a parameter B<volumes>, which is a comma separated list of volume target names or source paths of storage volumes to be removed along with the undefined domain. Volumes can be undefined and thus removed only

(5) Unrelated, please include a separate patch for it: yesterday both Alex Williamson and myself independently noticed that you updated the *code* according to Eric's request, ie. that the domain XML say readonly="yes", but the *example* in the docs still says readonly='on'. (The flowing text in the docs is correct.) Thank you! Laszlo