openhpi2 / Open-HPI

Open HPI is an open source implementation of the SA Forum's Hardware Platform Interface (HPI). HPI provides an abstracted interface to managing computer hardware, typically for chassis and rack based servers
Other
3 stars 1 forks source link

segfault in IpmiGetEvent - probable use after free race #2530

Open openhpi2 opened 10 years ago

openhpi2 commented 10 years ago
Core was generated by `/usr/sbin/openhpid -c /etc/openhpi/openhpi.conf'.
Program terminated with signal 11, Segmentation fault.
#0  0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00007fdf9a3921ad in IpmiGetEvent (hnd=<value optimized out>) at ipmi.cpp:454
#2  0x000000000041269e in harvest_events_for_handler () at event.c:178
#3  oh_harvest_events () at event.c:208
#4  0x000000000042301f in evtget_func (data=<value optimized out>) at threaded.c:81
#5  0x00007fdf9c635004 in g_thread_create_proxy (data=0x65d4c0) at gthread.c:635
#6  0x00007fdf9c3bd9d1 in start_thread (arg=0x7fdf877eb700) at pthread_create.c:301
#7  0x00007fdf9bc70b6d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115

(gdb) disass /m IpmiGetEvent  
Dump of assembler code for function IpmiGetEvent(void*):
31        if (!hnd)
   0x000000000001f15a <+26>:    test   %rdi,%rdi
   0x000000000001f15d <+29>:    je     0x1f174 <IpmiGetEvent(void*)+52>
----8<----
444       cIpmi *ipmi = VerifyIpmi( hnd );
445       struct oh_event event;
446
447       if ( !ipmi )
448          {
449            return SA_ERR_HPI_INTERNAL_ERROR;
450          }
451
452       // there is no need to get a lock because
453       // the event queue has its own lock
454       SaErrorT rv = ipmi->IfGetEvent( &event );
   0x000000000001f1a1 <+97>:    mov    (%rdx),%rax
   0x000000000001f1a4 <+100>:   mov    %rsp,%rsi
   0x000000000001f1a7 <+103>:   mov    %rdx,%rdi
   0x000000000001f1aa <+106>:   callq  *0x58(%rax)  <----============ HERE
```~~
So %rax == ipmi == 0x656340 and 0x58+0x656340 == 0x656398 and the value stored within that should be the address of the function IfGetEvent is 0x0

It looks like there may be a race where the following thread has already freed the cIpmi instance in oh_close_handlers().

```~~
(gdb) thread 3
[Switching to thread 3 (Thread 0x7fdf9e2637e0 (LWP 22987))]#0  __lll_lock_wait ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:136
136 2:  movl    %edx, %eax
(gdb) bt
#0  __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:136
#1  0x00007fdf9c3bf508 in _L_lock_854 () from /lib64/libpthread-2.12.so
#2  0x00007fdf9c3bf3d7 in __pthread_mutex_lock (mutex=0x65d450) at pthread_mutex_lock.c:61
#3  0x0000000000422c55 in oh_threaded_stop () at threaded.c:159
#4  0x0000000000412f98 in oh_finit () at init.c:207
#5  0x0000000000412072 in main (argc=1, argv=0x7fff0b978bb8) at openhpid-posix.cpp:422
```~~

A quick and dirty patch might look something like this as the problem seems to lie with the fact the cIpmi instance has been deleted but the pointer stored in the handler struct is not set to NULL.

```~~
$ cat openhpi-3.2.1.patch 
diff -up openhpi-3.2.1/plugins/ipmidirect/ipmi.cpp.fix openhpi-3.2.1/plugins/ipmidirect/ipmi.cpp
--- openhpi-3.2.1/plugins/ipmidirect/ipmi.cpp.fix       2014-02-21 09:38:26.830875375 +1000
+++ openhpi-3.2.1/plugins/ipmidirect/ipmi.cpp   2014-02-21 09:39:05.818649744 +1000
@@ -421,6 +421,7 @@ IpmiClose( void *hnd )
   delete ipmi;

   oh_handler_state *handler = (oh_handler_state *)hnd;
+  handler->data = NULL;

   if ( handler->rptcache )
   {
```~~

Whilst this was seen on an earlier version I checked and the code does not appear to have changed and the conditions for the race still appear to exist in 3.4

Reported by: bradhub
openhpi2 commented 9 years ago

Original comment by: dr_mohan