[libvirt] [PATCH] security:selinux: Fix crash when tcon is NULL
https://bugzilla.redhat.com/show_bug.cgi?id=1161831 Libvirtd will crash when parameter tcon = NULL in virSecuritySELinuxSetFileconHelper function, because libvirt do not check the first parameter when use strcmp(). Add a check for tcon before use strcmp() and output a error in log when tcon is NULL. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/security/security_selinux.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f96be50..4fd09b8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -887,6 +887,11 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool optional) int setfilecon_errno = errno; if (getfilecon_raw(path, &econ) >= 0) { + if (tcon == NULL) { + virReportSystemError(errno,"%s", + _("Invalid security context : NULL")); + return -1; + } if (STREQ(tcon, econ)) { freecon(econ); /* It's alright, there's nothing to change anyway. */ -- 1.8.3.1
On Sat, Nov 08, 2014 at 06:17:26PM +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1161831
Libvirtd will crash when parameter tcon = NULL in virSecuritySELinuxSetFileconHelper function, because libvirt do not check the first parameter when use strcmp(). Add a check for tcon before use strcmp() and output a error in log when tcon is NULL.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/security/security_selinux.c | 5 +++++ 1 file changed, 5 insertions(+)
NACK, calling virSecuritySELinuxSetFileconHelper() with NULL doesn't make sense at all. If you are trying to just fix a possible future errot, then the check for non-NULL parameters needs to be done when the function is starting. But I doubt it's useful. It's another story if you managed to cause such call to this function.
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f96be50..4fd09b8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -887,6 +887,11 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool optional) int setfilecon_errno = errno;
if (getfilecon_raw(path, &econ) >= 0) { + if (tcon == NULL) { + virReportSystemError(errno,"%s", + _("Invalid security context : NULL")); + return -1; + }
Explaining how did you reach this point with tcon == NULL, or if you even managed to do that, would help others understanding the issue from higher level. Anyway, the problem here is that this function is called with tcon == NULL and not that it doesn't check for that (if you managed to see the segfault). To explain what I mean; if you managed to see the segfault when you performed some operation, that's bad. But when this patch is applied, you still won't be able to perform that operation successfully and we need to find out the root cause of that. The backtrace would fit nicely in the commit message as well. Martin
if (STREQ(tcon, econ)) { freecon(econ); /* It's alright, there's nothing to change anyway. */ -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/10/2014 03:57 PM, Martin Kletzander wrote:
On Sat, Nov 08, 2014 at 06:17:26PM +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1161831
Libvirtd will crash when parameter tcon = NULL in virSecuritySELinuxSetFileconHelper function, because libvirt do not check the first parameter when use strcmp(). Add a check for tcon before use strcmp() and output a error in log when tcon is NULL.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/security/security_selinux.c | 5 +++++ 1 file changed, 5 insertions(+)
NACK, calling virSecuritySELinuxSetFileconHelper() with NULL doesn't make sense at all. If you are trying to just fix a possible future errot, then the check for non-NULL parameters needs to be done when the function is starting. But I doubt it's useful. It's another story if you managed to cause such call to this function. Thanks your reply and It's a real issue :) what i do : # /usr/libexec/qemu-kvm -cdrom /tmp/test.iso -monitor unix:/tmp/demo,server,nowait -name foo -uuid cece4f9f-dff0-575d-0e8e-01fe380f12ea & [1] 16636
# VNC server running on `::1:5900' # virsh qemu-attach 16636 Domain foo attached to pid 16636 # virsh screenshot foo error: could not take a screenshot of foo error: Cannot recv data: Connection reset by peer error: Failed to reconnect to the hypervisor Maybe root cause is : vm doesn't have a image label <seclabel type='static' model='selinux' relabel='yes'> <label>system_u:system_r:virtd_t:s0-s0:c0.c1023</label> </seclabel>
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f96be50..4fd09b8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -887,6 +887,11 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool optional) int setfilecon_errno = errno;
if (getfilecon_raw(path, &econ) >= 0) { + if (tcon == NULL) { + virReportSystemError(errno,"%s", + _("Invalid security context : NULL")); + return -1; + }
Explaining how did you reach this point with tcon == NULL, or if you even managed to do that, would help others understanding the issue from higher level. Anyway, the problem here is that this function is called with tcon == NULL and not that it doesn't check for that (if you managed to see the segfault). To explain what I mean; if you managed to see the segfault when you performed some operation, that's bad. But when this patch is applied, you still won't be able to perform that operation successfully and we need to find out the root cause of that.
The backtrace would fit nicely in the commit message as well.
Martin
Backtrace is here: (gdb) bt #0 0x00007ff38a0c9e76 in __strcmp_sse42 () from /lib64/libc.so.6 #1 0x00007ff38d08d6a9 in virSecuritySELinuxSetFileconHelper (path=0x7ff36c2626c0 "/var/cache/libvirt/qemu/qemu.screendump.xRL8fw", tcon=0x0, optional=<optimized out>) at security/security_selinux.c:890 #2 0x00007ff38d089c63 in virSecurityManagerSetSavedStateLabel (mgr=0x7ff36c0f6f40, vm=vm@entry=0x7ff3680011c0, savefile=savefile@entry=0x7ff36c2626c0 "/var/cache/libvirt/qemu/qemu.screendump.xRL8fw") at security/security_manager.c:547 #3 0x00007ff38d086bc6 in virSecurityStackSetSavedStateLabel (mgr=<optimized out>, vm=0x7ff3680011c0, savefile=0x7ff36c2626c0 "/var/cache/libvirt/qemu/qemu.screendump.xRL8fw") at security/security_stack.c:351 #4 0x00007ff38d089c63 in virSecurityManagerSetSavedStateLabel (mgr=0x7ff36c106fc0, vm=0x7ff3680011c0, savefile=0x7ff36c2626c0 "/var/cache/libvirt/qemu/qemu.screendump.xRL8fw") at security/security_manager.c:547 #5 0x00007ff375e7d8b4 in qemuDomainScreenshot (dom=0x7ff36c000910, st=0x7ff36c262010, screen=<optimized out>, flags=<optimized out>) at qemu/qemu_driver.c:3854 #6 0x00007ff38cfa5d60 in virDomainScreenshot (domain=domain@entry=0x7ff36c000910, stream=stream@entry=0x7ff36c262010, screen=0, flags=0) at libvirt.c:3171 #7 0x00007ff38da489d3 in remoteDispatchDomainScreenshot (server=<optimized out>, ret=0x7ff36c000a20, args=0x7ff36c23ef80, rerr=0x7ff37d6cdc80, msg=<optimized out>, client=0x7ff38fb404e0) at remote_dispatch.h:7473 #8 remoteDispatchDomainScreenshotHelper (server=<optimized out>, client=0x7ff38fb404e0, msg=<optimized out>, rerr=0x7ff37d6cdc80, args=0x7ff36c23ef80, ret=0x7ff36c000a20) at remote_dispatch.h:7440 #9 0x00007ff38d019752 in virNetServerProgramDispatchCall (msg=0x7ff38fb40470, client=0x7ff38fb404e0, server=0x7ff38fb3f460, prog=0x7ff38fb4aea0) at rpc/virnetserverprogram.c:437 #10 virNetServerProgramDispatch (prog=0x7ff38fb4aea0, server=server@entry=0x7ff38fb3f460, client=0x7ff38fb404e0, msg=0x7ff38fb40470) at rpc/virnetserverprogram.c:307 #11 0x00007ff38da6820d in virNetServerProcessMsg (msg=<optimized out>, prog=<optimized out>, client=<optimized out>, srv=0x7ff38fb3f460) at rpc/virnetserver.c:172 #12 virNetServerHandleJob (jobOpaque=<optimized out>, opaque=0x7ff38fb3f460) at rpc/virnetserver.c:193 #13 0x00007ff38cf1e2e5 in virThreadPoolWorker (opaque=opaque@entry=0x7ff38fb21a80) at util/virthreadpool.c:145 #14 0x00007ff38cf1dc7e in virThreadHelper (data=<optimized out>) at util/virthread.c:197 #15 0x00007ff38a367df3 in start_thread () from /lib64/libpthread.so.0 #16 0x00007ff38a08e05d in clone () from /lib64/libc.so.6
if (STREQ(tcon, econ)) { freecon(econ); /* It's alright, there's nothing to change anyway. */ -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Luyao Huang
participants (3)
-
lhuang -
Luyao Huang
-
Martin Kletzander