MCH Driver - Why no thread safety?

c++ / delphi package - dll injection and api hooking

MCH Driver - Why no thread safety?

Postby EaSy » Wed Feb 27, 2013 8:01 am

Hi,
we are experiencing some BSODs in MadCodeHook driver and I traced origin of last BSOD to this code. To be exact: "dll->IncCount" crashed.
Code: Select all
driver InjectIntoProcess:
...
if ( ((pathLen) || (nameLen)) &&
                 ( ((dll->IncCount) && (!MatchStrArray(pathBuf, nameBuf, pathLen, nameLen, dll->IncCount, dll->IncPathBuf, dll->IncNameBuf, dll->IncPathLen, dll->IncNameLen))) ||
                   ((dll->ExcCount) && ( MatchStrArray(pathBuf, nameBuf, pathLen, nameLen, dll->ExcCount, dll->ExcPathBuf, dll->ExcNameBuf, dll->ExcPathLen, dll->ExcNameLen)))    ) )
...

I wonder why the driver is not thread safe around "PDllItem *DllList" variable. Is it possible that this frequently called code can somehow contribute to BSOD like this?
When we get a new rule we erase the old ones from driver and insert a new one.
Code: Select all
user space InjectAllLibs:
...
   //disable uninjection of running processes
   SetMadCHookOption( UNINJECT_FROM_RUNNING_PROCESSES, (LPCWSTR)0 );

   //delete last session from driver
   UninjectLibraryW(CINJ_DRIVER_NAME, CINJ_DLP_FILE_32, ALL_SESSIONS,   true, ProcessIncludeMaskLast, ProcessExcludeMaskLast);
#ifdef _WIN64
   UninjectLibraryW(CINJ_DRIVER_NAME, CINJ_DLP_FILE_64, ALL_SESSIONS,   true, ProcessIncludeMaskLast, ProcessExcludeMaskLast);
#endif

   //enable uninjection of running processes again
   SetMadCHookOption( UNINJECT_FROM_RUNNING_PROCESSES, (LPCWSTR)1 );
...
   //insert new session into driver
   ret = (InjectLibraryW(CINJ_DRIVER_NAME, CINJ_DLP_FILE_32, ALL_SESSIONS,   true, ProcessIncludeMask, ProcessExcludeMask) != FALSE) && ret;
#ifdef _WIN64
   ret = (InjectLibraryW(CINJ_DRIVER_NAME, CINJ_DLP_FILE_64, ALL_SESSIONS,   true, ProcessIncludeMask, ProcessExcludeMask) != FALSE) && ret;
#endif
...


Thanks
PP
EaSy
 
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Postby madshi » Wed Feb 27, 2013 8:19 am

Please try the latest beta build:

http://madshi.net/madCollectionBeta.exe (2.7.4.12)

I think it will likely fix this issue. Thread related problems could only occur if you call Inject/UninjectLibrary at the same time from the context of 2 different thread, which is rather unlikely. I do have a reworked driver here with added critical sections and some more changes, but I'm planning to use that for madCodeHook 4, because I'm a bit afraid of introducing new bugs in the current version.
madshi
Site Admin
 
Posts: 9759
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Postby EaSy » Wed Feb 27, 2013 9:10 am

What about scenario of new starting application:

-->SwitchContext
[thread1]
ProcessCallback
DriverEvent_NewProcess
InjectIntoProcess
-->SwitchContext
[thread2]
DriverEvent_UninjectionRequest
InjectionUninjectionRequest
DelDll (dll freed)
ExFreePool
-->SwitchContext
[thread1]
dll->IncCount (crash, dll accessed)

Thanks
PP
EaSy
 
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Postby madshi » Wed Feb 27, 2013 9:23 am

Well, I guess that's possible, but somewhat unlikely. In any case, please try the beta build to check if that fixes the problem you had. There was a real bug in the driver, resulting in bluescreens once in a while. I rather think that that is the likely cause of the problems you ran into.
madshi
Site Admin
 
Posts: 9759
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Postby EaSy » Wed Feb 27, 2013 1:12 pm

Well, it may be very unlikely, but it happens...
I have no symbols, but this I have manually decoded it in this screenshot. Dumpfile showed here is attached (.rar file). You can take a look yourself.
MCHdrvbug_symbols2.png
MCHdrvbug_symbols2.png (174.49 KiB) Viewed 5543 times


Thanks.
PP
Attachments
Mini022613-01.rar
(16.78 KiB) Downloaded 129 times
EaSy
 
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Postby madshi » Wed Feb 27, 2013 2:14 pm

This shows the crashing thread. But can you confirm via crash dump analysis that there was another thread which was in DriverEvent_UninjectionRequest at the same time when the crash occurred? Or are you just guessing that that is what happened?

Can you reproduce this crash at will?
madshi
Site Admin
 
Posts: 9759
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Postby EaSy » Thu Feb 28, 2013 8:50 am

