[ros-kernel] Bugs in NtRead/WriteFile (Eric)

[Gunnar André Dalsnes]

> 
> I had a similar problem with synchronous io operations in the 
> disk format
> code of usetup.exe (fslib). IoStatusBlock, which was 
> allocated from the
> user-mode stack, was out of scope when the disk driver 
> completed a write
> operation. Usetup.exe crashed in kernel mode.

Huh? If the operation was synchronous, NtWriteFile should wait for io to
complete thus it's not possible for IoStatusBlock to get out of scope...

And where did usetup.exe crash? In
IoSecondStageCompletion->*Irp->UserIosb=Irp->IoStatus ????

> 
> For synchronous operations, a safe IoStatusBlock should be 
> allocated from
> non-paged pool and use the MmSafeCopy* routines to copy the 
> io status upon
> completion.
> 
> For asynchronous operations, a pointer to the original 
> IoStatus Block should
> be passed to the Irp.
> 
> Can you agree with that?

Well...

Allocating IoStatusBlock (Irp->UserIosb) from NonPagedPool is not a
requirement, so it seems to me you are trying to work-around another bug. 

We should do a MmSafeCopyToUserMode in IoSecondStageCompletion when doing
*Irp->UserIosb=Irp->IoStatus, if the irp originated from umode. This makes
it unnecessary (and incorrect) to use a local "safe" variable for
IoStatusBlock (Irp->UserIosb) no matter what kind of operation
(asynch/synch).

> 
> 

I think the real bug is that IoSecondStageCompletion are sometimes, for
synchronous operations, called at DISPATCH_LEVEL. Copying read data back to
umode and copying the Irp->IoStatus to the umode buffer in Irp->UserIosb
should fail in this case, but strangely, this seems to work ok (i have never
seen it crash). You might have been "unlucky", having the umode stack paged
out while waiting for the write to complete, making usetup.exe crash in
IoSecondStageCompletion->*Irp->UserIosb=Irp->IoStatus.

Regards,
Gunnar