On Fri, Aug 10, 2012 at 03:50:25PM -0600, Eric Blake wrote: > On 08/10/2012 07:48 AM, Daniel P. Berrange wrote: > > +++ b/tests/securityselinuxhelper.c > > @@ -0,0 +1,65 @@ > > +/* > > + * Copyright (C) 2011-2012 Red Hat, Inc. > > + * > > + * This library is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later version. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * License along with this library; If not, see > > + * <http://www.gnu.org/licenses/>. > > + * > > + */ > > + > > +#include <config.h> > > + > > +#include <selinux/selinux.h> > > +#include <stdlib.h> > > +#include <string.h> > > +#include <unistd.h> > > +#include <errno.h> > > +/* > > + * The kernel policy will not allow us to arbitrarily change > > + * test process context. This helper is used as an LD_PRELOAD > > + * so that the libvirt code /thinks/ it is changing/reading > > + * the process context, where as in fact we're faking it all > > + */ > > + > > +int getcon(security_context_t *context) > > +{ > > + if (getenv("FAKE_CONTEXT") == NULL) { > > + *context = NULL; > > + errno = EINVAL; > > + return -1; > > + } > > + *context = strdup(getenv("FAKE_CONTEXT")); > > No check for allocation failure? (not that it's likely, but it never > hurts to be thorough) Ok, adding these > > +static virDomainDefPtr > > +testBuildDomainDef(bool dynamic, > > + const char *label, > > + const char *baselabel) > > +{ > > + virDomainDefPtr def; > > + > > + if (VIR_ALLOC(def) < 0) > > + goto no_memory; > > + > > + def->seclabel.type = dynamic ? VIR_DOMAIN_SECLABEL_DYNAMIC : VIR_DOMAIN_SECLABEL_STATIC; > > +// if (!(def->seclabel.model = strdup("selinux"))) > > +// goto no_memory; > > Why the comments? Obsolete code, removing it. > > > +static bool > > +testSELinuxCheckCon(context_t con, > > + const char *user, > > + const char *role, > > + const char *type, > > + int sensMin, > > + int sensMax, > > + int catMin, > > + int catMax) > > +{ > > > + tmp++; > > + if (virStrToLong_i(tmp, &tmp, 10, &gotCatOne) < 0) { > > + fprintf(stderr, "Malformed range %s, cannot parse category one\n", > > + tmp); > > + return false; > > + } > > + if (tmp && *tmp == ',') > > Did you mean '.' instead of ','? No. Subtle distinction in semantics. 'cN.cM' is used to denote a category range, while 'cM,cM' is used to denate a category pair > > > + tmp++; > > + if (tmp && *tmp == 'c') { > > + tmp++; > > + if (virStrToLong_i(tmp, &tmp, 10, &gotCatTwo) < 0) { > > + fprintf(stderr, "Malformed range %s, cannot parse category two\n", > > + tmp); > > + return false; > > + } > > + } else { > > + gotCatTwo = gotCatOne; > > + } > > Where do you check that theres no junk after the last category? Added a check for this. > > > + > > + if (gotSens < sensMin || > > + gotSens > sensMax) { > > + fprintf(stderr, "Sensitivity %d is out of range %d-%d\n", > > + gotSens, sensMin, sensMax); > > + return false; > > + } > > Are you meaning to do a range check here (where input of s2-s3 could let > libvirt choose s3), or actually enforcing that libvirt always picks the > lowest end of the range? Good point, I'm changing this to enforce gotSens == sensMin > > + > > + /* We can only run these tests if we're unconfined */ > > Does the test gracefully skip when run in a different context? This is an obsolete comment now - it predates my use of the LD_PRELOAD hack > > + DO_TEST_GEN_LABEL("dynamic unconfined, s0, c0.c1023", > > + "unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023", > > + true, NULL, NULL, > > + "unconfined_u", "unconfined_r", "svirt_t", "svirt_image_t", > > + 0, 0, 0, 1023); > > + DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c1023", > > + "system_u:system_r:virtd_t:s0-s0:c0.c1023", > > + true, NULL, NULL, > > + "system_u", "system_r", "svirt_t", "svirt_image_t", > > + 0, 0, 0, 1023); > > + DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c10", > > + "system_u:system_r:virtd_t:s0-s0:c0.c10", > > + true, NULL, NULL, > > + "system_u", "system_r", "svirt_t", "svirt_image_t", > > + 0, 0, 0, 10); > > + DO_TEST_GEN_LABEL("dynamic virtd, s2-s3, c0.c1023", > > + "system_u:system_r:virtd_t:s2-s3:c0.c1023", > > + true, NULL, NULL, > > + "system_u", "system_r", "svirt_t", "svirt_image_t", > > + 2, 3, 0, 1023); > > No test of a degenerate single-range category? I'll change one of the 's0-s0' case to just 's0' > > index f372c23..16414d9 100644 > > --- a/tests/testutils.h > > +++ b/tests/testutils.h > > @@ -70,4 +70,13 @@ int virtTestMain(int argc, > > return virtTestMain(argc, argv, func); \ > > } > > > > +# define VIRT_TEST_MAIN_PRELOAD(func, lib) \ > > + int main(int argc, char **argv) { \ > > + if (getenv("LD_PRELOAD") == NULL) { \ > > + setenv("LD_PRELOAD", lib, 1); \ > > + execv(argv[0], argv); \ > > This is insufficient - if LD_PRELOAD is already set for other reasons > (such as valgrind), then you still want to modify it and re-exec. I > think you need to strstr() the existing contents for the new lib before > deciding whether to re-exec. Ah yes, good point Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list