search

dokan-dev / dokany

User mode file system library for windows with FUSE Wrapper
http://dokan-dev.github.io
5.26k stars 666 forks source link

init.c:DeleteDeviceDelayed() may call ExAcquireResource...() at wrong IRQL #480

Open d-hoke opened 7 years ago

d-hoke commented 7 years ago

Feature request can skip this form. Bug report must complete it. Check List must be 100% match or it will be automatically closed without further discussion. Please remove this line.

Environment

Check List

Description

(From inspection, not currently in position to debug/display the IRQL) Appears DeleteDeviceDelayed() is not called at appropriate IRQL.

DokanDeleteDeviceThread() is created with PsCreateSystemThread(), with such threads apparently generally running at IRQL PASSIVE_LEVEL. DokanDeleteDeviceThread() may call DeleteDeviceDelayed(), which will call ExAcquireResourceExclusiveLite(), WHICH is supposed to be running at an IRQL that will 'disable normal kernel APC delivery', something that PASSIVE_LEVEL apparently does not prevent.

It appears that the ExAcquireResource.../ExReleaseResource... calls should be within a critical region of KeEnterCriticalRegion()/KeLeaveCriticalRegion() which is held for the duration of the 'resource' need. (Apparently some other means of preventing that 'normal kernel APC delivery' would also be acceptable. https://msdn.microsoft.com/en-us/library/windows/hardware/ff543219%28v=vs.85%29.aspx)

Probably worthwhile to add debug info to see if my observation is correct that the routine does not currently run at an appropriate IRQL level to not require the additional critical region calls, before blindly changing them. I'm not currently in a position to attempt that myself, and not sure when, or even if, I might be.)

Logs

Please attach in separate files: mirror output, library logs and kernel logs. In case of BSOD, please attach minidump or dump analyze output.

d-hoke commented 7 years ago

I see now, however, there may be a problem - as IoDeleteSymbolicLink() is apparently supposed to be called at PASSIVE_LEVEL, which I think would not be the case with the KeEnterCritical/KeLeaveCritical enclosure.

Liryna commented 7 years ago

Hi @d-hoke ,

I will do the test and see on which IRQL this is running and will come back to you 👍

Liryna commented 7 years ago

I printed KeGetCurrentIrql in DeleteDeviceDelayed (also after ExAcquireResource) and it is called with PASSIVE_LEVEL 👍 (like expected from PsCreateSystemThread doc)

ExAcquireResource.../ExReleaseResource <= APC_LEVEL https://msdn.microsoft.com/en-us/library/windows/hardware/ff544351(v=vs.85).aspx

Do you see an issue in this ?

d-hoke commented 7 years ago

Acquire/Release <= APC_LEVEL maybe not directly a problem - BUT, docs indicate they SHOULD be called with 'normal kernel APC delivery disabled'. And I don't see where that's happening in current code. Is it but I've overlooked it?

So, IF KeEnterCriticalRegion() addition does NOT actually raise IRQL level, then it needs to be 'surrounding' (along with /KeLeaveCriticalRegion()) the held 'resource' for the complete duration of its 'hold'. And, if it does not raise IRQL, then calling the IoDeleteSymbolicLink() is not a problem, since IRQL presumably would still remain at passive...

Q's to answer: 1)Is there a KeEnterCriticalRegion() 'active' before the 'Acquire' and held until after the 'Release' (that I've overlooked)? If there is one active, then probably all is good as is. 2) But, if there is not one currently active, then does KeEnterCriticalRegion() raise IRQL above the PASSIVE_LEVEL you currently see? If it does NOT raise IRQL, then KeEnterCriticalRegion()/KeLeaveCriticalRegion() can be added appropriately within the routine to surround the holding of the resource, and all should be good. 3)But, if KeEnterCriticalRegion() raises IRQL above the current PASSIVE_LEVEL, all is BAD, as with current code there appears no reasonable way to perform the IoDeleteSymbolicLink() at PASSIVE_LEVEL, since its performed while the resource is held, and the resource must be held with the normal APC delivery disabled...

I'm hoping per (2), that we find KeEnterCriticalRegion() does NOT raise the IRQL, something I haven't (yet) been able to determine either from MSDN docs nor quick searches elsewhere... This post (http://comp.os.ms-windows.programmer.nt.kernel-mode.narkive.com/jKohPf6S/keentercriticalregion-vs-apc-level) does seem to IMPLY normal APC delivery is blocked by KeEnterCriticalRegion() withOUT raising IRQL... Ok, this (https://books.google.com/books?id=w65CAwAAQBAJ&pg=PT200&lpg=PT200&dq=keentercriticalregion+change+irql+level&source=bl&ots=4Ws-p5WhUr&sig=J06AuVXcbj9-bIdH_H7VQrnrgMY&hl=en&sa=X&ved=0ahUKEwiBxa_nn-zSAhWZ0YMKHeq2C6kQ6AEIUjAJ#v=onepage&q=keentercriticalregion%20change%20irql%20level&f=false) seems to further support the notion that (2) is appropriate solution, (1) is not found to already be the case.

d-hoke commented 7 years ago

On WindowsXP, a simple test I performed in a driver learning project indicates that KeEnterCriticalRegion does NOT change the irql from PASSIVE_LEVEL.

So it appears the change suggested in item (2) in my previous post are legal and may be needed, if there is not already a KeEnterCriticalRegion()/KeLeaveCriticalRegion() active that I overlooked...

Liryna commented 7 years ago

Hi @d-hoke ,

Do you think you will be able to make a contribution for this issue ? I can look into https://github.com/dokan-dev/dokany/issues/477 but will not have time for both 😢

d-hoke commented 7 years ago

I can probably code it, but as before, I am not able to build it or test it within its current domain. (I have been hacking at a version to run in WinXP, but do not yet have a functioning version there either.)