10:31 p.m.
Pavel Hrdina (4):
conf: move seclabel for chardev source to the correct sturcture
qemu: introduce chardevStdioLogd to qemu private data
qemu: propagate chardevStdioLogd to qemuBuildChrChardevStr
security: don't relabel chardev source if virtlogd is used as stdio
handler
src/conf/domain_conf.c | 46 +++++++-------
src/conf/domain_conf.h | 9 +--
src/lxc/lxc_process.c | 6 +-
src/qemu/qemu_command.c | 132 ++++++++++++++++++++++++++-------------
src/qemu/qemu_command.h | 3 +-
src/qemu/qemu_domain.c | 6 ++
src/qemu/qemu_domain.h | 3 +
src/qemu/qemu_process.c | 15 ++++-
src/qemu/qemu_security.c | 9 ++-
src/security/security_apparmor.c | 7 ++-
src/security/security_dac.c | 74 +++++++++++++++-------
src/security/security_driver.h | 6 +-
src/security/security_manager.c | 16 +++--
src/security/security_manager.h | 6 +-
src/security/security_nop.c | 6 +-
src/security/security_selinux.c | 71 ++++++++++++++-------
src/security/security_stack.c | 12 ++--
tests/securityselinuxlabeltest.c | 2 +-
18 files changed, 281 insertions(+), 148 deletions(-)
--
2.13.0
10:31 p.m.
New subject: [libvirt] [PATCH v2 1/4] conf: move seclabel for chardev source to the correct sturcture
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Notes:
new in v2
src/conf/domain_conf.c | 46 +++++++++++++++++++----------------------
src/conf/domain_conf.h | 9 ++++----
src/security/security_dac.c | 26 ++++++++++-------------
src/security/security_manager.c | 4 ++--
src/security/security_selinux.c | 24 +++++++++------------
5 files changed, 49 insertions(+), 60 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c7e20b8ba1..68dc2832cb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2076,12 +2076,21 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
{
+ size_t i;
+
if (!def)
return;
virDomainChrSourceDefClear(def);
virObjectUnref(def->privateData);
+ if (def->seclabels) {
+ for (i = 0; i < def->nseclabels; i++)
+ virSecurityDeviceLabelDefFree(def->seclabels[i]);
+ VIR_FREE(def->seclabels);
+ }
+
+
VIR_FREE(def);
}
@@ -2150,8 +2159,6 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
void virDomainChrDefFree(virDomainChrDefPtr def)
{
- size_t i;
-
if (!def)
return;
@@ -2176,12 +2183,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
virDomainChrSourceDefFree(def->source);
virDomainDeviceInfoClear(&def->info);
- if (def->seclabels) {
- for (i = 0; i < def->nseclabels; i++)
- virSecurityDeviceLabelDefFree(def->seclabels[i]);
- VIR_FREE(def->seclabels);
- }
-
VIR_FREE(def);
}
@@ -10688,8 +10689,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
if (chr_def) {
xmlNodePtr saved_node = ctxt->node;
ctxt->node = cur;
- if (virSecurityDeviceLabelDefParseXML(&chr_def->seclabels,
- &chr_def->nseclabels,
+ if (virSecurityDeviceLabelDefParseXML(&def->seclabels,
+ &def->nseclabels,
vmSeclabels,
nvmSeclabels,
ctxt,
@@ -22399,19 +22400,11 @@ virDomainNetDefFormat(virBufferPtr buf,
* output at " type='type'>". */
static int
virDomainChrSourceDefFormat(virBufferPtr buf,
- virDomainChrDefPtr chr_def,
virDomainChrSourceDefPtr def,
bool tty_compat,
unsigned int flags)
{
const char *type = virDomainChrTypeToString(def->type);
- size_t nseclabels = 0;
- virSecurityDeviceLabelDefPtr *seclabels = NULL;
-
- if (chr_def) {
- nseclabels = chr_def->nseclabels;
- seclabels = chr_def->seclabels;
- }
if (!type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -22449,7 +22442,8 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
def->data.file.append != VIR_TRISTATE_SWITCH_ABSENT)
virBufferAsprintf(buf, " append='%s'",
virTristateSwitchTypeToString(def->data.file.append));
- virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
+ virDomainSourceDefFormatSeclabel(buf, def->nseclabels,
+ def->seclabels, flags);
}
break;
@@ -22504,7 +22498,8 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "<source mode='%s'",
def->data.nix.listen ? "bind" :
"connect");
virBufferEscapeString(buf, " path='%s'",
def->data.nix.path);
- virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
+ virDomainSourceDefFormatSeclabel(buf, def->nseclabels,
+ def->seclabels, flags);
}
break;
@@ -22553,7 +22548,7 @@ virDomainChrDefFormat(virBufferPtr buf,
def->source->type == VIR_DOMAIN_CHR_TYPE_PTY &&
!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
def->source->data.file.path);
- if (virDomainChrSourceDefFormat(buf, def, def->source, tty_compat, flags) < 0)
+ if (virDomainChrSourceDefFormat(buf, def->source, tty_compat, flags) < 0)
return -1;
/* Format <target> block */
@@ -22675,7 +22670,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
break;
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
- if (virDomainChrSourceDefFormat(buf, NULL, def->data.passthru, false,
+ if (virDomainChrSourceDefFormat(buf, def->data.passthru, false,
flags) < 0)
return -1;
break;
@@ -22981,7 +22976,7 @@ virDomainRNGDefFormat(virBufferPtr buf,
case VIR_DOMAIN_RNG_BACKEND_EGD:
virBufferAdjustIndent(buf, 2);
- if (virDomainChrSourceDefFormat(buf, NULL, def->source.chardev,
+ if (virDomainChrSourceDefFormat(buf, def->source.chardev,
false, flags) < 0)
return -1;
virBufferAdjustIndent(buf, -2);
@@ -23797,7 +23792,7 @@ virDomainRedirdevDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "<redirdev bus='%s'", bus);
virBufferAdjustIndent(buf, 2);
- if (virDomainChrSourceDefFormat(buf, NULL, def->source, false, flags) < 0)
+ if (virDomainChrSourceDefFormat(buf, def->source, false, flags) < 0)
return -1;
if (virDomainDeviceInfoFormat(buf, &def->info,
flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0)
@@ -26195,7 +26190,8 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char
*model)
virSecurityDeviceLabelDefPtr
-virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model)
+virDomainChrSourceDefGetSecurityLabelDef(virDomainChrSourceDefPtr def,
+ const char *model)
{
size_t i;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 83e0672691..1951ba74bb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1166,6 +1166,9 @@ struct _virDomainChrSourceDef {
} data;
char *logfile;
int logappend;
+
+ size_t nseclabels;
+ virSecurityDeviceLabelDefPtr *seclabels;
};
/* A complete character device, both host and domain views. */
@@ -1188,9 +1191,6 @@ struct _virDomainChrDef {
virDomainChrSourceDefPtr source;
virDomainDeviceInfo info;
-
- size_t nseclabels;
- virSecurityDeviceLabelDefPtr *seclabels;
};
typedef enum {
@@ -3068,7 +3068,8 @@ virSecurityLabelDefPtr
virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model);
virSecurityDeviceLabelDefPtr
-virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);
+virDomainChrSourceDefGetSecurityLabelDef(virDomainChrSourceDefPtr def,
+ const char *model);
typedef const char* (*virEventActionToStringFunc)(int type);
typedef int (*virEventActionFromStringFunc)(const char *type);
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 7dcf4c15f7..fd4d8f5047 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1159,7 +1159,6 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
{
@@ -1173,9 +1172,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
- if (dev)
- chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
- SECURITY_DAC_NAME);
+ chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
+ SECURITY_DAC_NAME);
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
@@ -1245,7 +1243,6 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def ATTRIBUTE_UNUSED,
- virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
@@ -1253,9 +1250,8 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
char *in = NULL, *out = NULL;
int ret = -1;
- if (dev)
- chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
- SECURITY_DAC_NAME);
+ chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
+ SECURITY_DAC_NAME);
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
@@ -1304,12 +1300,12 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreChardevCallback(virDomainDefPtr def,
- virDomainChrDefPtr dev,
+ virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
virSecurityManagerPtr mgr = opaque;
- return virSecurityDACRestoreChardevLabel(mgr, def, dev, dev->source);
+ return virSecurityDACRestoreChardevLabel(mgr, def, dev->source);
}
@@ -1322,7 +1318,7 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
- ret = virSecurityDACSetChardevLabel(mgr, def, NULL,
+ ret = virSecurityDACSetChardevLabel(mgr, def,
&tpm->data.passthrough.source);
break;
case VIR_DOMAIN_TPM_TYPE_LAST:
@@ -1342,8 +1338,8 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
- ret = virSecurityDACRestoreChardevLabel(mgr, def, NULL,
- &tpm->data.passthrough.source);
+ ret = virSecurityDACRestoreChardevLabel(mgr, def,
+ &tpm->data.passthrough.source);
break;
case VIR_DOMAIN_TPM_TYPE_LAST:
break;
@@ -1506,12 +1502,12 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACSetChardevCallback(virDomainDefPtr def,
- virDomainChrDefPtr dev,
+ virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
virSecurityManagerPtr mgr = opaque;
- return virSecurityDACSetChardevLabel(mgr, def, dev, dev->source);
+ return virSecurityDACSetChardevLabel(mgr, def, dev->source);
}
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 6c777db1e6..90d491c1bc 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -811,8 +811,8 @@ virSecurityManagerCheckChardevLabel(virSecurityManagerPtr mgr,
{
size_t i;
- for (i = 0; i < dev->nseclabels; i++) {
- if (virSecurityManagerCheckModel(mgr, dev->seclabels[i]->model) < 0)
+ for (i = 0; i < dev->source->nseclabels; i++) {
+ if (virSecurityManagerCheckModel(mgr, dev->source->seclabels[i]->model)
< 0)
return -1;
}
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 9504a4be34..75f387b3fa 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2179,7 +2179,6 @@ virSecuritySELinuxRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
{
@@ -2193,9 +2192,8 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
if (!seclabel || !seclabel->relabel)
return 0;
- if (dev)
- chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
- SECURITY_SELINUX_NAME);
+ chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
+ SECURITY_SELINUX_NAME);
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
@@ -2254,7 +2252,6 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
{
@@ -2267,9 +2264,8 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
if (!seclabel || !seclabel->relabel)
return 0;
- if (dev)
- chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
- SECURITY_SELINUX_NAME);
+ chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
+ SECURITY_SELINUX_NAME);
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
@@ -2318,12 +2314,12 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def,
- virDomainChrDefPtr dev,
+ virDomainChrDefPtr dev
ATTRIBUTE_UNUSED,
void *opaque)
{
virSecurityManagerPtr mgr = opaque;
- return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev, dev->source);
+ return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->source);
}
@@ -2346,7 +2342,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr
def,
return virSecuritySELinuxRestoreFileLabel(mgr, database);
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
- return virSecuritySELinuxRestoreChardevLabel(mgr, def, NULL,
dev->data.passthru);
+ return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->data.passthru);
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2707,12 +2703,12 @@ virSecuritySELinuxClearSocketLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
static int
virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def,
- virDomainChrDefPtr dev,
+ virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
virSecurityManagerPtr mgr = opaque;
- return virSecuritySELinuxSetChardevLabel(mgr, def, dev, dev->source);
+ return virSecuritySELinuxSetChardevLabel(mgr, def, dev->source);
}
@@ -2736,7 +2732,7 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def,
return virSecuritySELinuxSetFilecon(mgr, database, data->content_context);
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
- return virSecuritySELinuxSetChardevLabel(mgr, def, NULL,
+ return virSecuritySELinuxSetChardevLabel(mgr, def,
dev->data.passthru);
default:
--
2.13.0
12:35 a.m.
New subject: [libvirt] [PATCH v2 1/4] conf: move seclabel for chardev source to the correct sturcture
On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Notes:
new in v2
src/conf/domain_conf.c | 46 +++++++++++++++++++----------------------
src/conf/domain_conf.h | 9 ++++----
src/security/security_dac.c | 26 ++++++++++-------------
src/security/security_manager.c | 4 ++--
src/security/security_selinux.c | 24 +++++++++------------
5 files changed, 49 insertions(+), 60 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c7e20b8ba1..68dc2832cb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2076,12 +2076,21 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
{
+ size_t i;
+
if (!def)
return;
virDomainChrSourceDefClear(def);
virObjectUnref(def->privateData);
+ if (def->seclabels) {
+ for (i = 0; i < def->nseclabels; i++)
+ virSecurityDeviceLabelDefFree(def->seclabels[i]);
+ VIR_FREE(def->seclabels);
+ }
+
+
VIR_FREE(def);
}
@@ -2150,8 +2159,6 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
void virDomainChrDefFree(virDomainChrDefPtr def)
{
- size_t i;
-
if (!def)
return;
@@ -2176,12 +2183,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
virDomainChrSourceDefFree(def->source);
virDomainDeviceInfoClear(&def->info);
- if (def->seclabels) {
- for (i = 0; i < def->nseclabels; i++)
- virSecurityDeviceLabelDefFree(def->seclabels[i]);
- VIR_FREE(def->seclabels);
- }
-
VIR_FREE(def);
}
@@ -10688,8 +10689,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
if (chr_def) {
Is the @chr_def check necessary still? I assume it's there because
chr_def can be passed as NULL in some cases.
Looks like all this was added as part of commit 'f8b08d0e'
The chr_def would be NULL for Smartcard, RNG, and Redirdev callers which
each now doesn't pass a NULL to virDomainChrSourceDefFormat, so that
leads me to believe that the @chr_def should be removed.
The rest looks good, so
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
xmlNodePtr saved_node = ctxt->node;
ctxt->node = cur;
- if (virSecurityDeviceLabelDefParseXML(&chr_def->seclabels,
- &chr_def->nseclabels,
+ if (virSecurityDeviceLabelDefParseXML(&def->seclabels,
+ &def->nseclabels,
vmSeclabels,
nvmSeclabels,
ctxt,
@@ -22399,19 +22400,11 @@ virDomainNetDefFormat(virBufferPtr buf,
* output at " type='type'>". */
static int
virDomainChrSourceDefFormat(virBufferPtr buf,
- virDomainChrDefPtr chr_def,
virDomainChrSourceDefPtr def,
bool tty_compat,
unsigned int flags)
{
const char *type = virDomainChrTypeToString(def->type);
- size_t nseclabels = 0;
- virSecurityDeviceLabelDefPtr *seclabels = NULL;
-
- if (chr_def) {
- nseclabels = chr_def->nseclabels;
- seclabels = chr_def->seclabels;
- }
if (!type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -22449,7 +22442,8 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
def->data.file.append != VIR_TRISTATE_SWITCH_ABSENT)
virBufferAsprintf(buf, " append='%s'",
virTristateSwitchTypeToString(def->data.file.append));
- virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
+ virDomainSourceDefFormatSeclabel(buf, def->nseclabels,
+ def->seclabels, flags);
}
break;
@@ -22504,7 +22498,8 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "<source mode='%s'",
def->data.nix.listen ? "bind" :
"connect");
virBufferEscapeString(buf, " path='%s'",
def->data.nix.path);
- virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
+ virDomainSourceDefFormatSeclabel(buf, def->nseclabels,
+ def->seclabels, flags);
}
break;
@@ -22553,7 +22548,7 @@ virDomainChrDefFormat(virBufferPtr buf,
def->source->type == VIR_DOMAIN_CHR_TYPE_PTY &&
!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
def->source->data.file.path);
- if (virDomainChrSourceDefFormat(buf, def, def->source, tty_compat, flags) <
0)
+ if (virDomainChrSourceDefFormat(buf, def->source, tty_compat, flags) < 0)
return -1;
/* Format <target> block */
@@ -22675,7 +22670,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
break;
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
- if (virDomainChrSourceDefFormat(buf, NULL, def->data.passthru, false,
+ if (virDomainChrSourceDefFormat(buf, def->data.passthru, false,
flags) < 0)
return -1;
break;
@@ -22981,7 +22976,7 @@ virDomainRNGDefFormat(virBufferPtr buf,
case VIR_DOMAIN_RNG_BACKEND_EGD:
virBufferAdjustIndent(buf, 2);
- if (virDomainChrSourceDefFormat(buf, NULL, def->source.chardev,
+ if (virDomainChrSourceDefFormat(buf, def->source.chardev,
false, flags) < 0)
return -1;
virBufferAdjustIndent(buf, -2);
@@ -23797,7 +23792,7 @@ virDomainRedirdevDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "<redirdev bus='%s'", bus);
virBufferAdjustIndent(buf, 2);
- if (virDomainChrSourceDefFormat(buf, NULL, def->source, false, flags) < 0)
+ if (virDomainChrSourceDefFormat(buf, def->source, false, flags) < 0)
return -1;
if (virDomainDeviceInfoFormat(buf, &def->info,
flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0)
@@ -26195,7 +26190,8 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char
*model)
virSecurityDeviceLabelDefPtr
-virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model)
+virDomainChrSourceDefGetSecurityLabelDef(virDomainChrSourceDefPtr def,
+ const char *model)
{
size_t i;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 83e0672691..1951ba74bb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1166,6 +1166,9 @@ struct _virDomainChrSourceDef {
} data;
char *logfile;
int logappend;
+
+ size_t nseclabels;
+ virSecurityDeviceLabelDefPtr *seclabels;
};
/* A complete character device, both host and domain views. */
@@ -1188,9 +1191,6 @@ struct _virDomainChrDef {
virDomainChrSourceDefPtr source;
virDomainDeviceInfo info;
-
- size_t nseclabels;
- virSecurityDeviceLabelDefPtr *seclabels;
};
typedef enum {
@@ -3068,7 +3068,8 @@ virSecurityLabelDefPtr
virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model);
virSecurityDeviceLabelDefPtr
-virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);
+virDomainChrSourceDefGetSecurityLabelDef(virDomainChrSourceDefPtr def,
+ const char *model);
typedef const char* (*virEventActionToStringFunc)(int type);
typedef int (*virEventActionFromStringFunc)(const char *type);
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 7dcf4c15f7..fd4d8f5047 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1159,7 +1159,6 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
{
@@ -1173,9 +1172,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
- if (dev)
- chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
- SECURITY_DAC_NAME);
+ chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
+ SECURITY_DAC_NAME);
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
@@ -1245,7 +1243,6 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def ATTRIBUTE_UNUSED,
- virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
@@ -1253,9 +1250,8 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
char *in = NULL, *out = NULL;
int ret = -1;
- if (dev)
- chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
- SECURITY_DAC_NAME);
+ chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
+ SECURITY_DAC_NAME);
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
@@ -1304,12 +1300,12 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreChardevCallback(virDomainDefPtr def,
- virDomainChrDefPtr dev,
+ virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
virSecurityManagerPtr mgr = opaque;
- return virSecurityDACRestoreChardevLabel(mgr, def, dev, dev->source);
+ return virSecurityDACRestoreChardevLabel(mgr, def, dev->source);
}
@@ -1322,7 +1318,7 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
- ret = virSecurityDACSetChardevLabel(mgr, def, NULL,
+ ret = virSecurityDACSetChardevLabel(mgr, def,
&tpm->data.passthrough.source);
break;
case VIR_DOMAIN_TPM_TYPE_LAST:
@@ -1342,8 +1338,8 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
- ret = virSecurityDACRestoreChardevLabel(mgr, def, NULL,
- &tpm->data.passthrough.source);
+ ret = virSecurityDACRestoreChardevLabel(mgr, def,
+ &tpm->data.passthrough.source);
break;
case VIR_DOMAIN_TPM_TYPE_LAST:
break;
@@ -1506,12 +1502,12 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACSetChardevCallback(virDomainDefPtr def,
- virDomainChrDefPtr dev,
+ virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
virSecurityManagerPtr mgr = opaque;
- return virSecurityDACSetChardevLabel(mgr, def, dev, dev->source);
+ return virSecurityDACSetChardevLabel(mgr, def, dev->source);
}
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 6c777db1e6..90d491c1bc 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -811,8 +811,8 @@ virSecurityManagerCheckChardevLabel(virSecurityManagerPtr mgr,
{
size_t i;
- for (i = 0; i < dev->nseclabels; i++) {
- if (virSecurityManagerCheckModel(mgr, dev->seclabels[i]->model) < 0)
+ for (i = 0; i < dev->source->nseclabels; i++) {
+ if (virSecurityManagerCheckModel(mgr, dev->source->seclabels[i]->model)
< 0)
return -1;
}
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 9504a4be34..75f387b3fa 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2179,7 +2179,6 @@ virSecuritySELinuxRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
{
@@ -2193,9 +2192,8 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
if (!seclabel || !seclabel->relabel)
return 0;
- if (dev)
- chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
- SECURITY_SELINUX_NAME);
+ chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
+ SECURITY_SELINUX_NAME);
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
@@ -2254,7 +2252,6 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
{
@@ -2267,9 +2264,8 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
if (!seclabel || !seclabel->relabel)
return 0;
- if (dev)
- chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
- SECURITY_SELINUX_NAME);
+ chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
+ SECURITY_SELINUX_NAME);
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
@@ -2318,12 +2314,12 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def,
- virDomainChrDefPtr dev,
+ virDomainChrDefPtr dev
ATTRIBUTE_UNUSED,
void *opaque)
{
virSecurityManagerPtr mgr = opaque;
- return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev, dev->source);
+ return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->source);
}
@@ -2346,7 +2342,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr
def,
return virSecuritySELinuxRestoreFileLabel(mgr, database);
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
- return virSecuritySELinuxRestoreChardevLabel(mgr, def, NULL,
dev->data.passthru);
+ return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->data.passthru);
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2707,12 +2703,12 @@ virSecuritySELinuxClearSocketLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
static int
virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def,
- virDomainChrDefPtr dev,
+ virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
virSecurityManagerPtr mgr = opaque;
- return virSecuritySELinuxSetChardevLabel(mgr, def, dev, dev->source);
+ return virSecuritySELinuxSetChardevLabel(mgr, def, dev->source);
}
@@ -2736,7 +2732,7 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr
def,
return virSecuritySELinuxSetFilecon(mgr, database, data->content_context);
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
- return virSecuritySELinuxSetChardevLabel(mgr, def, NULL,
+ return virSecuritySELinuxSetChardevLabel(mgr, def,
dev->data.passthru);
default:
2:39 p.m.
New subject: [libvirt] [PATCH v2 1/4] conf: move seclabel for chardev source to the correct sturcture
On Tue, Jun 13, 2017 at 12:35:10PM -0400, John Ferlan wrote:
On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
>
> Notes:
> new in v2
>
> src/conf/domain_conf.c | 46 +++++++++++++++++++----------------------
> src/conf/domain_conf.h | 9 ++++----
> src/security/security_dac.c | 26 ++++++++++-------------
> src/security/security_manager.c | 4 ++--
> src/security/security_selinux.c | 24 +++++++++------------
> 5 files changed, 49 insertions(+), 60 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c7e20b8ba1..68dc2832cb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2076,12 +2076,21 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
>
> void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
> {
> + size_t i;
> +
> if (!def)
> return;
>
> virDomainChrSourceDefClear(def);
> virObjectUnref(def->privateData);
>
> + if (def->seclabels) {
> + for (i = 0; i < def->nseclabels; i++)
> + virSecurityDeviceLabelDefFree(def->seclabels[i]);
> + VIR_FREE(def->seclabels);
> + }
> +
> +
> VIR_FREE(def);
> }
>
> @@ -2150,8 +2159,6 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef
*src,
>
> void virDomainChrDefFree(virDomainChrDefPtr def)
> {
> - size_t i;
> -
> if (!def)
> return;
>
> @@ -2176,12 +2183,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
> virDomainChrSourceDefFree(def->source);
> virDomainDeviceInfoClear(&def->info);
>
> - if (def->seclabels) {
> - for (i = 0; i < def->nseclabels; i++)
> - virSecurityDeviceLabelDefFree(def->seclabels[i]);
> - VIR_FREE(def->seclabels);
> - }
> -
> VIR_FREE(def);
> }
>
> @@ -10688,8 +10689,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> if (chr_def) {
Is the @chr_def check necessary still? I assume it's there because
chr_def can be passed as NULL in some cases.
Looks like all this was added as part of commit 'f8b08d0e'
The chr_def would be NULL for Smartcard, RNG, and Redirdev callers which
each now doesn't pass a NULL to virDomainChrSourceDefFormat, so that
leads me to believe that the @chr_def should be removed.
But this hunk is from virDomainChrSourceDefParseXML() function.
The rest looks good, so
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks
Pavel
6:50 p.m.
New subject: [libvirt] [PATCH v2 1/4] conf: move seclabel for chardev source to the correct sturcture
On 06/15/2017 02:39 AM, Pavel Hrdina wrote:
On Tue, Jun 13, 2017 at 12:35:10PM -0400, John Ferlan wrote:
>
>
> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>> ---
>>
>> Notes:
>> new in v2
>>
>> src/conf/domain_conf.c | 46 +++++++++++++++++++----------------------
>> src/conf/domain_conf.h | 9 ++++----
>> src/security/security_dac.c | 26 ++++++++++-------------
>> src/security/security_manager.c | 4 ++--
>> src/security/security_selinux.c | 24 +++++++++------------
>> 5 files changed, 49 insertions(+), 60 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index c7e20b8ba1..68dc2832cb 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2076,12 +2076,21 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
>>
>> void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
>> {
>> + size_t i;
>> +
>> if (!def)
>> return;
>>
>> virDomainChrSourceDefClear(def);
>> virObjectUnref(def->privateData);
>>
>> + if (def->seclabels) {
>> + for (i = 0; i < def->nseclabels; i++)
>> + virSecurityDeviceLabelDefFree(def->seclabels[i]);
>> + VIR_FREE(def->seclabels);
>> + }
>> +
>> +
>> VIR_FREE(def);
>> }
>>
>> @@ -2150,8 +2159,6 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef
*src,
>>
>> void virDomainChrDefFree(virDomainChrDefPtr def)
>> {
>> - size_t i;
>> -
>> if (!def)
>> return;
>>
>> @@ -2176,12 +2183,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
>> virDomainChrSourceDefFree(def->source);
>> virDomainDeviceInfoClear(&def->info);
>>
>> - if (def->seclabels) {
>> - for (i = 0; i < def->nseclabels; i++)
>> - virSecurityDeviceLabelDefFree(def->seclabels[i]);
>> - VIR_FREE(def->seclabels);
>> - }
>> -
>> VIR_FREE(def);
>> }
>>
>> @@ -10688,8 +10689,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr
def,
>> if (chr_def) {
>
> Is the @chr_def check necessary still? I assume it's there because
> chr_def can be passed as NULL in some cases.
>
> Looks like all this was added as part of commit 'f8b08d0e'
>
> The chr_def would be NULL for Smartcard, RNG, and Redirdev callers which
> each now doesn't pass a NULL to virDomainChrSourceDefFormat, so that
> leads me to believe that the @chr_def should be removed.
But this hunk is from virDomainChrSourceDefParseXML() function.
Yes, I knew that when I wrote the comment, but that wasn't the point.
Since you've moved the labels into @def instead and similarly altered
calls to virDomainChrSourceDefFormat such that they don't receive
chr_def (in the very next hunk of changes BTW), then I would think at
this point removing chr_def would be safe, but I suppose I could be
wrong. Hence why I asked.
So does it hurt to keep it, probably not; however, IIUC the reason why
it was there was because if it wasn't, then dereferencing chr_def to get
the [n]seclabels in the subsequent call wouldn't end well.
John
> The rest looks good, so
>
> Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks
Pavel
8:51 p.m.
New subject: [libvirt] [PATCH v2 1/4] conf: move seclabel for chardev source to the correct sturcture
On Thu, Jun 15, 2017 at 06:50:34AM -0400, John Ferlan wrote:
On 06/15/2017 02:39 AM, Pavel Hrdina wrote:
> On Tue, Jun 13, 2017 at 12:35:10PM -0400, John Ferlan wrote:
>>
>>
>> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
>>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>>> ---
>>>
>>> Notes:
>>> new in v2
>>>
>>> src/conf/domain_conf.c | 46
+++++++++++++++++++----------------------
>>> src/conf/domain_conf.h | 9 ++++----
>>> src/security/security_dac.c | 26 ++++++++++-------------
>>> src/security/security_manager.c | 4 ++--
>>> src/security/security_selinux.c | 24 +++++++++------------
>>> 5 files changed, 49 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index c7e20b8ba1..68dc2832cb 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -2076,12 +2076,21 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr
dest,
>>>
>>> void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
>>> {
>>> + size_t i;
>>> +
>>> if (!def)
>>> return;
>>>
>>> virDomainChrSourceDefClear(def);
>>> virObjectUnref(def->privateData);
>>>
>>> + if (def->seclabels) {
>>> + for (i = 0; i < def->nseclabels; i++)
>>> + virSecurityDeviceLabelDefFree(def->seclabels[i]);
>>> + VIR_FREE(def->seclabels);
>>> + }
>>> +
>>> +
>>> VIR_FREE(def);
>>> }
>>>
>>> @@ -2150,8 +2159,6 @@ virDomainChrSourceDefIsEqual(const
virDomainChrSourceDef *src,
>>>
>>> void virDomainChrDefFree(virDomainChrDefPtr def)
>>> {
>>> - size_t i;
>>> -
>>> if (!def)
>>> return;
>>>
>>> @@ -2176,12 +2183,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
>>> virDomainChrSourceDefFree(def->source);
>>> virDomainDeviceInfoClear(&def->info);
>>>
>>> - if (def->seclabels) {
>>> - for (i = 0; i < def->nseclabels; i++)
>>> - virSecurityDeviceLabelDefFree(def->seclabels[i]);
>>> - VIR_FREE(def->seclabels);
>>> - }
>>> -
>>> VIR_FREE(def);
>>> }
>>>
>>> @@ -10688,8 +10689,8 @@
virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>>> if (chr_def) {
>>
>> Is the @chr_def check necessary still? I assume it's there because
>> chr_def can be passed as NULL in some cases.
>>
>> Looks like all this was added as part of commit 'f8b08d0e'
>>
>> The chr_def would be NULL for Smartcard, RNG, and Redirdev callers which
>> each now doesn't pass a NULL to virDomainChrSourceDefFormat, so that
>> leads me to believe that the @chr_def should be removed.
>
> But this hunk is from virDomainChrSourceDefParseXML() function.
>
Yes, I knew that when I wrote the comment, but that wasn't the point.
Since you've moved the labels into @def instead and similarly altered
calls to virDomainChrSourceDefFormat such that they don't receive
chr_def (in the very next hunk of changes BTW), then I would think at
this point removing chr_def would be safe, but I suppose I could be
wrong. Hence why I asked.
So does it hurt to keep it, probably not; however, IIUC the reason why
it was there was because if it wasn't, then dereferencing chr_def to get
the [n]seclabels in the subsequent call wouldn't end well.
Oh right. The only thing what that check currently does is that for
smartcard, rng and redirdev it skips parsing of the seclabel. I would
probably leave it to a separate patch which would ensure that we don't
start parsing the seclabel for these devices.
Pavel
John
>> The rest looks good, so
>>
>> Reviewed-by: John Ferlan <jferlan(a)redhat.com>
>
> Thanks
>
> Pavel
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:31 p.m.
New subject: [libvirt] [PATCH v2 2/4] qemu: introduce chardevStdioLogd to qemu private data
In QEMU driver we can use virtlogd as stdio handler for source backend
of char devices if current QEMU is new enough and it's enabled in
qemu.conf. We should store this information while starting a guest
because the config option may change while the guest is running.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Notes:
new in v2
src/qemu/qemu_domain.c | 6 ++++++
src/qemu/qemu_domain.h | 3 +++
src/qemu/qemu_process.c | 9 +++++++++
3 files changed, 18 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0a85ee9d74..b0e3df7009 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1873,6 +1873,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
virBufferEscapeString(buf, "<channelTargetDir
path='%s'/>\n",
priv->channelTargetDir);
+ if (priv->chardevStdioLogd)
+ virBufferAddLit(buf, "<chardevStdioLogd/>");
+
return 0;
}
@@ -2141,6 +2144,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
if (qemuDomainSetPrivatePathsOld(driver, vm) < 0)
goto error;
+ priv->chardevStdioLogd = virXPathBoolean("boolean(./chardevStdioLogd)",
+ ctxt) == 1;
+
return 0;
error:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index aebd91ad37..9fb7c339a3 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -293,6 +293,9 @@ struct _qemuDomainObjPrivate {
/* Used when fetching/storing the current 'tls-creds' migration setting */
/* (not to be saved in our private XML). */
char *migTLSAlias;
+
+ /* If true virtlogd is used as stdio handler for character devices. */
+ bool chardevStdioLogd;
};
# define QEMU_DOMAIN_PRIVATE(vm) \
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index be031b56b9..77c2e5f6d3 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5367,6 +5367,7 @@ qemuProcessPrepareDomain(virConnectPtr conn,
size_t i;
char *nodeset = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
virCapsPtr caps;
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
@@ -5403,6 +5404,13 @@ qemuProcessPrepareDomain(virConnectPtr conn,
}
}
+ /* Whether we should use virtlogd as stdio handler for character
+ * devices source backend. */
+ if (cfg->stdioLogD &&
+ virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) {
+ priv->chardevStdioLogd = true;
+ }
+
/*
* Normally PCI addresses are assigned in the virDomainCreate
* or virDomainDefine methods. We might still need to assign
@@ -5466,6 +5474,7 @@ qemuProcessPrepareDomain(virConnectPtr conn,
cleanup:
VIR_FREE(nodeset);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
--
2.13.0
12:54 a.m.
New subject: [libvirt] [PATCH v2 2/4] qemu: introduce chardevStdioLogd to qemu private data
On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
In QEMU driver we can use virtlogd as stdio handler for source
backend
of char devices if current QEMU is new enough and it's enabled in
qemu.conf. We should store this information while starting a guest
because the config option may change while the guest is running.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Notes:
new in v2
src/qemu/qemu_domain.c | 6 ++++++
src/qemu/qemu_domain.h | 3 +++
src/qemu/qemu_process.c | 9 +++++++++
3 files changed, 18 insertions(+)
FWIW: This wouldn't git am -3 apply on top of tree, I found if I went
"back to" commit '880b1a2e', then I could apply. I was mainly a random
selection, although I did also try the patch just before jtomko's series
as well.
In any case,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
10:31 p.m.
New subject: [libvirt] [PATCH v2 3/4] qemu: propagate chardevStdioLogd to qemuBuildChrChardevStr
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Notes:
new in v2
This is not required to fix the issue by the last patch in the series,
however it improves the code that we decide whether to use virtlogd or
not by checking the same variable that is updated while preparing the
guest start.
src/qemu/qemu_command.c | 132 +++++++++++++++++++++++++++++++-----------------
src/qemu/qemu_command.h | 3 +-
src/qemu/qemu_process.c | 6 ++-
3 files changed, 93 insertions(+), 48 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 015af1036c..e6c50d1a64 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5043,7 +5043,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
const virDomainChrSourceDef *dev,
const char *alias,
virQEMUCapsPtr qemuCaps,
- bool nowait)
+ bool nowait,
+ bool chardevStdioLogd)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
bool telnet;
@@ -5081,8 +5082,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
_("append not supported in this QEMU binary"));
goto cleanup;
}
- if (qemuBuildChrChardevFileStr(virQEMUCapsGet(qemuCaps,
QEMU_CAPS_CHARDEV_FILE_APPEND) ?
- logManager : NULL, cmd, def, &buf,
+ if (qemuBuildChrChardevFileStr(chardevStdioLogd ? logManager : NULL,
+ cmd, def, &buf,
"path", dev->data.file.path,
"append", dev->data.file.append) <
0)
goto cleanup;
@@ -5562,8 +5563,9 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
virQEMUDriverConfigPtr cfg,
virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
- const virDomainChrSourceDef *monitor_chr,
- bool monitor_json)
+ virDomainChrSourceDefPtr monitor_chr,
+ bool monitor_json,
+ bool chardevStdioLogd)
{
char *chrdev;
@@ -5575,7 +5577,8 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
monitor_chr, "monitor",
- qemuCaps, true)))
+ qemuCaps, true,
+ chardevStdioLogd)))
return -1;
virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, chrdev);
@@ -5720,7 +5723,8 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
const virDomainDef *def,
virDomainRNGDefPtr rng,
virQEMUCapsPtr qemuCaps,
- char **chr)
+ char **chr,
+ bool chardevStdioLogd)
{
*chr = NULL;
@@ -5733,7 +5737,8 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
case VIR_DOMAIN_RNG_BACKEND_EGD:
if (!(*chr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
rng->source.chardev,
- rng->info.alias, qemuCaps, true)))
+ rng->info.alias, qemuCaps, true,
+ chardevStdioLogd)))
return -1;
}
@@ -5881,7 +5886,8 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ bool chardevStdioLogd)
{
size_t i;
@@ -5897,7 +5903,8 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager,
/* possibly add character device for backend */
if (qemuBuildRNGBackendChrdevStr(logManager, cmd, cfg, def,
- rng, qemuCaps, &tmp) < 0)
+ rng, qemuCaps, &tmp,
+ chardevStdioLogd) < 0)
return -1;
if (tmp) {
@@ -8256,7 +8263,8 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
virDomainDefPtr def,
virDomainNetDefPtr net,
virQEMUCapsPtr qemuCaps,
- unsigned int bootindex)
+ unsigned int bootindex,
+ bool chardevStdioLogd)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
char *chardev = NULL;
@@ -8274,7 +8282,8 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
case VIR_DOMAIN_CHR_TYPE_UNIX:
if (!(chardev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
net->data.vhostuser,
- net->info.alias, qemuCaps, false)))
+ net->info.alias, qemuCaps, false,
+ chardevStdioLogd)))
goto error;
break;
@@ -8353,7 +8362,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
virNetDevVPortProfileOp vmop,
bool standalone,
size_t *nnicindexes,
- int **nicindexes)
+ int **nicindexes,
+ bool chardevStdioLogd)
{
int ret = -1;
char *nic = NULL, *host = NULL;
@@ -8466,7 +8476,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
ret = qemuBuildVhostuserCommandLine(driver, logManager, cmd, def,
- net, qemuCaps, bootindex);
+ net, qemuCaps, bootindex,
+ chardevStdioLogd);
goto cleanup;
break;
@@ -8661,7 +8672,8 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver,
bool standalone,
size_t *nnicindexes,
int **nicindexes,
- unsigned int *bootHostdevNet)
+ unsigned int *bootHostdevNet,
+ bool chardevStdioLogd)
{
size_t i;
int last_good_net = -1;
@@ -8695,7 +8707,8 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver,
if (qemuBuildInterfaceCommandLine(driver, logManager, cmd, def, net,
qemuCaps, vlan, bootNet, vmop,
standalone, nnicindexes,
- nicindexes) < 0)
+ nicindexes,
+ chardevStdioLogd) < 0)
goto error;
last_good_net = i;
@@ -8731,7 +8744,8 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ bool chardevStdioLogd)
{
size_t i;
virDomainSmartcardDefPtr smartcard;
@@ -8818,7 +8832,8 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager,
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
smartcard->data.passthru,
smartcard->info.alias,
- qemuCaps, true))) {
+ qemuCaps, true,
+ chardevStdioLogd))) {
virBufferFreeAndReset(&opt);
return -1;
}
@@ -8942,7 +8957,8 @@ qemuBuildShmemBackendChrStr(virLogManagerPtr logManager,
virQEMUDriverConfigPtr cfg,
virDomainDefPtr def,
virDomainShmemDefPtr shmem,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ bool chardevStdioLogd)
{
char *devstr = NULL;
@@ -8951,7 +8967,8 @@ qemuBuildShmemBackendChrStr(virLogManagerPtr logManager,
devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
&shmem->server.chr,
- shmem->info.alias, qemuCaps, true);
+ shmem->info.alias, qemuCaps, true,
+ chardevStdioLogd);
return devstr;
}
@@ -9007,7 +9024,8 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
virQEMUDriverConfigPtr cfg,
virDomainDefPtr def,
virDomainShmemDefPtr shmem,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ bool chardevStdioLogd)
{
char *devstr = NULL;
@@ -9065,7 +9083,8 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
if (shmem->server.enabled) {
if (!(devstr = qemuBuildShmemBackendChrStr(logManager, cmd, cfg, def,
- shmem, qemuCaps)))
+ shmem, qemuCaps,
+ chardevStdioLogd)))
return -1;
virCommandAddArgList(cmd, "-chardev", devstr, NULL);
@@ -9097,7 +9116,8 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ bool chardevStdioLogd)
{
size_t i;
bool havespice = false;
@@ -9121,7 +9141,8 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager,
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
serial->source,
serial->info.alias,
- qemuCaps, true)))
+ qemuCaps, true,
+ chardevStdioLogd)))
return -1;
virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
@@ -9147,7 +9168,8 @@ qemuBuildParallelsCommandLine(virLogManagerPtr logManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ bool chardevStdioLogd)
{
size_t i;
@@ -9160,7 +9182,8 @@ qemuBuildParallelsCommandLine(virLogManagerPtr logManager,
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
parallel->source,
parallel->info.alias,
- qemuCaps, true)))
+ qemuCaps, true,
+ chardevStdioLogd)))
return -1;
virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
@@ -9187,7 +9210,8 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ bool chardevStdioLogd)
{
size_t i;
@@ -9206,7 +9230,8 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager,
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
channel->source,
channel->info.alias,
- qemuCaps, true)))
+ qemuCaps, true,
+ chardevStdioLogd)))
return -1;
virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
@@ -9229,7 +9254,8 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager,
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
channel->source,
channel->info.alias,
- qemuCaps, true)))
+ qemuCaps, true,
+ chardevStdioLogd)))
return -1;
virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
@@ -9251,7 +9277,8 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ bool chardevStdioLogd)
{
size_t i;
@@ -9272,7 +9299,8 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager,
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
console->source,
console->info.alias,
- qemuCaps, true)))
+ qemuCaps, true,
+ chardevStdioLogd)))
return -1;
virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
@@ -9286,7 +9314,8 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager,
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
console->source,
console->info.alias,
- qemuCaps, true)))
+ qemuCaps, true,
+ chardevStdioLogd)))
return -1;
virCommandAddArg(cmd, "-chardev");
virCommandAddArg(cmd, devstr);
@@ -9404,7 +9433,8 @@ qemuBuildRedirdevCommandLine(virLogManagerPtr logManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ bool chardevStdioLogd)
{
size_t i;
@@ -9415,7 +9445,8 @@ qemuBuildRedirdevCommandLine(virLogManagerPtr logManager,
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
redirdev->source,
redirdev->info.alias,
- qemuCaps, true))) {
+ qemuCaps, true,
+ chardevStdioLogd))) {
return -1;
}
@@ -9880,7 +9911,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
virBitmapPtr nodeset,
size_t *nnicindexes,
int **nicindexes,
- const char *domainLibDir)
+ const char *domainLibDir,
+ bool chardevStdioLogd)
{
size_t i;
char uuid[VIR_UUID_STRING_BUFLEN];
@@ -9986,7 +10018,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildMonitorCommandLine(logManager, cmd, cfg, def,
qemuCaps, monitor_chr,
- monitor_json) < 0)
+ monitor_json,
+ chardevStdioLogd) < 0)
goto error;
if (qemuBuildClockCommandLine(cmd, def, qemuCaps) < 0)
@@ -10018,22 +10051,28 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildNetCommandLine(driver, logManager, cmd, def,
qemuCaps, vmop, standalone,
- nnicindexes, nicindexes, &bootHostdevNet) < 0)
+ nnicindexes, nicindexes, &bootHostdevNet,
+ chardevStdioLogd) < 0)
goto error;
- if (qemuBuildSmartcardCommandLine(logManager, cmd, cfg, def, qemuCaps) < 0)
+ if (qemuBuildSmartcardCommandLine(logManager, cmd, cfg, def, qemuCaps,
+ chardevStdioLogd) < 0)
goto error;
- if (qemuBuildSerialCommandLine(logManager, cmd, cfg, def, qemuCaps) < 0)
+ if (qemuBuildSerialCommandLine(logManager, cmd, cfg, def, qemuCaps,
+ chardevStdioLogd) < 0)
goto error;
- if (qemuBuildParallelsCommandLine(logManager, cmd, cfg, def, qemuCaps) < 0)
+ if (qemuBuildParallelsCommandLine(logManager, cmd, cfg, def, qemuCaps,
+ chardevStdioLogd) < 0)
goto error;
- if (qemuBuildChannelsCommandLine(logManager, cmd, cfg, def, qemuCaps) < 0)
+ if (qemuBuildChannelsCommandLine(logManager, cmd, cfg, def, qemuCaps,
+ chardevStdioLogd) < 0)
goto error;
- if (qemuBuildConsoleCommandLine(logManager, cmd, cfg, def, qemuCaps) < 0)
+ if (qemuBuildConsoleCommandLine(logManager, cmd, cfg, def, qemuCaps,
+ chardevStdioLogd) < 0)
goto error;
if (qemuBuildTPMCommandLine(cmd, def, qemuCaps) < 0)
@@ -10057,7 +10096,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildWatchdogCommandLine(cmd, def, qemuCaps) < 0)
goto error;
- if (qemuBuildRedirdevCommandLine(logManager, cmd, cfg, def, qemuCaps) < 0)
+ if (qemuBuildRedirdevCommandLine(logManager, cmd, cfg, def, qemuCaps,
+ chardevStdioLogd) < 0)
goto error;
if (qemuBuildHostdevCommandLine(cmd, def, qemuCaps, &bootHostdevNet) < 0)
@@ -10069,7 +10109,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
if (qemuBuildMemballoonCommandLine(cmd, def, qemuCaps) < 0)
goto error;
- if (qemuBuildRNGCommandLine(logManager, cmd, cfg, def, qemuCaps) < 0)
+ if (qemuBuildRNGCommandLine(logManager, cmd, cfg, def, qemuCaps,
+ chardevStdioLogd) < 0)
goto error;
if (qemuBuildNVRAMCommandLine(cmd, def, qemuCaps) < 0)
@@ -10106,7 +10147,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
for (i = 0; i < def->nshmems; i++) {
if (qemuBuildShmemCommandLine(logManager, cmd, cfg,
- def, def->shmems[i], qemuCaps))
+ def, def->shmems[i], qemuCaps,
+ chardevStdioLogd))
goto error;
}
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 09cb00ee9b..f5e3e5fbef 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -57,7 +57,8 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
virBitmapPtr nodeset,
size_t *nnicindexes,
int **nicindexes,
- const char *domainLibDir)
+ const char *domainLibDir,
+ bool chardevStdioLogd)
ATTRIBUTE_NONNULL(15);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 77c2e5f6d3..fbcd51c1f1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5657,7 +5657,8 @@ qemuProcessLaunch(virConnectPtr conn,
qemuCheckFips(),
priv->autoNodeset,
&nnicindexes, &nicindexes,
- priv->libDir)))
+ priv->libDir,
+ priv->chardevStdioLogd)))
goto cleanup;
if (incoming && incoming->fd != -1)
@@ -6091,7 +6092,8 @@ qemuProcessCreatePretendCmd(virConnectPtr conn,
priv->autoNodeset,
NULL,
NULL,
- priv->libDir);
+ priv->libDir,
+ priv->chardevStdioLogd);
cleanup:
return cmd;
--
2.13.0
3:12 a.m.
New subject: [libvirt] [PATCH v2 3/4] qemu: propagate chardevStdioLogd to qemuBuildChrChardevStr
Would be nice to say something about what the commit does and why here.
The title is partially self explanatory, but the devil is in the details.
On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Notes:
new in v2
This is not required to fix the issue by the last patch in the series,
however it improves the code that we decide whether to use virtlogd or
not by checking the same variable that is updated while preparing the
guest start.
src/qemu/qemu_command.c | 132 +++++++++++++++++++++++++++++++-----------------
src/qemu/qemu_command.h | 3 +-
src/qemu/qemu_process.c | 6 ++-
3 files changed, 93 insertions(+), 48 deletions(-)
I just hope there isn't some oddball migration or restart scenario that
evades my consciousness...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:45 p.m.
New subject: [libvirt] [PATCH v2 3/4] qemu: propagate chardevStdioLogd to qemuBuildChrChardevStr
On Tue, Jun 13, 2017 at 03:12:59PM -0400, John Ferlan wrote:
Would be nice to say something about what the commit does and why here.
The title is partially self explanatory, but the devil is in the details.
I'll add some commit message.
Thanks,
Pavel
10:31 p.m.
New subject: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
In the case that virtlogd is used as stdio handler we pass to QEMU
only FD to a PIPE connected to virtlogd instead of the file itself.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Notes:
new in v2
src/lxc/lxc_process.c | 6 ++---
src/qemu/qemu_security.c | 9 +++++--
src/security/security_apparmor.c | 7 ++++--
src/security/security_dac.c | 54 +++++++++++++++++++++++++++++++---------
src/security/security_driver.h | 6 +++--
src/security/security_manager.c | 12 ++++++---
src/security/security_manager.h | 6 +++--
src/security/security_nop.c | 6 +++--
src/security/security_selinux.c | 53 ++++++++++++++++++++++++++++++---------
src/security/security_stack.c | 12 ++++++---
tests/securityselinuxlabeltest.c | 2 +-
11 files changed, 127 insertions(+), 46 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index d8727c3b43..2658ea61f8 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -852,7 +852,7 @@ int virLXCProcessStop(virLXCDriverPtr driver,
}
virSecurityManagerRestoreAllLabel(driver->securityManager,
- vm->def, false);
+ vm->def, false, false);
virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
/* Clear out dynamically assigned labels */
if (vm->def->nseclabels &&
@@ -1349,7 +1349,7 @@ int virLXCProcessStart(virConnectPtr conn,
VIR_DEBUG("Setting domain security labels");
if (virSecurityManagerSetAllLabel(driver->securityManager,
- vm->def, NULL) < 0)
+ vm->def, NULL, false) < 0)
goto cleanup;
VIR_DEBUG("Setting up consoles");
@@ -1578,7 +1578,7 @@ int virLXCProcessStart(virConnectPtr conn,
virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
} else {
virSecurityManagerRestoreAllLabel(driver->securityManager,
- vm->def, false);
+ vm->def, false, false);
virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
/* Clear out dynamically assigned labels */
if (vm->def->nseclabels &&
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 61934f9905..6fc3b0bb6e 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -38,6 +38,7 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
const char *stdin_path)
{
int ret = -1;
+ qemuDomainObjPrivatePtr priv = vm->privateData;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
virSecurityManagerTransactionStart(driver->securityManager) < 0)
@@ -45,7 +46,8 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
if (virSecurityManagerSetAllLabel(driver->securityManager,
vm->def,
- stdin_path) < 0)
+ stdin_path,
+ priv->chardevStdioLogd) < 0)
goto cleanup;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
@@ -65,6 +67,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool migrated)
{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+
/* In contrast to qemuSecuritySetAllLabel, do not use
* secdriver transactions here. This function is called from
* qemuProcessStop() which is meant to do cleanup after qemu
@@ -73,7 +77,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
* in entering the namespace then. */
virSecurityManagerRestoreAllLabel(driver->securityManager,
vm->def,
- migrated);
+ migrated,
+ priv->chardevStdioLogd);
}
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 62672b0af0..5afe0c5c85 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -489,7 +489,9 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
static int
AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def, const char *stdin_path)
+ virDomainDefPtr def,
+ const char *stdin_path,
+ bool chardevStdioLogd ATTRIBUTE_UNUSED)
{
virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
SECURITY_APPARMOR_NAME);
@@ -567,7 +569,8 @@ AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
static int
AppArmorRestoreSecurityAllLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr def,
- bool migrated ATTRIBUTE_UNUSED)
+ bool migrated ATTRIBUTE_UNUSED,
+ bool chardevStdioLogd ATTRIBUTE_UNUSED)
{
int rc = 0;
virSecurityLabelDefPtr secdef =
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index fd4d8f5047..79941f480a 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1159,7 +1159,8 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrSourceDefPtr dev_source)
+ virDomainChrSourceDefPtr dev_source,
+ bool chardevStdioLogd)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
@@ -1178,6 +1179,9 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && chardevStdioLogd)
+ return 0;
+
if (chr_seclabel && chr_seclabel->label) {
if (virParseOwnershipIds(chr_seclabel->label, &user, &group) < 0)
return -1;
@@ -1243,7 +1247,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def ATTRIBUTE_UNUSED,
- virDomainChrSourceDefPtr dev_source)
+ virDomainChrSourceDefPtr dev_source,
+ bool chardevStdioLogd)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityDeviceLabelDefPtr chr_seclabel = NULL;
@@ -1256,6 +1261,9 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && chardevStdioLogd)
+ return 0;
+
switch ((virDomainChrType) dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -1298,14 +1306,21 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
}
+struct _virSecuritySELinuxChardevCallbackData {
+ virSecurityManagerPtr mgr;
+ bool chardevStdioLogd;
+};
+
+
static int
virSecurityDACRestoreChardevCallback(virDomainDefPtr def,
virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
- virSecurityManagerPtr mgr = opaque;
+ struct _virSecuritySELinuxChardevCallbackData *data = opaque;
- return virSecurityDACRestoreChardevLabel(mgr, def, dev->source);
+ return virSecurityDACRestoreChardevLabel(data->mgr, def, dev->source,
+ data->chardevStdioLogd);
}
@@ -1319,7 +1334,8 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
ret = virSecurityDACSetChardevLabel(mgr, def,
- &tpm->data.passthrough.source);
+ &tpm->data.passthrough.source,
+ false);
break;
case VIR_DOMAIN_TPM_TYPE_LAST:
break;
@@ -1339,7 +1355,8 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
ret = virSecurityDACRestoreChardevLabel(mgr, def,
- &tpm->data.passthrough.source);
+ &tpm->data.passthrough.source,
+ false);
break;
case VIR_DOMAIN_TPM_TYPE_LAST:
break;
@@ -1436,7 +1453,8 @@ virSecurityDACRestoreMemoryLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- bool migrated)
+ bool migrated,
+ bool chardevStdioLogd)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityLabelDefPtr secdef;
@@ -1479,10 +1497,15 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
rc = -1;
}
+ struct _virSecuritySELinuxChardevCallbackData chardevData = {
+ .mgr = mgr,
+ .chardevStdioLogd = chardevStdioLogd,
+ };
+
if (virDomainChrDefForeach(def,
false,
virSecurityDACRestoreChardevCallback,
- mgr) < 0)
+ &chardevData) < 0)
rc = -1;
if (def->tpm) {
@@ -1505,9 +1528,10 @@ virSecurityDACSetChardevCallback(virDomainDefPtr def,
virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
- virSecurityManagerPtr mgr = opaque;
+ struct _virSecuritySELinuxChardevCallbackData *data = opaque;
- return virSecurityDACSetChardevLabel(mgr, def, dev->source);
+ return virSecurityDACSetChardevLabel(data->mgr, def, dev->source,
+ data->chardevStdioLogd);
}
@@ -1549,7 +1573,8 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- const char *stdin_path ATTRIBUTE_UNUSED)
+ const char *stdin_path ATTRIBUTE_UNUSED,
+ bool chardevStdioLogd)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityLabelDefPtr secdef;
@@ -1592,10 +1617,15 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
return -1;
}
+ struct _virSecuritySELinuxChardevCallbackData chardevData = {
+ .mgr = mgr,
+ .chardevStdioLogd = chardevStdioLogd,
+ };
+
if (virDomainChrDefForeach(def,
true,
virSecurityDACSetChardevCallback,
- mgr) < 0)
+ &chardevData) < 0)
return -1;
if (def->tpm) {
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 0f5cce5f8d..0b3b452486 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -91,10 +91,12 @@ typedef int (*virSecurityDomainReleaseLabel) (virSecurityManagerPtr
mgr,
virDomainDefPtr sec);
typedef int (*virSecurityDomainSetAllLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr sec,
- const char *stdin_path);
+ const char *stdin_path,
+ bool chardevStdioLogd);
typedef int (*virSecurityDomainRestoreAllLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
- bool migrated);
+ bool migrated,
+ bool chardevStdioLogd);
typedef int (*virSecurityDomainGetProcessLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
pid_t pid,
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 90d491c1bc..013bbc37ef 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -856,12 +856,14 @@ int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr,
int
virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- const char *stdin_path)
+ const char *stdin_path,
+ bool chardevStdioLogd)
{
if (mgr->drv->domainSetSecurityAllLabel) {
int ret;
virObjectLock(mgr);
- ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path);
+ ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path,
+ chardevStdioLogd);
virObjectUnlock(mgr);
return ret;
}
@@ -874,12 +876,14 @@ virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr,
int
virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- bool migrated)
+ bool migrated,
+ bool chardevStdioLogd)
{
if (mgr->drv->domainRestoreSecurityAllLabel) {
int ret;
virObjectLock(mgr);
- ret = mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated);
+ ret = mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated,
+ chardevStdioLogd);
virObjectUnlock(mgr);
return ret;
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 238e66cd0b..01296d339e 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -130,10 +130,12 @@ int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr sec);
int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr sec,
- const char *stdin_path);
+ const char *stdin_path,
+ bool chardevStdioLogd);
int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- bool migrated);
+ bool migrated,
+ bool chardevStdioLogd);
int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
pid_t pid,
diff --git a/src/security/security_nop.c b/src/security/security_nop.c
index 0a9b515288..527be11e5a 100644
--- a/src/security/security_nop.c
+++ b/src/security/security_nop.c
@@ -151,7 +151,8 @@ virSecurityDomainReleaseLabelNop(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
static int
virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr sec ATTRIBUTE_UNUSED,
- const char *stdin_path ATTRIBUTE_UNUSED)
+ const char *stdin_path ATTRIBUTE_UNUSED,
+ bool chardevStdioLogd ATTRIBUTE_UNUSED)
{
return 0;
}
@@ -159,7 +160,8 @@ virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
static int
virSecurityDomainRestoreAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr vm ATTRIBUTE_UNUSED,
- bool migrated ATTRIBUTE_UNUSED)
+ bool migrated ATTRIBUTE_UNUSED,
+ bool chardevStdioLogd ATTRIBUTE_UNUSED)
{
return 0;
}
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 75f387b3fa..26137f6d8d 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2179,7 +2179,8 @@ virSecuritySELinuxRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrSourceDefPtr dev_source)
+ virDomainChrSourceDefPtr dev_source,
+ bool chardevStdioLogd)
{
virSecurityLabelDefPtr seclabel;
@@ -2198,6 +2199,9 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && chardevStdioLogd)
+ return 0;
+
if (chr_seclabel)
imagelabel = chr_seclabel->label;
if (!imagelabel)
@@ -2252,7 +2256,8 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrSourceDefPtr dev_source)
+ virDomainChrSourceDefPtr dev_source,
+ bool chardevStdioLogd)
{
virSecurityLabelDefPtr seclabel;
@@ -2269,6 +2274,9 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && chardevStdioLogd)
+ return 0;
+
switch (dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -2312,14 +2320,21 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
}
+struct _virSecuritySELinuxChardevCallbackData {
+ virSecurityManagerPtr mgr;
+ bool chardevStdioLogd;
+};
+
+
static int
virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def,
virDomainChrDefPtr dev
ATTRIBUTE_UNUSED,
void *opaque)
{
- virSecurityManagerPtr mgr = opaque;
+ struct _virSecuritySELinuxChardevCallbackData *data = opaque;
- return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->source);
+ return virSecuritySELinuxRestoreChardevLabel(data->mgr, def, dev->source,
+ data->chardevStdioLogd);
}
@@ -2342,7 +2357,8 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr
def,
return virSecuritySELinuxRestoreFileLabel(mgr, database);
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
- return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->data.passthru);
+ return virSecuritySELinuxRestoreChardevLabel(mgr, def,
+ dev->data.passthru, false);
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2369,7 +2385,8 @@ virSecuritySELinuxGetBaseLabel(virSecurityManagerPtr mgr, int
virtType)
static int
virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- bool migrated)
+ bool migrated,
+ bool chardevStdioLogd)
{
virSecurityLabelDefPtr secdef;
virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr);
@@ -2414,10 +2431,15 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr,
rc = -1;
}
+ struct _virSecuritySELinuxChardevCallbackData chardevData = {
+ .mgr = mgr,
+ .chardevStdioLogd = chardevStdioLogd
+ };
+
if (virDomainChrDefForeach(def,
false,
virSecuritySELinuxRestoreSecurityChardevCallback,
- mgr) < 0)
+ &chardevData) < 0)
rc = -1;
if (virDomainSmartcardDefForeach(def,
@@ -2706,9 +2728,10 @@ virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def,
virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
- virSecurityManagerPtr mgr = opaque;
+ struct _virSecuritySELinuxChardevCallbackData *data = opaque;
- return virSecuritySELinuxSetChardevLabel(mgr, def, dev->source);
+ return virSecuritySELinuxSetChardevLabel(data->mgr, def, dev->source,
+ data->chardevStdioLogd);
}
@@ -2733,7 +2756,7 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def,
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
return virSecuritySELinuxSetChardevLabel(mgr, def,
- dev->data.passthru);
+ dev->data.passthru, false);
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2749,7 +2772,8 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def,
static int
virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- const char *stdin_path)
+ const char *stdin_path,
+ bool chardevStdioLogd)
{
size_t i;
virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr);
@@ -2797,10 +2821,15 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,
return -1;
}
+ struct _virSecuritySELinuxChardevCallbackData chardevData = {
+ .mgr = mgr,
+ .chardevStdioLogd = chardevStdioLogd
+ };
+
if (virDomainChrDefForeach(def,
true,
virSecuritySELinuxSetSecurityChardevCallback,
- mgr) < 0)
+ &chardevData) < 0)
return -1;
if (virDomainSmartcardDefForeach(def,
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 9a1a7b30c5..53eee1692f 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -350,14 +350,16 @@ virSecurityStackRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecurityStackSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- const char *stdin_path)
+ const char *stdin_path,
+ bool chardevStdioLogd)
{
virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItemPtr item = priv->itemsHead;
int rc = 0;
for (; item; item = item->next) {
- if (virSecurityManagerSetAllLabel(item->securityManager, vm, stdin_path) <
0)
+ if (virSecurityManagerSetAllLabel(item->securityManager, vm,
+ stdin_path, chardevStdioLogd) < 0)
rc = -1;
}
@@ -368,14 +370,16 @@ virSecurityStackSetAllLabel(virSecurityManagerPtr mgr,
static int
virSecurityStackRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- bool migrated)
+ bool migrated,
+ bool chardevStdioLogd)
{
virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItemPtr item = priv->itemsHead;
int rc = 0;
for (; item; item = item->next) {
- if (virSecurityManagerRestoreAllLabel(item->securityManager, vm, migrated)
< 0)
+ if (virSecurityManagerRestoreAllLabel(item->securityManager, vm,
+ migrated, chardevStdioLogd) < 0)
rc = -1;
}
diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index 3e134991f2..ddcc954429 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -313,7 +313,7 @@ testSELinuxLabeling(const void *opaque)
if (!(def = testSELinuxLoadDef(testname)))
goto cleanup;
- if (virSecurityManagerSetAllLabel(mgr, def, NULL) < 0)
+ if (virSecurityManagerSetAllLabel(mgr, def, NULL, false) < 0)
goto cleanup;
if (testSELinuxCheckLabels(files, nfiles) < 0)
--
2.13.0
8 a.m.
New subject: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
In the case that virtlogd is used as stdio handler we pass to QEMU
only FD to a PIPE connected to virtlogd instead of the file itself.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Notes:
new in v2
src/lxc/lxc_process.c | 6 ++---
src/qemu/qemu_security.c | 9 +++++--
src/security/security_apparmor.c | 7 ++++--
src/security/security_dac.c | 54 +++++++++++++++++++++++++++++++---------
src/security/security_driver.h | 6 +++--
src/security/security_manager.c | 12 ++++++---
src/security/security_manager.h | 6 +++--
src/security/security_nop.c | 6 +++--
src/security/security_selinux.c | 53 ++++++++++++++++++++++++++++++---------
src/security/security_stack.c | 12 ++++++---
tests/securityselinuxlabeltest.c | 2 +-
11 files changed, 127 insertions(+), 46 deletions(-)
Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why
is (!chr_seclabel) even matter?
IIUC, whether or not someone set the label for the chardev, for this
particular issue/config where the chardev has a <log file=$path>, we
don't want to set (or restore) the label. I feel like I'm missing
something subtle. Maybe a bit more explanation of the adjustment would
help me...
Wouldn't these changes end up selecting "any" chardev if
chardevStdioLogd ended up being set regardless of whether they were
actually using the log file?
As an aside, I think there's an "oddity" when it comes to the Restore,
but I'm not sure how to put it into words exactly. If a guest is running
code prior to this set of changes, would it have successfully run a Set?
If so, then after applying this change and restarting, the label
wouldn't be reset, right? What happens at guest shutdown - does the
label not get unset now? Of course this is all "interaction" with
virtlogd restart that really throws a monkey wrench into things.
Also, why is the Smartcard chardev handling not using this
John
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index d8727c3b43..2658ea61f8 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -852,7 +852,7 @@ int virLXCProcessStop(virLXCDriverPtr driver,
}
virSecurityManagerRestoreAllLabel(driver->securityManager,
- vm->def, false);
+ vm->def, false, false);
virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
/* Clear out dynamically assigned labels */
if (vm->def->nseclabels &&
@@ -1349,7 +1349,7 @@ int virLXCProcessStart(virConnectPtr conn,
VIR_DEBUG("Setting domain security labels");
if (virSecurityManagerSetAllLabel(driver->securityManager,
- vm->def, NULL) < 0)
+ vm->def, NULL, false) < 0)
goto cleanup;
VIR_DEBUG("Setting up consoles");
@@ -1578,7 +1578,7 @@ int virLXCProcessStart(virConnectPtr conn,
virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
} else {
virSecurityManagerRestoreAllLabel(driver->securityManager,
- vm->def, false);
+ vm->def, false, false);
virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
/* Clear out dynamically assigned labels */
if (vm->def->nseclabels &&
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 61934f9905..6fc3b0bb6e 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -38,6 +38,7 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
const char *stdin_path)
{
int ret = -1;
+ qemuDomainObjPrivatePtr priv = vm->privateData;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
virSecurityManagerTransactionStart(driver->securityManager) < 0)
@@ -45,7 +46,8 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
if (virSecurityManagerSetAllLabel(driver->securityManager,
vm->def,
- stdin_path) < 0)
+ stdin_path,
+ priv->chardevStdioLogd) < 0)
goto cleanup;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
@@ -65,6 +67,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
bool migrated)
{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+
/* In contrast to qemuSecuritySetAllLabel, do not use
* secdriver transactions here. This function is called from
* qemuProcessStop() which is meant to do cleanup after qemu
@@ -73,7 +77,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
* in entering the namespace then. */
virSecurityManagerRestoreAllLabel(driver->securityManager,
vm->def,
- migrated);
+ migrated,
+ priv->chardevStdioLogd);
}
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 62672b0af0..5afe0c5c85 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -489,7 +489,9 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
static int
AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def, const char *stdin_path)
+ virDomainDefPtr def,
+ const char *stdin_path,
+ bool chardevStdioLogd ATTRIBUTE_UNUSED)
{
virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
SECURITY_APPARMOR_NAME);
@@ -567,7 +569,8 @@ AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
static int
AppArmorRestoreSecurityAllLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr def,
- bool migrated ATTRIBUTE_UNUSED)
+ bool migrated ATTRIBUTE_UNUSED,
+ bool chardevStdioLogd ATTRIBUTE_UNUSED)
{
int rc = 0;
virSecurityLabelDefPtr secdef =
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index fd4d8f5047..79941f480a 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1159,7 +1159,8 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrSourceDefPtr dev_source)
+ virDomainChrSourceDefPtr dev_source,
+ bool chardevStdioLogd)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
@@ -1178,6 +1179,9 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && chardevStdioLogd)
+ return 0;
+
if (chr_seclabel && chr_seclabel->label) {
if (virParseOwnershipIds(chr_seclabel->label, &user, &group) < 0)
return -1;
@@ -1243,7 +1247,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def ATTRIBUTE_UNUSED,
- virDomainChrSourceDefPtr dev_source)
+ virDomainChrSourceDefPtr dev_source,
+ bool chardevStdioLogd)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityDeviceLabelDefPtr chr_seclabel = NULL;
@@ -1256,6 +1261,9 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && chardevStdioLogd)
+ return 0;
+
switch ((virDomainChrType) dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -1298,14 +1306,21 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
}
+struct _virSecuritySELinuxChardevCallbackData {
+ virSecurityManagerPtr mgr;
+ bool chardevStdioLogd;
+};
+
+
static int
virSecurityDACRestoreChardevCallback(virDomainDefPtr def,
virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
- virSecurityManagerPtr mgr = opaque;
+ struct _virSecuritySELinuxChardevCallbackData *data = opaque;
- return virSecurityDACRestoreChardevLabel(mgr, def, dev->source);
+ return virSecurityDACRestoreChardevLabel(data->mgr, def, dev->source,
+ data->chardevStdioLogd);
}
@@ -1319,7 +1334,8 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
ret = virSecurityDACSetChardevLabel(mgr, def,
- &tpm->data.passthrough.source);
+ &tpm->data.passthrough.source,
+ false);
break;
case VIR_DOMAIN_TPM_TYPE_LAST:
break;
@@ -1339,7 +1355,8 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
ret = virSecurityDACRestoreChardevLabel(mgr, def,
- &tpm->data.passthrough.source);
+ &tpm->data.passthrough.source,
+ false);
break;
case VIR_DOMAIN_TPM_TYPE_LAST:
break;
@@ -1436,7 +1453,8 @@ virSecurityDACRestoreMemoryLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- bool migrated)
+ bool migrated,
+ bool chardevStdioLogd)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityLabelDefPtr secdef;
@@ -1479,10 +1497,15 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
rc = -1;
}
+ struct _virSecuritySELinuxChardevCallbackData chardevData = {
+ .mgr = mgr,
+ .chardevStdioLogd = chardevStdioLogd,
+ };
+
if (virDomainChrDefForeach(def,
false,
virSecurityDACRestoreChardevCallback,
- mgr) < 0)
+ &chardevData) < 0)
rc = -1;
if (def->tpm) {
@@ -1505,9 +1528,10 @@ virSecurityDACSetChardevCallback(virDomainDefPtr def,
virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
- virSecurityManagerPtr mgr = opaque;
+ struct _virSecuritySELinuxChardevCallbackData *data = opaque;
- return virSecurityDACSetChardevLabel(mgr, def, dev->source);
+ return virSecurityDACSetChardevLabel(data->mgr, def, dev->source,
+ data->chardevStdioLogd);
}
@@ -1549,7 +1573,8 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- const char *stdin_path ATTRIBUTE_UNUSED)
+ const char *stdin_path ATTRIBUTE_UNUSED,
+ bool chardevStdioLogd)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityLabelDefPtr secdef;
@@ -1592,10 +1617,15 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
return -1;
}
+ struct _virSecuritySELinuxChardevCallbackData chardevData = {
+ .mgr = mgr,
+ .chardevStdioLogd = chardevStdioLogd,
+ };
+
if (virDomainChrDefForeach(def,
true,
virSecurityDACSetChardevCallback,
- mgr) < 0)
+ &chardevData) < 0)
return -1;
if (def->tpm) {
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 0f5cce5f8d..0b3b452486 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -91,10 +91,12 @@ typedef int (*virSecurityDomainReleaseLabel) (virSecurityManagerPtr
mgr,
virDomainDefPtr sec);
typedef int (*virSecurityDomainSetAllLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr sec,
- const char *stdin_path);
+ const char *stdin_path,
+ bool chardevStdioLogd);
typedef int (*virSecurityDomainRestoreAllLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
- bool migrated);
+ bool migrated,
+ bool chardevStdioLogd);
typedef int (*virSecurityDomainGetProcessLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
pid_t pid,
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 90d491c1bc..013bbc37ef 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -856,12 +856,14 @@ int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr,
int
virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- const char *stdin_path)
+ const char *stdin_path,
+ bool chardevStdioLogd)
{
if (mgr->drv->domainSetSecurityAllLabel) {
int ret;
virObjectLock(mgr);
- ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path);
+ ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path,
+ chardevStdioLogd);
virObjectUnlock(mgr);
return ret;
}
@@ -874,12 +876,14 @@ virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr,
int
virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- bool migrated)
+ bool migrated,
+ bool chardevStdioLogd)
{
if (mgr->drv->domainRestoreSecurityAllLabel) {
int ret;
virObjectLock(mgr);
- ret = mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated);
+ ret = mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated,
+ chardevStdioLogd);
virObjectUnlock(mgr);
return ret;
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 238e66cd0b..01296d339e 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -130,10 +130,12 @@ int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr sec);
int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr sec,
- const char *stdin_path);
+ const char *stdin_path,
+ bool chardevStdioLogd);
int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- bool migrated);
+ bool migrated,
+ bool chardevStdioLogd);
int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
pid_t pid,
diff --git a/src/security/security_nop.c b/src/security/security_nop.c
index 0a9b515288..527be11e5a 100644
--- a/src/security/security_nop.c
+++ b/src/security/security_nop.c
@@ -151,7 +151,8 @@ virSecurityDomainReleaseLabelNop(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
static int
virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr sec ATTRIBUTE_UNUSED,
- const char *stdin_path ATTRIBUTE_UNUSED)
+ const char *stdin_path ATTRIBUTE_UNUSED,
+ bool chardevStdioLogd ATTRIBUTE_UNUSED)
{
return 0;
}
@@ -159,7 +160,8 @@ virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
static int
virSecurityDomainRestoreAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr vm ATTRIBUTE_UNUSED,
- bool migrated ATTRIBUTE_UNUSED)
+ bool migrated ATTRIBUTE_UNUSED,
+ bool chardevStdioLogd ATTRIBUTE_UNUSED)
{
return 0;
}
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 75f387b3fa..26137f6d8d 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2179,7 +2179,8 @@ virSecuritySELinuxRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrSourceDefPtr dev_source)
+ virDomainChrSourceDefPtr dev_source,
+ bool chardevStdioLogd)
{
virSecurityLabelDefPtr seclabel;
@@ -2198,6 +2199,9 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && chardevStdioLogd)
+ return 0;
+
if (chr_seclabel)
imagelabel = chr_seclabel->label;
if (!imagelabel)
@@ -2252,7 +2256,8 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virDomainChrSourceDefPtr dev_source)
+ virDomainChrSourceDefPtr dev_source,
+ bool chardevStdioLogd)
{
virSecurityLabelDefPtr seclabel;
@@ -2269,6 +2274,9 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
if (chr_seclabel && !chr_seclabel->relabel)
return 0;
+ if (!chr_seclabel && chardevStdioLogd)
+ return 0;
+
switch (dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -2312,14 +2320,21 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
}
+struct _virSecuritySELinuxChardevCallbackData {
+ virSecurityManagerPtr mgr;
+ bool chardevStdioLogd;
+};
+
+
static int
virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def,
virDomainChrDefPtr dev
ATTRIBUTE_UNUSED,
void *opaque)
{
- virSecurityManagerPtr mgr = opaque;
+ struct _virSecuritySELinuxChardevCallbackData *data = opaque;
- return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->source);
+ return virSecuritySELinuxRestoreChardevLabel(data->mgr, def, dev->source,
+ data->chardevStdioLogd);
}
@@ -2342,7 +2357,8 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr
def,
return virSecuritySELinuxRestoreFileLabel(mgr, database);
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
- return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->data.passthru);
+ return virSecuritySELinuxRestoreChardevLabel(mgr, def,
+ dev->data.passthru, false);
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2369,7 +2385,8 @@ virSecuritySELinuxGetBaseLabel(virSecurityManagerPtr mgr, int
virtType)
static int
virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- bool migrated)
+ bool migrated,
+ bool chardevStdioLogd)
{
virSecurityLabelDefPtr secdef;
virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr);
@@ -2414,10 +2431,15 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr,
rc = -1;
}
+ struct _virSecuritySELinuxChardevCallbackData chardevData = {
+ .mgr = mgr,
+ .chardevStdioLogd = chardevStdioLogd
+ };
+
if (virDomainChrDefForeach(def,
false,
virSecuritySELinuxRestoreSecurityChardevCallback,
- mgr) < 0)
+ &chardevData) < 0)
rc = -1;
if (virDomainSmartcardDefForeach(def,
@@ -2706,9 +2728,10 @@ virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def,
virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
void *opaque)
{
- virSecurityManagerPtr mgr = opaque;
+ struct _virSecuritySELinuxChardevCallbackData *data = opaque;
- return virSecuritySELinuxSetChardevLabel(mgr, def, dev->source);
+ return virSecuritySELinuxSetChardevLabel(data->mgr, def, dev->source,
+ data->chardevStdioLogd);
}
@@ -2733,7 +2756,7 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr
def,
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
return virSecuritySELinuxSetChardevLabel(mgr, def,
- dev->data.passthru);
+ dev->data.passthru, false);
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2749,7 +2772,8 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr
def,
static int
virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- const char *stdin_path)
+ const char *stdin_path,
+ bool chardevStdioLogd)
{
size_t i;
virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr);
@@ -2797,10 +2821,15 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,
return -1;
}
+ struct _virSecuritySELinuxChardevCallbackData chardevData = {
+ .mgr = mgr,
+ .chardevStdioLogd = chardevStdioLogd
+ };
+
if (virDomainChrDefForeach(def,
true,
virSecuritySELinuxSetSecurityChardevCallback,
- mgr) < 0)
+ &chardevData) < 0)
return -1;
if (virDomainSmartcardDefForeach(def,
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 9a1a7b30c5..53eee1692f 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -350,14 +350,16 @@ virSecurityStackRestoreHostdevLabel(virSecurityManagerPtr mgr,
static int
virSecurityStackSetAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- const char *stdin_path)
+ const char *stdin_path,
+ bool chardevStdioLogd)
{
virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItemPtr item = priv->itemsHead;
int rc = 0;
for (; item; item = item->next) {
- if (virSecurityManagerSetAllLabel(item->securityManager, vm, stdin_path) <
0)
+ if (virSecurityManagerSetAllLabel(item->securityManager, vm,
+ stdin_path, chardevStdioLogd) < 0)
rc = -1;
}
@@ -368,14 +370,16 @@ virSecurityStackSetAllLabel(virSecurityManagerPtr mgr,
static int
virSecurityStackRestoreAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- bool migrated)
+ bool migrated,
+ bool chardevStdioLogd)
{
virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItemPtr item = priv->itemsHead;
int rc = 0;
for (; item; item = item->next) {
- if (virSecurityManagerRestoreAllLabel(item->securityManager, vm, migrated)
< 0)
+ if (virSecurityManagerRestoreAllLabel(item->securityManager, vm,
+ migrated, chardevStdioLogd) < 0)
rc = -1;
}
diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index 3e134991f2..ddcc954429 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -313,7 +313,7 @@ testSELinuxLabeling(const void *opaque)
if (!(def = testSELinuxLoadDef(testname)))
goto cleanup;
- if (virSecurityManagerSetAllLabel(mgr, def, NULL) < 0)
+ if (virSecurityManagerSetAllLabel(mgr, def, NULL, false) < 0)
goto cleanup;
if (testSELinuxCheckLabels(files, nfiles) < 0)
3:11 p.m.
New subject: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote:
On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
> In the case that virtlogd is used as stdio handler we pass to QEMU
> only FD to a PIPE connected to virtlogd instead of the file itself.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
>
> Notes:
> new in v2
>
> src/lxc/lxc_process.c | 6 ++---
> src/qemu/qemu_security.c | 9 +++++--
> src/security/security_apparmor.c | 7 ++++--
> src/security/security_dac.c | 54 +++++++++++++++++++++++++++++++---------
> src/security/security_driver.h | 6 +++--
> src/security/security_manager.c | 12 ++++++---
> src/security/security_manager.h | 6 +++--
> src/security/security_nop.c | 6 +++--
> src/security/security_selinux.c | 53 ++++++++++++++++++++++++++++++---------
> src/security/security_stack.c | 12 ++++++---
> tests/securityselinuxlabeltest.c | 2 +-
> 11 files changed, 127 insertions(+), 46 deletions(-)
>
Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why
is (!chr_seclabel) even matter?
If you configure the label we shouldn't ignore it in some cases, that's
just wrong. If the label is set for the char device we will configure
it every time even if it will fail to start the guest, it's a
responsibility of the user to configure proper label if it is provided.
IIUC, whether or not someone set the label for the chardev, for this
particular issue/config where the chardev has a <log file=$path>, we
don't want to set (or restore) the label. I feel like I'm missing
something subtle. Maybe a bit more explanation of the adjustment would
help me...
This is not for the <log file=$path/> but for the <source path=$path/>.
We don't relabel $path for <log file=$path/> at all.
Wouldn't these changes end up selecting "any" chardev
if
chardevStdioLogd ended up being set regardless of whether they were
actually using the log file?
I don't know what you mean by this sentence?
As an aside, I think there's an "oddity" when it comes
to the Restore,
but I'm not sure how to put it into words exactly. If a guest is running
code prior to this set of changes, would it have successfully run a Set?
If so, then after applying this change and restarting, the label
wouldn't be reset, right? What happens at guest shutdown - does the
label not get unset now? Of course this is all "interaction" with
virtlogd restart that really throws a monkey wrench into things.
No, that's not correct. The @chardevStdioLogd is stored in the status
XML (the one stored in /var/run/libvirt/qemu/$domain_name.xml). So when
the libvirtd is stopped and started with this patch applied the status
XML doesn't have the @chardevStdioLogd stored in it so it will be false
and we will reset the label. The @chardevStdioLogd is updated only when
the domain is started and we will store the value in the status XML
only with new libvirtd and only in that case we will not set/restore
the label.
Also, why is the Smartcard chardev handling not using this
The smartcard doesn't ever use virtlogd as stdio handler.
Pavel
7:57 p.m.
New subject: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On 06/15/2017 03:11 AM, Pavel Hrdina wrote:
On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote:
>
>
> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
>> In the case that virtlogd is used as stdio handler we pass to QEMU
>> only FD to a PIPE connected to virtlogd instead of the file itself.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988
>>
>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>> ---
>>
>> Notes:
>> new in v2
>>
>> src/lxc/lxc_process.c | 6 ++---
>> src/qemu/qemu_security.c | 9 +++++--
>> src/security/security_apparmor.c | 7 ++++--
>> src/security/security_dac.c | 54 +++++++++++++++++++++++++++++++---------
>> src/security/security_driver.h | 6 +++--
>> src/security/security_manager.c | 12 ++++++---
>> src/security/security_manager.h | 6 +++--
>> src/security/security_nop.c | 6 +++--
>> src/security/security_selinux.c | 53 ++++++++++++++++++++++++++++++---------
>> src/security/security_stack.c | 12 ++++++---
>> tests/securityselinuxlabeltest.c | 2 +-
>> 11 files changed, 127 insertions(+), 46 deletions(-)
>>
>
> Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why
> is (!chr_seclabel) even matter?
If you configure the label we shouldn't ignore it in some cases, that's
just wrong. If the label is set for the char device we will configure
it every time even if it will fail to start the guest, it's a
responsibility of the user to configure proper label if it is provided.
When email doesn't convey the question... Ugh... I'm also trying to
speed learn an area of the code and review at the same time.
If I go back to commit id 'f8b08d0e' where the original implementation
to add labels for chardevs was done (but has been modified for patch 1
to change where the label is stored), I get the impression that a label
should be added either from something specifically supplied for the
<chardev> or to use the per domain one:
"The source element may contain an optional seclabel to override the way
that labelling is done on the socket path. If this element is not
present, the security label is inherited from the per-domain setting."
So if I look at the condition "(!chr_seclabel && chardevStdioLogd)"
added by this patch to decide whether or not to supply the label, I'm
left with the impression that if for this particular chardev a label
doesn't exist *and* the configuration option chardevStdioLogd is true,
then we're going to return happy status *and* not inherit the per-domain
setting.
So the bug is then that applying the default domain label for a chardev
configured to use a stdio handle is incorrect? Perhaps I didn't get
that from reading bz or the patch.
> IIUC, whether or not someone set the label for the chardev, for
this
> particular issue/config where the chardev has a <log file=$path>, we
> don't want to set (or restore) the label. I feel like I'm missing
> something subtle. Maybe a bit more explanation of the adjustment would
> help me...
This is not for the <log file=$path/> but for the <source path=$path/>.
We don't relabel $path for <log file=$path/> at all.
hmm.. ah, right... I kept scrolling back and forth in the bz and the
docs, but missed this in the bz:
3) Check the virtlogd.log:
error : virRotatingFileWriterEntryNew:113 : Unable to open file:
/var/log/libvirt/qemu/log: Permission denied
I guess I got lost in the power of suggestion of reading the docs
regarding the "optional log file" that can be associated paragraph and
trying to learn on the fly so that you at least get a review in a
somewhat timely manner ;-)
> Wouldn't these changes end up selecting "any"
chardev if
> chardevStdioLogd ended up being set regardless of whether they were
> actually using the log file?
I don't know what you mean by this sentence?
Well let's see, chardevStdioLogd is set to true when meeting the two
conditions a qemu.conf global "cfg->stdioLogd" && a per domain or
emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND".
So, conceivably chardevStdioLogd could be true for *any* domain as long
as those conditions are met, right?
If you have a domain that has chardev's which are not configured to use
the stdio handler, then the chardevStdioLogd could still be true, right?
If that's the case and the chardev doesn't have a label associated, then
we just return happy status and we do not inherit the per domain
setting. Wouldn't that be incorrect?
My concern is more we're making a change in a (mostly) common set of
functions for a (very) specific problem.
> As an aside, I think there's an "oddity" when it
comes to the Restore,
> but I'm not sure how to put it into words exactly. If a guest is running
> code prior to this set of changes, would it have successfully run a Set?
> If so, then after applying this change and restarting, the label
> wouldn't be reset, right? What happens at guest shutdown - does the
> label not get unset now? Of course this is all "interaction" with
> virtlogd restart that really throws a monkey wrench into things.
No, that's not correct. The @chardevStdioLogd is stored in the status
XML (the one stored in /var/run/libvirt/qemu/$domain_name.xml). So when
the libvirtd is stopped and started with this patch applied the status
XML doesn't have the @chardevStdioLogd stored in it so it will be false
and we will reset the label. The @chardevStdioLogd is updated only when
the domain is started and we will store the value in the status XML
only with new libvirtd and only in that case we will not set/restore
the label.
hmmm.. Reading the bz indicates the 'virtlogd' daemon restarting... This
is where all this gets a bit "odd" for me. Like I said it was a weird
thing to even try and explain, but I think you talked me off the ledge
of concern.
John
> Also, why is the Smartcard chardev handling not using this
The smartcard doesn't ever use virtlogd as stdio handler.
Pavel
10:40 p.m.
New subject: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote:
On 06/15/2017 03:11 AM, Pavel Hrdina wrote:
> On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote:
>>
>>
>> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
>>> In the case that virtlogd is used as stdio handler we pass to QEMU
>>> only FD to a PIPE connected to virtlogd instead of the file itself.
>>>
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988
>>>
>>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>>> ---
>>>
>>> Notes:
>>> new in v2
>>>
>>> src/lxc/lxc_process.c | 6 ++---
>>> src/qemu/qemu_security.c | 9 +++++--
>>> src/security/security_apparmor.c | 7 ++++--
>>> src/security/security_dac.c | 54
+++++++++++++++++++++++++++++++---------
>>> src/security/security_driver.h | 6 +++--
>>> src/security/security_manager.c | 12 ++++++---
>>> src/security/security_manager.h | 6 +++--
>>> src/security/security_nop.c | 6 +++--
>>> src/security/security_selinux.c | 53
++++++++++++++++++++++++++++++---------
>>> src/security/security_stack.c | 12 ++++++---
>>> tests/securityselinuxlabeltest.c | 2 +-
>>> 11 files changed, 127 insertions(+), 46 deletions(-)
>>>
>>
>> Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why
>> is (!chr_seclabel) even matter?
>
> If you configure the label we shouldn't ignore it in some cases, that's
> just wrong. If the label is set for the char device we will configure
> it every time even if it will fail to start the guest, it's a
> responsibility of the user to configure proper label if it is provided.
>
When email doesn't convey the question... Ugh... I'm also trying to
speed learn an area of the code and review at the same time.
If I go back to commit id 'f8b08d0e' where the original implementation
to add labels for chardevs was done (but has been modified for patch 1
to change where the label is stored), I get the impression that a label
should be added either from something specifically supplied for the
<chardev> or to use the per domain one:
"The source element may contain an optional seclabel to override the way
that labelling is done on the socket path. If this element is not
present, the security label is inherited from the per-domain setting."
So if I look at the condition "(!chr_seclabel && chardevStdioLogd)"
added by this patch to decide whether or not to supply the label, I'm
left with the impression that if for this particular chardev a label
doesn't exist *and* the configuration option chardevStdioLogd is true,
then we're going to return happy status *and* not inherit the per-domain
setting.
So the bug is then that applying the default domain label for a chardev
configured to use a stdio handle is incorrect? Perhaps I didn't get
that from reading bz or the patch.
Yes, that's the bug. If virtlogd is used to handle stdio for char
devices we shouldn't relabel the @path to the default labels, the
@path doesn't have to be accessible by the qemu process, it has to be
accessible by the virtlogd process.
>> IIUC, whether or not someone set the label for the chardev,
for this
>> particular issue/config where the chardev has a <log file=$path>, we
>> don't want to set (or restore) the label. I feel like I'm missing
>> something subtle. Maybe a bit more explanation of the adjustment would
>> help me...
>
> This is not for the <log file=$path/> but for the <source path=$path/>.
> We don't relabel $path for <log file=$path/> at all.
>
hmm.. ah, right... I kept scrolling back and forth in the bz and the
docs, but missed this in the bz:
3) Check the virtlogd.log:
error : virRotatingFileWriterEntryNew:113 : Unable to open file:
/var/log/libvirt/qemu/log: Permission denied
I guess I got lost in the power of suggestion of reading the docs
regarding the "optional log file" that can be associated paragraph and
trying to learn on the fly so that you at least get a review in a
somewhat timely manner ;-)
>> Wouldn't these changes end up selecting "any" chardev if
>> chardevStdioLogd ended up being set regardless of whether they were
>> actually using the log file?
>
> I don't know what you mean by this sentence?
>
Well let's see, chardevStdioLogd is set to true when meeting the two
conditions a qemu.conf global "cfg->stdioLogd" && a per domain or
emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND".
So, conceivably chardevStdioLogd could be true for *any* domain as long
as those conditions are met, right?
Yes, the two conditions are checked while starting a new domain in
qemuProcessPrepareDomain() and stored in the private date of that
domain.
If you have a domain that has chardev's which are not configured
to use
the stdio handler, then the chardevStdioLogd could still be true, right?
No, if the @chardevStdioLogd is true all char devices for that domain
will use virtlogd.
The only issue I've just found out is that the code path for chardev
hot-plug isn't updated to use virtlogd when it should be so for
hot-plugged char devices we pass the path directly to QEMU.
With this patch applied the hot-plug fails if virtlogd is used because
we don't relabel the path but we pass it directly to QEMU, this needs to
be fixed to not introduce a regression, sigh.
Pavel
If that's the case and the chardev doesn't have a label
associated, then
we just return happy status and we do not inherit the per domain
setting. Wouldn't that be incorrect?
My concern is more we're making a change in a (mostly) common set of
functions for a (very) specific problem.
>> As an aside, I think there's an "oddity" when it comes to the
Restore,
>> but I'm not sure how to put it into words exactly. If a guest is running
>> code prior to this set of changes, would it have successfully run a Set?
>> If so, then after applying this change and restarting, the label
>> wouldn't be reset, right? What happens at guest shutdown - does the
>> label not get unset now? Of course this is all "interaction" with
>> virtlogd restart that really throws a monkey wrench into things.
>
> No, that's not correct. The @chardevStdioLogd is stored in the status
> XML (the one stored in /var/run/libvirt/qemu/$domain_name.xml). So when
> the libvirtd is stopped and started with this patch applied the status
> XML doesn't have the @chardevStdioLogd stored in it so it will be false
> and we will reset the label. The @chardevStdioLogd is updated only when
> the domain is started and we will store the value in the status XML
> only with new libvirtd and only in that case we will not set/restore
> the label.
>
hmmm.. Reading the bz indicates the 'virtlogd' daemon restarting... This
is where all this gets a bit "odd" for me. Like I said it was a weird
thing to even try and explain, but I think you talked me off the ledge
of concern.
John
>> Also, why is the Smartcard chardev handling not using this
>
> The smartcard doesn't ever use virtlogd as stdio handler.
>
> Pavel
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:45 p.m.
New subject: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On Thu, Jun 15, 2017 at 04:40:49PM +0200, Pavel Hrdina wrote:
On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote:
>
>
> On 06/15/2017 03:11 AM, Pavel Hrdina wrote:
> > On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote:
> >>
> >>
> >> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
> >>> In the case that virtlogd is used as stdio handler we pass to QEMU
> >>> only FD to a PIPE connected to virtlogd instead of the file itself.
> >>>
> >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988
> >>>
> >>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> >>> ---
> >>>
> >>> Notes:
> >>> new in v2
> >>>
> >>> src/lxc/lxc_process.c | 6 ++---
> >>> src/qemu/qemu_security.c | 9 +++++--
> >>> src/security/security_apparmor.c | 7 ++++--
> >>> src/security/security_dac.c | 54
+++++++++++++++++++++++++++++++---------
> >>> src/security/security_driver.h | 6 +++--
> >>> src/security/security_manager.c | 12 ++++++---
> >>> src/security/security_manager.h | 6 +++--
> >>> src/security/security_nop.c | 6 +++--
> >>> src/security/security_selinux.c | 53
++++++++++++++++++++++++++++++---------
> >>> src/security/security_stack.c | 12 ++++++---
> >>> tests/securityselinuxlabeltest.c | 2 +-
> >>> 11 files changed, 127 insertions(+), 46 deletions(-)
> >>>
> >>
> >> Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point
why
> >> is (!chr_seclabel) even matter?
> >
> > If you configure the label we shouldn't ignore it in some cases,
that's
> > just wrong. If the label is set for the char device we will configure
> > it every time even if it will fail to start the guest, it's a
> > responsibility of the user to configure proper label if it is provided.
> >
>
> When email doesn't convey the question... Ugh... I'm also trying to
> speed learn an area of the code and review at the same time.
>
> If I go back to commit id 'f8b08d0e' where the original implementation
> to add labels for chardevs was done (but has been modified for patch 1
> to change where the label is stored), I get the impression that a label
> should be added either from something specifically supplied for the
> <chardev> or to use the per domain one:
>
> "The source element may contain an optional seclabel to override the way
> that labelling is done on the socket path. If this element is not
> present, the security label is inherited from the per-domain setting."
>
> So if I look at the condition "(!chr_seclabel &&
chardevStdioLogd)"
> added by this patch to decide whether or not to supply the label, I'm
> left with the impression that if for this particular chardev a label
> doesn't exist *and* the configuration option chardevStdioLogd is true,
> then we're going to return happy status *and* not inherit the per-domain
> setting.
>
> So the bug is then that applying the default domain label for a chardev
> configured to use a stdio handle is incorrect? Perhaps I didn't get
> that from reading bz or the patch.
Yes, that's the bug. If virtlogd is used to handle stdio for char
devices we shouldn't relabel the @path to the default labels, the
@path doesn't have to be accessible by the qemu process, it has to be
accessible by the virtlogd process.
> >> IIUC, whether or not someone set the label for the chardev, for this
> >> particular issue/config where the chardev has a <log file=$path>, we
> >> don't want to set (or restore) the label. I feel like I'm missing
> >> something subtle. Maybe a bit more explanation of the adjustment would
> >> help me...
> >
> > This is not for the <log file=$path/> but for the <source
path=$path/>.
> > We don't relabel $path for <log file=$path/> at all.
> >
>
> hmm.. ah, right... I kept scrolling back and forth in the bz and the
> docs, but missed this in the bz:
>
> 3) Check the virtlogd.log:
> error : virRotatingFileWriterEntryNew:113 : Unable to open file:
> /var/log/libvirt/qemu/log: Permission denied
>
> I guess I got lost in the power of suggestion of reading the docs
> regarding the "optional log file" that can be associated paragraph and
> trying to learn on the fly so that you at least get a review in a
> somewhat timely manner ;-)
>
> >> Wouldn't these changes end up selecting "any" chardev if
> >> chardevStdioLogd ended up being set regardless of whether they were
> >> actually using the log file?
> >
> > I don't know what you mean by this sentence?
> >
>
> Well let's see, chardevStdioLogd is set to true when meeting the two
> conditions a qemu.conf global "cfg->stdioLogd" && a per domain
or
> emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND".
>
> So, conceivably chardevStdioLogd could be true for *any* domain as long
> as those conditions are met, right?
Yes, the two conditions are checked while starting a new domain in
qemuProcessPrepareDomain() and stored in the private date of that
domain.
> If you have a domain that has chardev's which are not configured to use
> the stdio handler, then the chardevStdioLogd could still be true, right?
No, if the @chardevStdioLogd is true all char devices for that domain
will use virtlogd.
The only issue I've just found out is that the code path for chardev
hot-plug isn't updated to use virtlogd when it should be so for
hot-plugged char devices we pass the path directly to QEMU.
With this patch applied the hot-plug fails if virtlogd is used because
we don't relabel the path but we pass it directly to QEMU, this needs to
be fixed to not introduce a regression, sigh.
Ok, I've tried it without this patch and it still fails with permission
denied so there is no regression, we just don't relabel the hot-plugged
chardev at all. Still it should be fixed.
Pavel
> If that's the case and the chardev doesn't have a label associated, then
> we just return happy status and we do not inherit the per domain
> setting. Wouldn't that be incorrect?
>
> My concern is more we're making a change in a (mostly) common set of
> functions for a (very) specific problem.
>
>
> >> As an aside, I think there's an "oddity" when it comes to the
Restore,
> >> but I'm not sure how to put it into words exactly. If a guest is
running
> >> code prior to this set of changes, would it have successfully run a Set?
> >> If so, then after applying this change and restarting, the label
> >> wouldn't be reset, right? What happens at guest shutdown - does the
> >> label not get unset now? Of course this is all "interaction"
with
> >> virtlogd restart that really throws a monkey wrench into things.
> >
> > No, that's not correct. The @chardevStdioLogd is stored in the status
> > XML (the one stored in /var/run/libvirt/qemu/$domain_name.xml). So when
> > the libvirtd is stopped and started with this patch applied the status
> > XML doesn't have the @chardevStdioLogd stored in it so it will be false
> > and we will reset the label. The @chardevStdioLogd is updated only when
> > the domain is started and we will store the value in the status XML
> > only with new libvirtd and only in that case we will not set/restore
> > the label.
> >
>
> hmmm.. Reading the bz indicates the 'virtlogd' daemon restarting... This
> is where all this gets a bit "odd" for me. Like I said it was a weird
> thing to even try and explain, but I think you talked me off the ledge
> of concern.
>
> John
>
> >> Also, why is the Smartcard chardev handling not using this
> >
> > The smartcard doesn't ever use virtlogd as stdio handler.
> >
> > Pavel
> >
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:12 a.m.
New subject: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On 06/15/2017 10:40 AM, Pavel Hrdina wrote:
On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote:
>
>
> On 06/15/2017 03:11 AM, Pavel Hrdina wrote:
>> On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote:
>>>
>>>
>>> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
>>>> In the case that virtlogd is used as stdio handler we pass to QEMU
>>>> only FD to a PIPE connected to virtlogd instead of the file itself.
>>>>
>>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988
>>>>
>>>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>>>> ---
>>>>
>>>> Notes:
>>>> new in v2
>>>>
>>>> src/lxc/lxc_process.c | 6 ++---
>>>> src/qemu/qemu_security.c | 9 +++++--
>>>> src/security/security_apparmor.c | 7 ++++--
>>>> src/security/security_dac.c | 54
+++++++++++++++++++++++++++++++---------
>>>> src/security/security_driver.h | 6 +++--
>>>> src/security/security_manager.c | 12 ++++++---
>>>> src/security/security_manager.h | 6 +++--
>>>> src/security/security_nop.c | 6 +++--
>>>> src/security/security_selinux.c | 53
++++++++++++++++++++++++++++++---------
>>>> src/security/security_stack.c | 12 ++++++---
>>>> tests/securityselinuxlabeltest.c | 2 +-
>>>> 11 files changed, 127 insertions(+), 46 deletions(-)
>>>>
>>>
>>> Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point
why
>>> is (!chr_seclabel) even matter?
>>
>> If you configure the label we shouldn't ignore it in some cases, that's
>> just wrong. If the label is set for the char device we will configure
>> it every time even if it will fail to start the guest, it's a
>> responsibility of the user to configure proper label if it is provided.
>>
>
> When email doesn't convey the question... Ugh... I'm also trying to
> speed learn an area of the code and review at the same time.
>
> If I go back to commit id 'f8b08d0e' where the original implementation
> to add labels for chardevs was done (but has been modified for patch 1
> to change where the label is stored), I get the impression that a label
> should be added either from something specifically supplied for the
> <chardev> or to use the per domain one:
>
> "The source element may contain an optional seclabel to override the way
> that labelling is done on the socket path. If this element is not
> present, the security label is inherited from the per-domain setting."
>
> So if I look at the condition "(!chr_seclabel &&
chardevStdioLogd)"
> added by this patch to decide whether or not to supply the label, I'm
> left with the impression that if for this particular chardev a label
> doesn't exist *and* the configuration option chardevStdioLogd is true,
> then we're going to return happy status *and* not inherit the per-domain
> setting.
>
> So the bug is then that applying the default domain label for a chardev
> configured to use a stdio handle is incorrect? Perhaps I didn't get
> that from reading bz or the patch.
Yes, that's the bug. If virtlogd is used to handle stdio for char
devices we shouldn't relabel the @path to the default labels, the
@path doesn't have to be accessible by the qemu process, it has to be
accessible by the virtlogd process.
>>> IIUC, whether or not someone set the label for the chardev, for this
>>> particular issue/config where the chardev has a <log file=$path>, we
>>> don't want to set (or restore) the label. I feel like I'm missing
>>> something subtle. Maybe a bit more explanation of the adjustment would
>>> help me...
>>
>> This is not for the <log file=$path/> but for the <source
path=$path/>.
>> We don't relabel $path for <log file=$path/> at all.
>>
>
> hmm.. ah, right... I kept scrolling back and forth in the bz and the
> docs, but missed this in the bz:
>
> 3) Check the virtlogd.log:
> error : virRotatingFileWriterEntryNew:113 : Unable to open file:
> /var/log/libvirt/qemu/log: Permission denied
>
> I guess I got lost in the power of suggestion of reading the docs
> regarding the "optional log file" that can be associated paragraph and
> trying to learn on the fly so that you at least get a review in a
> somewhat timely manner ;-)
>
>>> Wouldn't these changes end up selecting "any" chardev if
>>> chardevStdioLogd ended up being set regardless of whether they were
>>> actually using the log file?
>>
>> I don't know what you mean by this sentence?
>>
>
> Well let's see, chardevStdioLogd is set to true when meeting the two
> conditions a qemu.conf global "cfg->stdioLogd" && a per domain
or
> emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND".
>
> So, conceivably chardevStdioLogd could be true for *any* domain as long
> as those conditions are met, right?
Yes, the two conditions are checked while starting a new domain in
qemuProcessPrepareDomain() and stored in the private date of that
domain.
> If you have a domain that has chardev's which are not configured to use
> the stdio handler, then the chardevStdioLogd could still be true, right?
No, if the @chardevStdioLogd is true all char devices for that domain
will use virtlogd.
This is the part I'm stuck on as to why - based on the previous
assertion. I'm just not visualizing all those various chardev configs.
Just so you know - I have to be out Friday, but will be back Monday. If
you get someone else to ACK this one in the mean time - that's fine.
John
The only issue I've just found out is that the code path for
chardev
hot-plug isn't updated to use virtlogd when it should be so for
hot-plugged char devices we pass the path directly to QEMU.
With this patch applied the hot-plug fails if virtlogd is used because
we don't relabel the path but we pass it directly to QEMU, this needs to
be fixed to not introduce a regression, sigh.
Pavel
> If that's the case and the chardev doesn't have a label associated, then
> we just return happy status and we do not inherit the per domain
> setting. Wouldn't that be incorrect?
>
> My concern is more we're making a change in a (mostly) common set of
> functions for a (very) specific problem.
>
>
>>> As an aside, I think there's an "oddity" when it comes to the
Restore,
>>> but I'm not sure how to put it into words exactly. If a guest is running
>>> code prior to this set of changes, would it have successfully run a Set?
>>> If so, then after applying this change and restarting, the label
>>> wouldn't be reset, right? What happens at guest shutdown - does the
>>> label not get unset now? Of course this is all "interaction" with
>>> virtlogd restart that really throws a monkey wrench into things.
>>
>> No, that's not correct. The @chardevStdioLogd is stored in the status
>> XML (the one stored in /var/run/libvirt/qemu/$domain_name.xml). So when
>> the libvirtd is stopped and started with this patch applied the status
>> XML doesn't have the @chardevStdioLogd stored in it so it will be false
>> and we will reset the label. The @chardevStdioLogd is updated only when
>> the domain is started and we will store the value in the status XML
>> only with new libvirtd and only in that case we will not set/restore
>> the label.
>>
>
> hmmm.. Reading the bz indicates the 'virtlogd' daemon restarting... This
> is where all this gets a bit "odd" for me. Like I said it was a weird
> thing to even try and explain, but I think you talked me off the ledge
> of concern.
>
> John
>
>>> Also, why is the Smartcard chardev handling not using this
>>
>> The smartcard doesn't ever use virtlogd as stdio handler.
>>
>> Pavel
>>
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
4:18 p.m.
New subject: [libvirt] [PATCH v2 4/4] security: don't relabel chardev source if virtlogd is used as stdio handler
On Thu, Jun 15, 2017 at 10:12:51PM -0400, John Ferlan wrote:
On 06/15/2017 10:40 AM, Pavel Hrdina wrote:
> On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote:
>> On 06/15/2017 03:11 AM, Pavel Hrdina wrote:
>>> On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote:
[...]
>>>> Wouldn't these changes end up selecting
"any" chardev if
>>>> chardevStdioLogd ended up being set regardless of whether they were
>>>> actually using the log file?
>>>
>>> I don't know what you mean by this sentence?
>>>
>>
>> Well let's see, chardevStdioLogd is set to true when meeting the two
>> conditions a qemu.conf global "cfg->stdioLogd" && a per
domain or
>> emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND".
>>
>> So, conceivably chardevStdioLogd could be true for *any* domain as long
>> as those conditions are met, right?
>
> Yes, the two conditions are checked while starting a new domain in
> qemuProcessPrepareDomain() and stored in the private date of that
> domain.
>
>> If you have a domain that has chardev's which are not configured to use
>> the stdio handler, then the chardevStdioLogd could still be true, right?
>
> No, if the @chardevStdioLogd is true all char devices for that domain
> will use virtlogd.
>
This is the part I'm stuck on as to why - based on the previous
assertion. I'm just not visualizing all those various chardev configs.
Just so you know - I have to be out Friday, but will be back Monday. If
you get someone else to ACK this one in the mean time - that's fine.
The usage of virlogd can be only set in qemu.conf, not per-chardev, not
even per-domain. We need to save it per-domain, though, as it can
differ for domains started before and after that setting was changed in
qemu.conf and the daemon restarted (or updated to a version with support
for virtlogd as that is the default if nothing is specified in the
config).
The hotplug should be fixed, but as you said that can be a follow-up patch.
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
7:43 p.m.
On Mon, May 29, 2017 at 04:31:46PM +0200, Pavel Hrdina wrote:
Pavel Hrdina (4):
conf: move seclabel for chardev source to the correct sturcture
qemu: introduce chardevStdioLogd to qemu private data
qemu: propagate chardevStdioLogd to qemuBuildChrChardevStr
security: don't relabel chardev source if virtlogd is used as stdio
handler
Ping
2694
days inactive
2712
days old
19 comments
3 participants
participants (3)
-
John Ferlan -
Martin Kletzander -
Pavel Hrdina