9:23 a.m.
See path 2 for detail.
v2 -> v3:
- Change the directory to put the temp files of screenshot and
memorypeek from autoDumpPath to per-domain directory [Daniel]
v1 -> v2:
- Add commit message for path1 [Peter]
Peng Liang (2):
qemu: move temp file of screenshot and memorypeek to per-domain dir
qemu: don't change ownership of cache directory
src/qemu/qemu_driver.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
--
2.31.1
9:23 a.m.
New subject: [PATCH v3 1/2] qemu: move temp file of screenshot and memorypeek to per-domain dir
The temp files of screenshot and memory peek, which are created by QEMU,
are put in the cache directory. However, the caches of domain
capabilities, which are created and used by libvirtd, are also put in
the cache directory. In order to make the cache directory more secure,
move the temp files of screenshot and memory peek to per-domain
directory.
Since the temp files are just temporary files and are only used by
libvirtd (libvirtd will delete them after use), the use of screenshot
and memory peek will be affected.
Signed-off-by: Peng Liang <liangpeng10(a)huawei.com>
---
src/qemu/qemu_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dfc27572c461..7ffe5f1e3856 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3431,7 +3431,7 @@ qemuDomainScreenshot(virDomainPtr dom,
}
}
- tmp = g_strdup_printf("%s/qemu.screendump.XXXXXX", cfg->cacheDir);
+ tmp = g_strdup_printf("%s/qemu.screendump.XXXXXX", priv->libDir);
if ((tmp_fd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) {
virReportSystemError(errno, _("g_mkstemp(\"%s\") failed"),
tmp);
@@ -10675,6 +10675,7 @@ qemuDomainMemoryPeek(virDomainPtr dom,
if (!(vm = qemuDomainObjFromDomain(dom)))
goto cleanup;
+ priv = vm->privateData;
cfg = virQEMUDriverGetConfig(driver);
if (virDomainMemoryPeekEnsureACL(dom->conn, vm->def) < 0)
@@ -10692,7 +10693,7 @@ qemuDomainMemoryPeek(virDomainPtr dom,
if (virDomainObjCheckActive(vm) < 0)
goto endjob;
- tmp = g_strdup_printf("%s/qemu.mem.XXXXXX", cfg->cacheDir);
+ tmp = g_strdup_printf("%s/qemu.mem.XXXXXX", priv->libDir);
/* Create a temporary filename. */
if ((fd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) {
@@ -10703,7 +10704,6 @@ qemuDomainMemoryPeek(virDomainPtr dom,
qemuSecurityDomainSetPathLabel(driver, vm, tmp, false);
- priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
if (flags == VIR_MEMORY_VIRTUAL) {
if (qemuMonitorSaveVirtualMemory(priv->mon, offset, size, tmp) < 0) {
--
2.31.1
9:23 a.m.
New subject: [PATCH v3 2/2] qemu: don't change ownership of cache directory
Commit 6bcf25017bc6 ("virDomainMemoryPeek API") introduced memory peek
and commit 9936aecfd1b4 ("qemu: Implement the driver methods")
introduced screenshot. Both of them will put temporary files in
/var/cache/libvirt/qemu, and the temporary files are created by QEMU.
Therefore, the ownership of /var/cache/libvirt/qemu should be changed to
user and group configured in qemu.conf to make sure that QEMU process
can create and write files in the cache directory.
Libvirt will only put the temporary files in /var/cache/libvirt/qemu
until commit cbde35899b90 ("Cache result of QEMU capabilities
extraction"), which will put the cache of QEMU capabilities in
'capabilities' subdir of the cache directory. Because the capabilities
is used by libvirt, the ownership of both 'capabilities' subdir and
capabilitie files are root. However, when QEMU process runs as a
regular user (e.g. qemu user), the ownership of /var/cache/libvirt/qemu
will be changed to qemu:qemu while that of
/var/cache/libvirt/qemu/capabilities will be still root:root. Then the
regular user could spoof different capabilities, which maybe lead to
denial of service.
Since the previous patch has move the temp files of screenshot and
memory peek to per-domain directory, no one except domain capabilities
uses cacheDir currently. And since domain capabilities are used by
libvirtd instead of QEMU, no need to change the ownership of cacheDir to
qemu:qemu explicitly.
Signed-off-by: Peng Liang <liangpeng10(a)huawei.com>
---
src/qemu/qemu_driver.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7ffe5f1e3856..818889219b11 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -748,13 +748,6 @@ qemuStateInitialize(bool privileged,
(int)cfg->group);
goto error;
}
- if (chown(cfg->cacheDir, cfg->user, cfg->group) < 0) {
- virReportSystemError(errno,
- _("unable to set ownership of '%s' to
%d:%d"),
- cfg->cacheDir, (int)cfg->user,
- (int)cfg->group);
- goto error;
- }
if (chown(cfg->saveDir, cfg->user, cfg->group) < 0) {
virReportSystemError(errno,
_("unable to set ownership of '%s' to
%d:%d"),
--
2.31.1
5:48 a.m.
On 9/13/21 4:23 PM, Peng Liang wrote:
See path 2 for detail.
v2 -> v3:
- Change the directory to put the temp files of screenshot and
memorypeek from autoDumpPath to per-domain directory [Daniel]
v1 -> v2:
- Add commit message for path1 [Peter]
Peng Liang (2):
qemu: move temp file of screenshot and memorypeek to per-domain dir
qemu: don't change ownership of cache directory
src/qemu/qemu_driver.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
and pushed.
Michal
1192
days inactive
1202
days old
3 comments
2 participants
participants (2)
-
Michal Prívozník -
Peng Liang