Re: [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&gt; >>> --- >>> 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 >