3:54 a.m.
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
3:54 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: fix some small issue in qemuProcessAttach
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(a)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
4:17 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: fix some small issue in qemuProcessAttach
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:27 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: fix some small issue in qemuProcessAttach
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(a)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(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
9:30 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: fix some small issue in qemuProcessAttach
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(a)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(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
9:41 a.m.
New subject: [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(a)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.
4:11 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: fix some small issue in qemuProcessAttach
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(a)redhat.com>
To: "Luyao Huang" <lhuang(a)redhat.com>
Cc: libvir-list(a)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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:54 a.m.
New subject: [libvirt] [PATCH 2/2] security: Add a new func use stat to get process DAC label
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(a)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
4:24 a.m.
New subject: [libvirt] [PATCH 2/2] security: Add a new func use stat to get process DAC label
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:05 a.m.
New subject: [libvirt] [PATCH 2/2] security: Add a new func use stat to get process DAC label
On 12/01/2014 06:24 PM, Martin Kletzander 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(a)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
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
9:20 a.m.
New subject: [libvirt] [PATCH 2/2] security: Add a new func use stat to get process DAC label
On Mon, Dec 01, 2014 at 11:05:30PM +0800, Luyao Huang wrote:
On 12/01/2014 06:24 PM, Martin Kletzander 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(a)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
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(a)redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
9:28 a.m.
New subject: [libvirt] [PATCH 2/2] security: Add a new func use stat to get process DAC label
On 12/01/2014 11:20 PM, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 11:05:30PM +0800, Luyao Huang wrote:
>
> On 12/01/2014 06:24 PM, Martin Kletzander 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(a)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
> 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(a)redhat.com
>>> https://www.redhat.com/mailman/listinfo/libvir-list
>
3678
days inactive
3680
days old
11 comments
2 participants
participants (2)
-
Luyao Huang -
Martin Kletzander