[libvirt] [PATCH] security:selinux: Fix crash when tcon is NULL

[lhuang]

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 at 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 at entry=0x7ff3680011c0,
     savefile=savefile at 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 at entry=0x7ff36c000910, stream=stream at 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 at 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 at 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

Luyao Huang