I am able to generate "C:\InjectIntoProcess crash" file with my simple stress test case in 5mins in your renameme64.sys driver (2.7.4.12).
Is it enough or should I wait for BSOD caused by some exception that is not "catchable" by those "__try" "__except" handlers like in minidump i posted before?
EaSy
 
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Postby madshi » Thu Feb 28, 2013 9:15 am

Well, the simple question for me is: Should I fix this in a minor madCodeHook 3.x build or not? If I do fix it, I'll have to add some critical sections and stuff and doing such things always comes with a small risk of introducing deadlocks in special situations. So IMHO it's a justified question to ask how likely this problem is to occur in real life. Of course with a stress test targetted to make the driver crash you can reproduce it. But how likely is this to happen in real life? And is fixing it worth the risk of eventually introducing new bugs? Sure, if I do my job right, fixing this should not result in any new bugs. If I had done my job right from the start, this problem would never have occurred in the first place. But the situation is now what it is now.

As I mentioned before, I already have a modified driver sitting here with several added critical sections to make everything totally thread safe. And the driver seems to work just fine. But then, my experience tells me that although the driver appears to be working fine on my PC, in real life there might be situations where the newly added critical sections could eventually produce a deadlock, if I missed something somewhere. So I'm not totally sure whether I should release the modified driver with the next 3.x build or not.
madshi
Site Admin
 
Posts: 9759
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Postby EaSy » Thu Feb 28, 2013 10:24 am

Well, my problem is that this minidump was reported from our testing department and it wasn't the first one and they are definitely not stresstesting it. Otherwise I would not care at all. I am definitely not a mch source code police. :D So it answers your first philosophical question whether it can happen in real life. The answer is yes, it is happening. :?

As for the second one:
1) I can delete this part of code, but that will introduce some complications.
Code: Select all
   //disable uninjection of running processes
   SetMadCHookOption( UNINJECT_FROM_RUNNING_PROCESSES, (LPCWSTR)0 );

   //delete last session from driver
   UninjectLibraryW(CINJ_DRIVER_NAME, CINJ_DLP_FILE_32, ALL_SESSIONS,   true, ProcessIncludeMaskLast, ProcessExcludeMaskLast);
#ifdef _WIN64
   UninjectLibraryW(CINJ_DRIVER_NAME, CINJ_DLP_FILE_64, ALL_SESSIONS,   true, ProcessIncludeMaskLast, ProcessExcludeMaskLast);
#endif

   //enable uninjection of running processes again
   SetMadCHookOption( UNINJECT_FROM_RUNNING_PROCESSES, (LPCWSTR)1 );

2) You can give me some workaround that i don't know about.
3) Mch v4. When is the release date? I can w8 up to 3 weeks.
4) You can fix it in mch v3. We will test it extensively.
EaSy
 
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Postby madshi » Thu Feb 28, 2013 10:39 am

But the thing is that those minidumps from your testing department could also be caused by the bug in the driver I mentioned before (and which should already be fixed in the latest beta build). Actually I find that more likely. I've been trying to tell you that in every single one of my posts, but somehow we're dancing around the issue. You're convinced that the crash in the minidump is caused by a threading issue. I think it's more likely the bug I already fixed. You've not answered my question yet (in one of my earlier comments) whether the minidump really proves that you're right and I'm wrong.

Anyway, I've no problem providing you with a driver which fixes the problem. I'm just not sure if I'll ship it with the next official madCodeHook build.
madshi
Site Admin
 
Posts: 9759
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Postby EaSy » Thu Feb 28, 2013 11:05 am

Ok, we will try to use your latest build. If we encounter more BSODs I'll let you know.

Thanks
PP
EaSy
 
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Postby EaSy » Tue Mar 05, 2013 1:00 pm

I must sadly report that the BSOD is still happenning in the latest http://madshi.net/madCollectionBeta.exe (last modification 13.2.2013?) downloaded last week. Minidump attached is showing the same crash in the same place as the previous one I posted here before.
Attachments
030513-27128-01.rar
(37.6 KiB) Downloaded 120 times
EaSy
 
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Postby madshi » Thu Mar 07, 2013 5:51 pm

Ok, thanks. I'll create a fixed driver and let you know when it's ready for testing.
madshi
Site Admin
 
Posts: 9759
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Postby madshi » Fri Mar 08, 2013 4:36 pm

Here's a new build which should hopefully fix this threading issue:

http://madshi.net/madCollectionBeta.exe (2.7.4.15)

Please let me know if the fix works for you - thanks!
madshi
Site Admin
 
Posts: 9759
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Postby EaSy » Sat Mar 09, 2013 8:50 am

Hi,
I took a quick look at the code and I am sure that a potentional BSOD is still there. It has only moved from "dll->IncCount" into MatchStrArray. Once it tries to access freed "dll->IncPathBuf" or some other allocated buffer, it will BSOD again.
I believe that simple memcpy dll won't cut it. You will have to copy all the allocated data inside or do more proper locking inside functions that work with EnumDlls.
EaSy
 
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Next

Return to madCodeHook

Who is online

Users browsing this forum: No registered users and 6 guests