[libvirt] [PATCH 0/2] fix attached vm cannot get a right label
When call qemuProcessAttach to attach a qemu process, libvirt will generate a wrong label for DAC, and do not set imagelabel for both of them, no imagelabel will cause some other issue. After this patch guest label will be : <seclabel type='static' model='selinux' relabel='yes'> <label>unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023</label> <imagelabel>system_u:object_r:svirt_image_t:s0-s0:c0.c1023</imagelabel> </seclabel> <seclabel type='static' model='dac' relabel='yes'> <label>+0:+0</label> <imagelabel>+0:+0</imagelabel> </seclabel> Luyao Huang (2): qemu: fix some small issue in qemuProcessAttach security: Add a new func use stat to get process DAC label src/qemu/qemu_process.c | 10 ++++++--- src/security/security_dac.c | 50 +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 5 deletions(-) -- 1.8.3.1
There are some small issue in qemuProcessAttach: 1.Fix virSecurityManagerGetProcessLabel always get pid = 0, move 'vm->pid = pid' before call virSecurityManagerGetProcessLabel. 2.Use virSecurityManagerGenLabel to get image label. 3.Fix always set selinux label for other security driver label. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 382d802..ee95adb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5255,6 +5255,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, if (VIR_STRDUP(priv->pidfile, pidfile) < 0) goto error; + vm->pid = pid; + VIR_DEBUG("Detect security driver config"); sec_managers = virSecurityManagerGetNested(driver->securityManager); if (sec_managers == NULL) @@ -5272,7 +5274,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, seclabeldef->type = VIR_DOMAIN_SECLABEL_STATIC; if (VIR_ALLOC(seclabel) < 0) goto error; - if (virSecurityManagerGetProcessLabel(driver->securityManager, + if (virSecurityManagerGetProcessLabel(sec_managers[i], vm->def, vm->pid, seclabel) < 0) goto error; @@ -5290,6 +5292,10 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, } } + if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0) { + goto error; + } + VIR_DEBUG("Creating domain log file"); if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0) goto error; @@ -5334,8 +5340,6 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, logfile); - vm->pid = pid; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, QEMU_ASYNC_JOB_NONE, priv->qemuCaps, -1) < 0) goto error; -- 1.8.3.1
On Mon, Dec 01, 2014 at 05:54:35PM +0800, Luyao Huang wrote:
There are some small issue in qemuProcessAttach:
1.Fix virSecurityManagerGetProcessLabel always get pid = 0, move 'vm->pid = pid' before call virSecurityManagerGetProcessLabel.
2.Use virSecurityManagerGenLabel to get image label.
3.Fix always set selinux label for other security driver label.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
It looks like we were doing everything already, but we just did it wrong. Nice catch! ACK. Martin
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 382d802..ee95adb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5255,6 +5255,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, if (VIR_STRDUP(priv->pidfile, pidfile) < 0) goto error;
+ vm->pid = pid; + VIR_DEBUG("Detect security driver config"); sec_managers = virSecurityManagerGetNested(driver->securityManager); if (sec_managers == NULL) @@ -5272,7 +5274,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, seclabeldef->type = VIR_DOMAIN_SECLABEL_STATIC; if (VIR_ALLOC(seclabel) < 0) goto error; - if (virSecurityManagerGetProcessLabel(driver->securityManager, + if (virSecurityManagerGetProcessLabel(sec_managers[i], vm->def, vm->pid, seclabel) < 0) goto error;
@@ -5290,6 +5292,10 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, } }
+ if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0) { + goto error; + } + VIR_DEBUG("Creating domain log file"); if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0) goto error; @@ -5334,8 +5340,6 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, logfile);
- vm->pid = pid; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, QEMU_ASYNC_JOB_NONE, priv->qemuCaps, -1) < 0) goto error; -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Dec 01, 2014 at 11:17:54AM +0100, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 05:54:35PM +0800, Luyao Huang wrote:
There are some small issue in qemuProcessAttach:
1.Fix virSecurityManagerGetProcessLabel always get pid = 0, move 'vm->pid = pid' before call virSecurityManagerGetProcessLabel.
2.Use virSecurityManagerGenLabel to get image label.
3.Fix always set selinux label for other security driver label.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
It looks like we were doing everything already, but we just did it wrong. Nice catch! ACK.
Oh, I spoke too soon. Two minor things are wrong with this patch that I'll fixup before pushing: 1) The commit message summary is not very descriptive. When someone will go over the git log he/she won't figure out what was the deal from "fix some small issue:. 2) see below...
Martin
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 382d802..ee95adb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5255,6 +5255,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, if (VIR_STRDUP(priv->pidfile, pidfile) < 0) goto error;
+ vm->pid = pid; + VIR_DEBUG("Detect security driver config"); sec_managers = virSecurityManagerGetNested(driver->securityManager); if (sec_managers == NULL) @@ -5272,7 +5274,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, seclabeldef->type = VIR_DOMAIN_SECLABEL_STATIC; if (VIR_ALLOC(seclabel) < 0) goto error; - if (virSecurityManagerGetProcessLabel(driver->securityManager, + if (virSecurityManagerGetProcessLabel(sec_managers[i], vm->def, vm->pid, seclabel) < 0) goto error;
@@ -5290,6 +5292,10 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, } }
+ if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0) { + goto error; + } +
This won't pass our syntax-check.
VIR_DEBUG("Creating domain log file"); if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0) goto error; @@ -5334,8 +5340,6 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, logfile);
- vm->pid = pid; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, QEMU_ASYNC_JOB_NONE, priv->qemuCaps, -1) < 0) goto error; -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/01/2014 06:27 PM, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 11:17:54AM +0100, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 05:54:35PM +0800, Luyao Huang wrote:
There are some small issue in qemuProcessAttach:
1.Fix virSecurityManagerGetProcessLabel always get pid = 0, move 'vm->pid = pid' before call virSecurityManagerGetProcessLabel.
2.Use virSecurityManagerGenLabel to get image label.
3.Fix always set selinux label for other security driver label.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
It looks like we were doing everything already, but we just did it wrong. Nice catch! ACK.
Oh, I spoke too soon. Two minor things are wrong with this patch that I'll fixup before pushing:
1) The commit message summary is not very descriptive. When someone will go over the git log he/she won't figure out what was the deal from "fix some small issue:.
Thanks for pointing out, BTW, maybe qemuprocessattach will failed after this patch without the patch 2/2, because we should give a way to get process uid and gid in virSecurityDACGetProcessLabel otherwise it will always return -1 without a error settings. I will edit Patch 2 and give a v2 in these days (otherwise i cannot use qemu-attach : )).
2) see below...
Martin
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 382d802..ee95adb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5255,6 +5255,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, if (VIR_STRDUP(priv->pidfile, pidfile) < 0) goto error;
+ vm->pid = pid; + VIR_DEBUG("Detect security driver config"); sec_managers = virSecurityManagerGetNested(driver->securityManager); if (sec_managers == NULL) @@ -5272,7 +5274,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, seclabeldef->type = VIR_DOMAIN_SECLABEL_STATIC; if (VIR_ALLOC(seclabel) < 0) goto error; - if (virSecurityManagerGetProcessLabel(driver->securityManager, + if (virSecurityManagerGetProcessLabel(sec_managers[i], vm->def, vm->pid, seclabel) < 0) goto error;
@@ -5290,6 +5292,10 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, } }
+ if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0) { + goto error; + } +
This won't pass our syntax-check. Oh that is a accident, i removed the error settings but forgot the {} when i sent this patch : )
VIR_DEBUG("Creating domain log file"); if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0) goto error; @@ -5334,8 +5340,6 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, logfile);
- vm->pid = pid; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, QEMU_ASYNC_JOB_NONE, priv->qemuCaps, -1) < 0) goto error; -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Dec 01, 2014 at 11:30:09PM +0800, Luyao Huang wrote:
On 12/01/2014 06:27 PM, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 11:17:54AM +0100, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 05:54:35PM +0800, Luyao Huang wrote:
There are some small issue in qemuProcessAttach:
1.Fix virSecurityManagerGetProcessLabel always get pid = 0, move 'vm->pid = pid' before call virSecurityManagerGetProcessLabel.
2.Use virSecurityManagerGenLabel to get image label.
3.Fix always set selinux label for other security driver label.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
It looks like we were doing everything already, but we just did it wrong. Nice catch! ACK.
Oh, I spoke too soon. Two minor things are wrong with this patch that I'll fixup before pushing:
1) The commit message summary is not very descriptive. When someone will go over the git log he/she won't figure out what was the deal from "fix some small issue:.
Thanks for pointing out, BTW, maybe qemuprocessattach will failed after this patch without the patch 2/2, because we should give a way to get process uid and gid in virSecurityDACGetProcessLabel otherwise it will always return -1 without a error settings.
I missed that, those patches should've been sent in reverse order to indicate that this one is dependant on the second one.
I will edit Patch 2 and give a v2 in these days (otherwise i cannot use qemu-attach : )).
Looking forward to v2.
Sorry i didn't know that, i will pay attention next time. And i have send a v2(but i haven't work in FreeBSD before, so...): https://www.redhat.com/archives/libvir-list/2014-December/msg00207.html Thanks, Luyao Huang ----- Original Message ----- From: "Martin Kletzander" <mkletzan@redhat.com> To: "Luyao Huang" <lhuang@redhat.com> Cc: libvir-list@redhat.com Sent: Monday, December 1, 2014 11:41:59 PM Subject: Re: [libvirt] [PATCH 1/2] qemu: fix some small issue in qemuProcessAttach On Mon, Dec 01, 2014 at 11:30:09PM +0800, Luyao Huang wrote:
On 12/01/2014 06:27 PM, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 11:17:54AM +0100, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 05:54:35PM +0800, Luyao Huang wrote:
There are some small issue in qemuProcessAttach:
1.Fix virSecurityManagerGetProcessLabel always get pid = 0, move 'vm->pid = pid' before call virSecurityManagerGetProcessLabel.
2.Use virSecurityManagerGenLabel to get image label.
3.Fix always set selinux label for other security driver label.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
It looks like we were doing everything already, but we just did it wrong. Nice catch! ACK.
Oh, I spoke too soon. Two minor things are wrong with this patch that I'll fixup before pushing:
1) The commit message summary is not very descriptive. When someone will go over the git log he/she won't figure out what was the deal from "fix some small issue:.
Thanks for pointing out, BTW, maybe qemuprocessattach will failed after this patch without the patch 2/2, because we should give a way to get process uid and gid in virSecurityDACGetProcessLabel otherwise it will always return -1 without a error settings.
I missed that, those patches should've been sent in reverse order to indicate that this one is dependant on the second one.
I will edit Patch 2 and give a v2 in these days (otherwise i cannot use qemu-attach : )).
Looking forward to v2. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
When use qemuProcessAttach to attach a qemu process, cannot get a right DAC label. Add a new func to get process label via stat func. Do not remove virDomainDefGetSecurityLabelDef before try to use stat to get process DAC label, because There are some other func call virSecurityDACGetProcessLabel. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/security/security_dac.c | 50 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 85253af..2977f71 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1237,17 +1237,63 @@ virSecurityDACReserveLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, } static int +virSecurityDACGetProcessLabelInternal(pid_t pid, + virSecurityLabelPtr seclabel) +{ + struct stat sb; + char *path = NULL; + char *label = NULL; + int ret = -1; + + VIR_INFO("Getting DAC user and group on process '%d'", pid); + + if (virAsprintf(&path, "/proc/%d", (int) pid) < 0) + goto cleanup; + + if (stat(path, &sb) < 0) + goto cleanup; + + if (virAsprintf(&label, "+%u:+%u", + (unsigned int) sb.st_uid, + (unsigned int) sb.st_gid) < 0) + goto cleanup; + + if (virStrcpy(seclabel->label, label,VIR_SECURITY_LABEL_BUFLEN) == NULL) + goto cleanup; + ret = 0; + +cleanup: + VIR_FREE(path); + VIR_FREE(label); + return ret; +} + +static int virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def, - pid_t pid ATTRIBUTE_UNUSED, + pid_t pid, virSecurityLabelPtr seclabel) { virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (!secdef || !seclabel) + if (!seclabel) return -1; + if (secdef == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing label for DAC security " + "driver in domain %s"), def->name); + + if (virSecurityDACGetProcessLabelInternal(pid, seclabel) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot get process %d DAC label"),pid); + return -1; + } + + return 0; + } + if (secdef->label) ignore_value(virStrcpy(seclabel->label, secdef->label, VIR_SECURITY_LABEL_BUFLEN)); -- 1.8.3.1
On Mon, Dec 01, 2014 at 05:54:36PM +0800, Luyao Huang wrote:
When use qemuProcessAttach to attach a qemu process, cannot get a right DAC label. Add a new func to get process label via stat func. Do not remove virDomainDefGetSecurityLabelDef before try to use stat to get process DAC label, because There are some other func call virSecurityDACGetProcessLabel.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/security/security_dac.c | 50 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 85253af..2977f71 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1237,17 +1237,63 @@ virSecurityDACReserveLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, }
static int +virSecurityDACGetProcessLabelInternal(pid_t pid, + virSecurityLabelPtr seclabel) +{ + struct stat sb; + char *path = NULL; + char *label = NULL; + int ret = -1; + + VIR_INFO("Getting DAC user and group on process '%d'", pid); + + if (virAsprintf(&path, "/proc/%d", (int) pid) < 0) + goto cleanup; +
This won't work on systems without /proc.
+ if (stat(path, &sb) < 0) + goto cleanup; +
Better use lstat.
+ if (virAsprintf(&label, "+%u:+%u", + (unsigned int) sb.st_uid, + (unsigned int) sb.st_gid) < 0) + goto cleanup; + + if (virStrcpy(seclabel->label, label,VIR_SECURITY_LABEL_BUFLEN) == NULL) + goto cleanup; + ret = 0; + +cleanup: + VIR_FREE(path); + VIR_FREE(label); + return ret; +} + +static int virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def, - pid_t pid ATTRIBUTE_UNUSED, + pid_t pid, virSecurityLabelPtr seclabel) { virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
- if (!secdef || !seclabel) + if (!seclabel)
I wonder whether this won't screw up domain definitions that don't want to have any seclabel set (those defined with XML), I need to figure that out.
return -1;
+ if (secdef == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing label for DAC security " + "driver in domain %s"), def->name); +
This should probably be VIR_DEBUG or VIR_INFO, otherwise you report error without erroring out (returning -1) and it gets saved for the connection.
+ if (virSecurityDACGetProcessLabelInternal(pid, seclabel) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot get process %d DAC label"),pid); + return -1;
Also two errors will be reported if this fails. Martin
+ } + + return 0; + } + if (secdef->label) ignore_value(virStrcpy(seclabel->label, secdef->label, VIR_SECURITY_LABEL_BUFLEN)); -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Dec 01, 2014 at 05:54:36PM +0800, Luyao Huang wrote:
When use qemuProcessAttach to attach a qemu process, cannot get a right DAC label. Add a new func to get process label via stat func. Do not remove virDomainDefGetSecurityLabelDef before try to use stat to get process DAC label, because There are some other func call virSecurityDACGetProcessLabel.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/security/security_dac.c | 50 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 85253af..2977f71 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1237,17 +1237,63 @@ virSecurityDACReserveLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, }
static int +virSecurityDACGetProcessLabelInternal(pid_t pid, + virSecurityLabelPtr seclabel) +{ + struct stat sb; + char *path = NULL; + char *label = NULL; + int ret = -1; + + VIR_INFO("Getting DAC user and group on process '%d'", pid); + + if (virAsprintf(&path, "/proc/%d", (int) pid) < 0) + goto cleanup; +
This won't work on systems without /proc. Okay, i need find a way to check this or find a way to get other system
On 12/01/2014 06:24 PM, Martin Kletzander wrote: process uid and gid.
+ if (stat(path, &sb) < 0) + goto cleanup; +
Better use lstat.
Thanks your advice.
+ if (virAsprintf(&label, "+%u:+%u", + (unsigned int) sb.st_uid, + (unsigned int) sb.st_gid) < 0) + goto cleanup; + + if (virStrcpy(seclabel->label, label,VIR_SECURITY_LABEL_BUFLEN) == NULL) + goto cleanup; + ret = 0; + +cleanup: + VIR_FREE(path); + VIR_FREE(label); + return ret; +} + +static int virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def, - pid_t pid ATTRIBUTE_UNUSED, + pid_t pid, virSecurityLabelPtr seclabel) { virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
- if (!secdef || !seclabel) + if (!seclabel)
I wonder whether this won't screw up domain definitions that don't want to have any seclabel set (those defined with XML), I need to figure that out.
After check the func wihch call virSecurityManagerGetProcessLabel, both of them will report error and go to error when failed get the DAC label (also for selinux). The most important reason is the virSecuritySELinuxGetSecurityProcessLabel, After i see this func, i find it don't use def in this func ( don't use virSecuritySELinuxGetSecurityProcessLabel to get the labels from def) And i think this is the right way, because virSecurityManagerGetProcessLabel should be a func which get the label from the process. So in my opinion, if we can give a right and good way to get process DAC label for the system we support, we can remove virDomainDefGetSecurityLabelDef and do not use def in this func.
return -1;
+ if (secdef == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing label for DAC security " + "driver in domain %s"), def->name); +
This should probably be VIR_DEBUG or VIR_INFO, otherwise you report error without erroring out (returning -1) and it gets saved for the connection.
Yes, i used wrong func in this place, it should be VIR_DEBUG.
+ if (virSecurityDACGetProcessLabelInternal(pid, seclabel) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot get process %d DAC label"),pid); + return -1;
Also two errors will be reported if this fails.
Oh, i didn't notice that, thanks for pointing out.
Martin
+ } + + return 0; + } + if (secdef->label) ignore_value(virStrcpy(seclabel->label, secdef->label, VIR_SECURITY_LABEL_BUFLEN)); -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Dec 01, 2014 at 11:05:30PM +0800, Luyao Huang wrote:
On Mon, Dec 01, 2014 at 05:54:36PM +0800, Luyao Huang wrote:
When use qemuProcessAttach to attach a qemu process, cannot get a right DAC label. Add a new func to get process label via stat func. Do not remove virDomainDefGetSecurityLabelDef before try to use stat to get process DAC label, because There are some other func call virSecurityDACGetProcessLabel.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/security/security_dac.c | 50 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 85253af..2977f71 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1237,17 +1237,63 @@ virSecurityDACReserveLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, }
static int +virSecurityDACGetProcessLabelInternal(pid_t pid, + virSecurityLabelPtr seclabel) +{ + struct stat sb; + char *path = NULL; + char *label = NULL; + int ret = -1; + + VIR_INFO("Getting DAC user and group on process '%d'", pid); + + if (virAsprintf(&path, "/proc/%d", (int) pid) < 0) + goto cleanup; +
This won't work on systems without /proc. Okay, i need find a way to check this or find a way to get other system
On 12/01/2014 06:24 PM, Martin Kletzander wrote: process uid and gid.
Have a look at virProcessGetStartTime(), its implementation for linux and freebsd should be enough for where virSecurityDACGetProcessLabelInternal is going to be called.
+ if (stat(path, &sb) < 0) + goto cleanup; +
Better use lstat. Thanks your advice.
+ if (virAsprintf(&label, "+%u:+%u", + (unsigned int) sb.st_uid, + (unsigned int) sb.st_gid) < 0) + goto cleanup; + + if (virStrcpy(seclabel->label, label,VIR_SECURITY_LABEL_BUFLEN) == NULL) + goto cleanup; + ret = 0; + +cleanup: + VIR_FREE(path); + VIR_FREE(label); + return ret; +} + +static int virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def, - pid_t pid ATTRIBUTE_UNUSED, + pid_t pid, virSecurityLabelPtr seclabel) { virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
- if (!secdef || !seclabel) + if (!seclabel)
I wonder whether this won't screw up domain definitions that don't want to have any seclabel set (those defined with XML), I need to figure that out. After check the func wihch call virSecurityManagerGetProcessLabel, both of them will report error and go to error when failed get the DAC label (also for selinux).
The most important reason is the virSecuritySELinuxGetSecurityProcessLabel, After i see this func, i find it don't use def in this func ( don't use virSecuritySELinuxGetSecurityProcessLabel to get the labels from def) And i think this is the right way, because virSecurityManagerGetProcessLabel should be a func which get the label from the process.
So in my opinion, if we can give a right and good way to get process DAC label for the system we support, we can remove virDomainDefGetSecurityLabelDef and do not use def in this func.
Well, if there is some, we can output that. And if there is not, we'll get the current one. I think this function can stay as is (apart from the problem pointed out below, of course).
return -1;
+ if (secdef == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing label for DAC security " + "driver in domain %s"), def->name); +
This should probably be VIR_DEBUG or VIR_INFO, otherwise you report error without erroring out (returning -1) and it gets saved for the connection.
Yes, i used wrong func in this place, it should be VIR_DEBUG.
+ if (virSecurityDACGetProcessLabelInternal(pid, seclabel) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot get process %d DAC label"),pid); + return -1;
Also two errors will be reported if this fails.
Oh, i didn't notice that, thanks for pointing out.
Martin
+ } + + return 0; + } + if (secdef->label) ignore_value(virStrcpy(seclabel->label, secdef->label, VIR_SECURITY_LABEL_BUFLEN)); -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/01/2014 11:20 PM, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 11:05:30PM +0800, Luyao Huang wrote:
On Mon, Dec 01, 2014 at 05:54:36PM +0800, Luyao Huang wrote:
When use qemuProcessAttach to attach a qemu process, cannot get a right DAC label. Add a new func to get process label via stat func. Do not remove virDomainDefGetSecurityLabelDef before try to use stat to get process DAC label, because There are some other func call virSecurityDACGetProcessLabel.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/security/security_dac.c | 50 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 85253af..2977f71 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1237,17 +1237,63 @@ virSecurityDACReserveLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, }
static int +virSecurityDACGetProcessLabelInternal(pid_t pid, + virSecurityLabelPtr seclabel) +{ + struct stat sb; + char *path = NULL; + char *label = NULL; + int ret = -1; + + VIR_INFO("Getting DAC user and group on process '%d'", pid); + + if (virAsprintf(&path, "/proc/%d", (int) pid) < 0) + goto cleanup; +
This won't work on systems without /proc. Okay, i need find a way to check this or find a way to get other system
On 12/01/2014 06:24 PM, Martin Kletzander wrote: process uid and gid.
Have a look at virProcessGetStartTime(), its implementation for linux and freebsd should be enough for where virSecurityDACGetProcessLabelInternal is going to be called. Thanks a lot! I am worrying about that ; )
+ if (stat(path, &sb) < 0) + goto cleanup; +
Better use lstat. Thanks your advice.
+ if (virAsprintf(&label, "+%u:+%u", + (unsigned int) sb.st_uid, + (unsigned int) sb.st_gid) < 0) + goto cleanup; + + if (virStrcpy(seclabel->label, label,VIR_SECURITY_LABEL_BUFLEN) == NULL) + goto cleanup; + ret = 0; + +cleanup: + VIR_FREE(path); + VIR_FREE(label); + return ret; +} + +static int virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def, - pid_t pid ATTRIBUTE_UNUSED, + pid_t pid, virSecurityLabelPtr seclabel) { virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
- if (!secdef || !seclabel) + if (!seclabel)
I wonder whether this won't screw up domain definitions that don't want to have any seclabel set (those defined with XML), I need to figure that out. After check the func wihch call virSecurityManagerGetProcessLabel, both of them will report error and go to error when failed get the DAC label (also for selinux).
The most important reason is the virSecuritySELinuxGetSecurityProcessLabel, After i see this func, i find it don't use def in this func ( don't use virSecuritySELinuxGetSecurityProcessLabel to get the labels from def) And i think this is the right way, because virSecurityManagerGetProcessLabel should be a func which get the label from the process.
So in my opinion, if we can give a right and good way to get process DAC label for the system we support, we can remove virDomainDefGetSecurityLabelDef and do not use def in this func.
Well, if there is some, we can output that. And if there is not, we'll get the current one. I think this function can stay as is (apart from the problem pointed out below, of course).
I am agree with you :)
return -1;
+ if (secdef == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing label for DAC security " + "driver in domain %s"), def->name); +
This should probably be VIR_DEBUG or VIR_INFO, otherwise you report error without erroring out (returning -1) and it gets saved for the connection.
Yes, i used wrong func in this place, it should be VIR_DEBUG.
+ if (virSecurityDACGetProcessLabelInternal(pid, seclabel) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot get process %d DAC label"),pid); + return -1;
Also two errors will be reported if this fails.
Oh, i didn't notice that, thanks for pointing out.
Martin
+ } + + return 0; + } + if (secdef->label) ignore_value(virStrcpy(seclabel->label, secdef->label, VIR_SECURITY_LABEL_BUFLEN)); -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Luyao Huang -
Martin Kletzander