MCH Driver - Why no thread safety?

c++ / delphi package - dll injection and api hooking
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

MCH Driver - Why no thread safety?

Post by EaSy »

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
madshi
Site Admin
Posts: 10754
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Post by madshi »

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.
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Post by EaSy »

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
madshi
Site Admin
Posts: 10754
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Post by madshi »

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.
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Post by EaSy »

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 (173.88 KiB) Viewed 12683 times
Thanks.
PP
Attachments
Mini022613-01.rar
(16.73 KiB) Downloaded 317 times
madshi
Site Admin
Posts: 10754
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Post by madshi »

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?
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Post by EaSy »

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?
madshi
Site Admin
Posts: 10754
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Post by madshi »

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.
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Post by EaSy »

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.
madshi
Site Admin
Posts: 10754
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Post by madshi »

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.
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Post by EaSy »

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?

Post by EaSy »

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.46 KiB) Downloaded 321 times
madshi
Site Admin
Posts: 10754
Joined: Sun Mar 21, 2004 5:25 pm

Re: MCH Driver - Why no thread safety?

Post by madshi »

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

Re: MCH Driver - Why no thread safety?

Post by madshi »

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!
EaSy
Posts: 150
Joined: Tue Oct 23, 2012 12:33 pm

Re: MCH Driver - Why no thread safety?

Post by EaSy »

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.
Post Reply