[libvirt] [PATCH 0/2] Seclabel SIGSEGV issue
There's been a crasher reported just a few days ago: https://www.redhat.com/archives/libvirt-users/2013-July/msg00067.html I'm proposing two patches which I can't decide which one is better. Both fixes the problem, though. Opinions? Michal Privoznik (2): lxcCapsInit: Allocate primary security driver unconditionally virSecurityManagerGenLabel: Skip seclabels without model src/security/security_manager.c | 3 +++ 1 file changed, 3 insertions(+) -- 1.8.1.5
Currently, if the primary security driver is 'none', we skip initializing caps->host.secModels. This means, later, when LXC domain XML is parsed and <seclabel type='none'/> is found (see virSecurityLabelDefsParseXML), the model name is not copied to the seclabel. This leads to subsequent crash in virSecurityManagerGenLabel where we call STREQ() over the model (note, that we are expecting model to be !NULL). --- src/lxc/lxc_conf.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index 4e859c5..78b1559 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -114,16 +114,14 @@ virCapsPtr lxcCapsInit(virLXCDriverPtr driver) doi = virSecurityManagerGetDOI(driver->securityManager); model = virSecurityManagerGetModel(driver->securityManager); - if (STRNEQ(model, "none")) { - /* Allocate just the primary security driver for LXC. */ - if (VIR_ALLOC(caps->host.secModels) < 0) - goto error; - caps->host.nsecModels = 1; - if (VIR_STRDUP(caps->host.secModels[0].model, model) < 0) - goto error; - if (VIR_STRDUP(caps->host.secModels[0].doi, doi) < 0) - goto error; - } + /* Allocate the primary security driver for LXC. */ + if (VIR_ALLOC(caps->host.secModels) < 0) + goto error; + caps->host.nsecModels = 1; + if (VIR_STRDUP(caps->host.secModels[0].model, model) < 0) + goto error; + if (VIR_STRDUP(caps->host.secModels[0].doi, doi) < 0) + goto error; VIR_DEBUG("Initialized caps for security driver \"%s\" with " "DOI \"%s\"", model, doi); -- 1.8.1.5
On Mon, Jul 15, 2013 at 03:58:27PM +0200, Michal Privoznik wrote:
Currently, if the primary security driver is 'none', we skip initializing caps->host.secModels. This means, later, when LXC domain XML is parsed and <seclabel type='none'/> is found (see virSecurityLabelDefsParseXML), the model name is not copied to the seclabel. This leads to subsequent crash in virSecurityManagerGenLabel where we call STREQ() over the model (note, that we are expecting model to be !NULL). --- src/lxc/lxc_conf.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index 4e859c5..78b1559 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -114,16 +114,14 @@ virCapsPtr lxcCapsInit(virLXCDriverPtr driver)
doi = virSecurityManagerGetDOI(driver->securityManager); model = virSecurityManagerGetModel(driver->securityManager); - if (STRNEQ(model, "none")) { - /* Allocate just the primary security driver for LXC. */ - if (VIR_ALLOC(caps->host.secModels) < 0) - goto error; - caps->host.nsecModels = 1; - if (VIR_STRDUP(caps->host.secModels[0].model, model) < 0) - goto error; - if (VIR_STRDUP(caps->host.secModels[0].doi, doi) < 0) - goto error; - } + /* Allocate the primary security driver for LXC. */ + if (VIR_ALLOC(caps->host.secModels) < 0) + goto error; + caps->host.nsecModels = 1; + if (VIR_STRDUP(caps->host.secModels[0].model, model) < 0) + goto error; + if (VIR_STRDUP(caps->host.secModels[0].doi, doi) < 0) + goto error;
VIR_DEBUG("Initialized caps for security driver \"%s\" with " "DOI \"%s\"", model, doi);
The QEMU driver does not have any special handling of the "none" sec model type. So I'm inclined to say that removing the special handling from LXC is good from the POV of consistency. 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 :|
While generating seclabels, we check the seclabel stack if required driver is in the stack. If not, an error is returned. However, it is possible for a seclabel to not have any model set (happens with LXC domains that have just <seclabel type='none'>). If that's the case, we should just skip the iteration instead of calling STREQ(NULL, ...) and SIGSEGV-ing subsequently. --- src/security/security_manager.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 6946637..411a909 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -436,6 +436,9 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virObjectLock(mgr); for (i = 0; i < vm->nseclabels; i++) { + if (!vm->seclabels[i]->model) + continue; + for (j = 0; sec_managers[j]; j++) if (STREQ(vm->seclabels[i]->model, sec_managers[j]->drv->name)) break; -- 1.8.1.5
On Mon, Jul 15, 2013 at 03:58:28PM +0200, Michal Privoznik wrote:
While generating seclabels, we check the seclabel stack if required driver is in the stack. If not, an error is returned. However, it is possible for a seclabel to not have any model set (happens with LXC domains that have just <seclabel type='none'>). If that's the case, we should just skip the iteration instead of calling STREQ(NULL, ...) and SIGSEGV-ing subsequently. --- src/security/security_manager.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 6946637..411a909 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -436,6 +436,9 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
virObjectLock(mgr); for (i = 0; i < vm->nseclabels; i++) { + if (!vm->seclabels[i]->model) + continue; + for (j = 0; sec_managers[j]; j++) if (STREQ(vm->seclabels[i]->model, sec_managers[j]->drv->name)) break;
ACK to this one too. Even though we can fix the LXC driver in your first patch, adding this second patch is useful crash protection. Regards, 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 :|
On Wed, Jul 17, 2013 at 5:10 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Jul 15, 2013 at 03:58:28PM +0200, Michal Privoznik wrote:
While generating seclabels, we check the seclabel stack if required driver is in the stack. If not, an error is returned. However, it is possible for a seclabel to not have any model set (happens with LXC domains that have just <seclabel type='none'>). If that's the case, we should just skip the iteration instead of calling STREQ(NULL, ...) and SIGSEGV-ing subsequently. --- src/security/security_manager.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 6946637..411a909 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -436,6 +436,9 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
virObjectLock(mgr); for (i = 0; i < vm->nseclabels; i++) { + if (!vm->seclabels[i]->model) + continue; + for (j = 0; sec_managers[j]; j++) if (STREQ(vm->seclabels[i]->model, sec_managers[j]->drv->name)) break;
ACK to this one too. Even though we can fix the LXC driver in your first patch, adding this second patch is useful crash protection.
Regards, 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 :|
Ok to push this into v1.1.0-maint as this fixes a crasher for users with this configuration? Should we also push the 1/2 patch? -- Doug Goldstein
On 07/20/2013 07:40 AM, Doug Goldstein wrote:
On Wed, Jul 17, 2013 at 5:10 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Jul 15, 2013 at 03:58:28PM +0200, Michal Privoznik wrote:
While generating seclabels, we check the seclabel stack if required driver is in the stack. If not, an error is returned. However, it is possible for a seclabel to not have any model set (happens with LXC domains that have just <seclabel type='none'>). If that's the case, we should just skip the iteration instead of calling STREQ(NULL, ...) and SIGSEGV-ing subsequently. --- src/security/security_manager.c | 3 +++
ACK to this one too. Even though we can fix the LXC driver in your first patch, adding this second patch is useful crash protection.
Regards, 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 :|
Ok to push this into v1.1.0-maint as this fixes a crasher for users with this configuration? Should we also push the 1/2 patch?
Yes, go ahead and push both into v1.1.0-maint. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange -
Doug Goldstein -
Eric Blake -
Michal Privoznik