4:17 a.m.
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(a)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
1:57 a.m.
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2:25 a.m.
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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Luyao Huang
3701
days inactive
3703
days old
2 comments
3 participants
participants (3)
-
lhuang -
Luyao Huang
-
Martin Kletzander