Re: [ros-dev] [ros-diffs] [pschweitzer] 69221: [MOUNTMGR] Implement the IOCTL IOCTL_MOUNTMGR_VOLUME_MOUNT_POINT_CREATED: - Implement WriteRemoteDatabaseEntry() - Implement MountMgrVolumeMountPointCrea

2015-09-15 Thread Alex Ionescu
yes

Best regards,
Alex Ionescu

On Mon, Sep 14, 2015 at 3:16 AM, Pierre Schweitzer 
wrote:

> Refering to CVE-2015-1769/MS15-085?
>
> On 14/09/2015 05:24, Alex Ionescu wrote:
> > Lol, make sure not to implement the huge vulnerability Microsoft patched
> > two months ago (win2k->xp-style database migration).
> >
> > Best regards,
> > Alex Ionescu
> >
> > On Sun, Sep 13, 2015 at 6:52 PM,  wrote:
> >
> >> Author: pschweitzer
> >> Date: Sun Sep 13 22:52:07 2015
> >> New Revision: 69221
> >>
> >> URL: http://svn.reactos.org/svn/reactos?rev=69221&view=rev
> >> Log:
> >> [MOUNTMGR]
> >> Implement the IOCTL IOCTL_MOUNTMGR_VOLUME_MOUNT_POINT_CREATED:
> >> - Implement WriteRemoteDatabaseEntry()
> >> - Implement MountMgrVolumeMountPointCreated()
> >>
> >> Modified:
> >> trunk/reactos/drivers/filters/mountmgr/database.c
> >> trunk/reactos/drivers/filters/mountmgr/device.c
> >> trunk/reactos/drivers/filters/mountmgr/mntmgr.h
> >> trunk/reactos/drivers/filters/mountmgr/mountmgr.c
> >>
> >> Modified: trunk/reactos/drivers/filters/mountmgr/database.c
> >> URL:
> >>
> http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filters/mountmgr/database.c?rev=69221&r1=69220&r2=69221&view=diff
> >>
> >>
> ==
> >> --- trunk/reactos/drivers/filters/mountmgr/database.c   [iso-8859-1]
> >> (original)
> >> +++ trunk/reactos/drivers/filters/mountmgr/database.c   [iso-8859-1] Sun
> >> Sep 13 22:52:07 2015
> >> @@ -192,6 +192,39 @@
> >>  }
> >>
> >>  return Entry;
> >> +}
> >> +
> >> +/*
> >> + * @implemented
> >> + */
> >> +NTSTATUS
> >> +WriteRemoteDatabaseEntry(IN HANDLE Database,
> >> + IN LONG Offset,
> >> + IN PDATABASE_ENTRY Entry)
> >> +{
> >> +NTSTATUS Status;
> >> +LARGE_INTEGER ByteOffset;
> >> +IO_STATUS_BLOCK IoStatusBlock;
> >> +
> >> +ByteOffset.QuadPart = Offset;
> >> +Status = ZwWriteFile(Database,
> >> + NULL,
> >> + NULL,
> >> + NULL,
> >> + &IoStatusBlock,
> >> + Entry,
> >> + Entry->EntrySize,
> >> + &ByteOffset,
> >> + NULL);
> >> +if (NT_SUCCESS(Status))
> >> +{
> >> +if (IoStatusBlock.Information < Entry->EntrySize)
> >> +{
> >> +Status = STATUS_INSUFFICIENT_RESOURCES;
> >> +}
> >> +}
> >> +
> >> +return Status;
> >>  }
> >>
> >>  /*
> >>
> >> Modified: trunk/reactos/drivers/filters/mountmgr/device.c
> >> URL:
> >>
> http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filters/mountmgr/device.c?rev=69221&r1=69220&r2=69221&view=diff
> >>
> >>
> ==
> >> --- trunk/reactos/drivers/filters/mountmgr/device.c [iso-8859-1]
> >> (original)
> >> +++ trunk/reactos/drivers/filters/mountmgr/device.c [iso-8859-1] Sun
> >> Sep 13 22:52:07 2015
> >> @@ -1688,15 +1688,242 @@
> >>  return Status;
> >>  }
> >>
> >> +/*
> >> + * @implemented
> >> + */
> >>  NTSTATUS
> >>  MountMgrVolumeMountPointCreated(IN PDEVICE_EXTENSION DeviceExtension,
> >>  IN PIRP Irp,
> >>  IN NTSTATUS LockStatus)
> >>  {
> >> -UNREFERENCED_PARAMETER(DeviceExtension);
> >> -UNREFERENCED_PARAMETER(Irp);
> >> -UNREFERENCED_PARAMETER(LockStatus);
> >> -return STATUS_NOT_IMPLEMENTED;
> >> +LONG Offset;
> >> +BOOLEAN Found;
> >> +NTSTATUS Status;
> >> +HANDLE RemoteDatabase;
> >> +PMOUNTDEV_UNIQUE_ID UniqueId;
> >> +PDATABASE_ENTRY DatabaseEntry;
> >> +PASSOCIATED_DEVICE_ENTRY AssociatedEntry;
> >> +PDEVICE_INFORMATION DeviceInformation, TargetDeviceInformation;
> >> +UNICODE_STRING LinkTarget, SourceDeviceName, SourceSymbolicName,
> >> TargetVolumeName, VolumeName, DbName;
> >> +
> >> +/* Initialize string */
> >> +LinkTarget.Length = 0;
> >> +LinkTarget.MaximumLength = 0xC8;
> >> +LinkTarget.Buffer = AllocatePool(LinkTarget.MaximumLength);
> >> +if (LinkTarget.Buffer == NULL)
> >> +{
> >> +return STATUS_INSUFFICIENT_RESOURCES;
> >> +}
> >> +
> >> +/* If the mount point was created, then, it changed!
> >> + * Also use it to query some information
> >> + */
> >> +Status = MountMgrVolumeMountPointChanged(DeviceExtension, Irp,
> >> LockStatus, &SourceDeviceName, &SourceSymbolicName, &TargetVolumeName);
> >> +/* Pending means DB are under synchronization, bail out */
> >> +if (Status == STATUS_PENDING)
> >> +{
> >> +FreePool(LinkTarget.Buffer);
> >> +FreePool(SourceDeviceName.Buffer);
> >> +FreePool(SourceSymbolicName.Buffer);
> >> +return STATUS_PENDING;
> >> +}
> >> +else if (!NT_SUCCESS(Status))
> >> +{
> >> +FreePool(LinkTarget.Buffer);
>

Re: [ros-dev] [ros-diffs] [tfaber] 69236: [MSFS] - Use a NULL timeout for infinite waits instead of waiting for 100 ns. CORE-10188 #resolve - Wait for available read data in user mode to handle thread

2015-09-15 Thread Alex Ionescu
I'm not convinced on the user->kernel changes Thomas.

Best regards,
Alex Ionescu

On Tue, Sep 15, 2015 at 5:40 AM,  wrote:

> Author: tfaber
> Date: Tue Sep 15 09:40:30 2015
> New Revision: 69236
>
> URL: http://svn.reactos.org/svn/reactos?rev=69236&view=rev
> Log:
> [MSFS]
> - Use a NULL timeout for infinite waits instead of waiting for 100 ns.
> CORE-10188 #resolve
> - Wait for available read data in user mode to handle thread termination
> - Return STATUS_IO_TIMEOUT also for a zero-length timeout. Fixes Wine tests
> - Avoid MmGetSystemAddressForMdl
> - Acquiring a mutex is not a UserRequest
>
> Modified:
> trunk/reactos/drivers/filesystems/msfs/msfs.h
> trunk/reactos/drivers/filesystems/msfs/rw.c
>
> Modified: trunk/reactos/drivers/filesystems/msfs/msfs.h
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/msfs/msfs.h?rev=69236&r1=69235&r2=69236&view=diff
>
> ==
> --- trunk/reactos/drivers/filesystems/msfs/msfs.h   [iso-8859-1]
> (original)
> +++ trunk/reactos/drivers/filesystems/msfs/msfs.h   [iso-8859-1] Tue
> Sep 15 09:40:30 2015
> @@ -54,7 +54,7 @@
>
>
>  #define KeLockMutex(x) KeWaitForSingleObject(x, \
> - UserRequest, \
> + Executive, \
>   KernelMode, \
>   FALSE, \
>   NULL);
>
> Modified: trunk/reactos/drivers/filesystems/msfs/rw.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/msfs/rw.c?rev=69236&r1=69235&r2=69236&view=diff
>
> ==
> --- trunk/reactos/drivers/filesystems/msfs/rw.c [iso-8859-1] (original)
> +++ trunk/reactos/drivers/filesystems/msfs/rw.c [iso-8859-1] Tue Sep 15
> 09:40:30 2015
> @@ -29,6 +29,7 @@
>  ULONG LengthRead = 0;
>  PVOID Buffer;
>  NTSTATUS Status;
> +PLARGE_INTEGER Timeout;
>
>  DPRINT("MsfsRead(DeviceObject %p Irp %p)\n", DeviceObject, Irp);
>
> @@ -52,16 +53,21 @@
>
>  Length = IoStack->Parameters.Read.Length;
>  if (Irp->MdlAddress)
> -Buffer = MmGetSystemAddressForMdl (Irp->MdlAddress);
> +Buffer = MmGetSystemAddressForMdlSafe(Irp->MdlAddress,
> NormalPagePriority);
>  else
>  Buffer = Irp->UserBuffer;
>
> +if (Fcb->TimeOut.QuadPart == -1LL)
> +Timeout = NULL;
> +else
> +Timeout = &Fcb->TimeOut;
> +
>  Status = KeWaitForSingleObject(&Fcb->MessageEvent,
> UserRequest,
> -   KernelMode,
> +   UserMode,
> FALSE,
> -   &Fcb->TimeOut);
> -if (NT_SUCCESS(Status))
> +   Timeout);
> +if (Status != STATUS_USER_APC)
>  {
>  if (Fcb->MessageCount > 0)
>  {
> @@ -84,7 +90,7 @@
>  KeClearEvent(&Fcb->MessageEvent);
>  }
>  }
> -else if (Fcb->TimeOut.QuadPart != 0LL)
> +else
>  {
>  /* No message found after waiting */
>  Status = STATUS_IO_TIMEOUT;
> @@ -135,7 +141,7 @@
>
>  Length = IoStack->Parameters.Write.Length;
>  if (Irp->MdlAddress)
> -Buffer = MmGetSystemAddressForMdl (Irp->MdlAddress);
> +Buffer = MmGetSystemAddressForMdlSafe(Irp->MdlAddress,
> NormalPagePriority);
>  else
>  Buffer = Irp->UserBuffer;
>
>
>
>
___
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

Re: [ros-dev] [ros-diffs] [sginsberg] 69248: [HAL] Get rid off REGISTERCALL ("regparm(3)" for GCC) and replace with good old NTAPI (stdcall). Such an obsession with avoiding stack usage is not healthy

2015-09-15 Thread Alex Ionescu
You seem to have a huge misunderstanding of modern processor architecture.
Everything you said is false. In fact, this is why x64 uses register
passing, as does ARM and every other major architecture except x86.
Push/pop operations are extremely expensive and break pipelining. The
compiler will then place the parameters in registers anyways after the fact.

Suffice it to say though, the original reason these were regparm/fastcall
is to easily allow them to be called from assembly at the time.

Best regards,
Alex Ionescu

On Tue, Sep 15, 2015 at 7:03 PM,  wrote:

> Author: sginsberg
> Date: Tue Sep 15 23:03:42 2015
> New Revision: 69248
>
> URL: http://svn.reactos.org/svn/reactos?rev=69248&view=rev
> Log:
> [HAL] Get rid off REGISTERCALL ("regparm(3)" for GCC) and replace with
> good old NTAPI (stdcall). Such an obsession with avoiding stack usage is
> not healthy nor makes much sense today (or even a long time before today)
> with processors that have a decent L1 cache, whose "cost of access" is
> basically the same as to that of a register, and with processors being
> capable of recognising basic access patterns to ensure frequently used
> memory (read: stack) is in the cache. Slapping FASTCALL/regparm on
> frequently used code does not ensure it actually operates faster. You want
> to know what really hurts performance (and cache)? Slapping FORCEINLINE on
> everything like if it was some kind of asm macro, making code needlessly
> bloated.
>
> Modified:
> trunk/reactos/hal/halx86/include/halp.h
> trunk/reactos/hal/halx86/up/pic.c
>
> Modified: trunk/reactos/hal/halx86/include/halp.h
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/hal/halx86/include/halp.h?rev=69248&r1=69247&r2=69248&view=diff
>
> ==
> --- trunk/reactos/hal/halx86/include/halp.h [iso-8859-1] (original)
> +++ trunk/reactos/hal/halx86/include/halp.h [iso-8859-1] Tue Sep 15
> 23:03:42 2015
> @@ -10,12 +10,6 @@
>  #define INIT_SECTION /* Done via alloc_text for MSC */
>  #endif
>
> -
> -#ifdef _MSC_VER
> -#define REGISTERCALL FASTCALL
> -#else
> -#define REGISTERCALL __attribute__((regparm(3)))
> -#endif
>
>  #ifdef CONFIG_SMP
>  #define HAL_BUILD_TYPE (DBG ? PRCB_BUILD_DEBUG : 0)
> @@ -430,14 +424,14 @@
>
>  typedef
>  BOOLEAN
> -( REGISTERCALL *PHAL_DISMISS_INTERRUPT)(
> +(NTAPI *PHAL_DISMISS_INTERRUPT)(
>  IN KIRQL Irql,
>  IN ULONG Irq,
>  OUT PKIRQL OldIrql
>  );
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrqGeneric(
>  IN KIRQL Irql,
>  IN ULONG Irq,
> @@ -445,7 +439,7 @@
>  );
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq15(
>  IN KIRQL Irql,
>  IN ULONG Irq,
> @@ -453,7 +447,7 @@
>  );
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq13(
>  IN KIRQL Irql,
>  IN ULONG Irq,
> @@ -461,7 +455,7 @@
>  );
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq07(
>  IN KIRQL Irql,
>  IN ULONG Irq,
> @@ -469,7 +463,7 @@
>  );
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrqLevel(
>  IN KIRQL Irql,
>  IN ULONG Irq,
> @@ -477,7 +471,7 @@
>  );
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq15Level(
>  IN KIRQL Irql,
>  IN ULONG Irq,
> @@ -485,7 +479,7 @@
>  );
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq13Level(
>  IN KIRQL Irql,
>  IN ULONG Irq,
> @@ -493,7 +487,7 @@
>  );
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq07Level(
>  IN KIRQL Irql,
>  IN ULONG Irq,
>
> Modified: trunk/reactos/hal/halx86/up/pic.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/hal/halx86/up/pic.c?rev=69248&r1=69247&r2=69248&view=diff
>
> ==
> --- trunk/reactos/hal/halx86/up/pic.c   [iso-8859-1] (original)
> +++ trunk/reactos/hal/halx86/up/pic.c   [iso-8859-1] Tue Sep 15 23:03:42
> 2015
> @@ -843,7 +843,7 @@
>  }
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrqGeneric(IN KIRQL Irql,
>IN ULONG Irq,
>OUT PKIRQL OldIrql)
> @@ -853,7 +853,7 @@
>  }
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq15(IN KIRQL Irql,
>   IN ULONG Irq,
>   OUT PKIRQL OldIrql)
> @@ -889,7 +889,7 @@
>
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq13(IN KIRQL Irql,
>   IN ULONG Irq,
>   OUT PKIRQL OldIrql)
> @@ -902,7 +902,7 @@
>  }
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq07(IN KIRQL Irql,
>   IN ULONG Irq,
>   OUT PKIRQL OldIrql)
> @@ -986,7 +986,7 @@
>  }
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrqLevel(IN KIRQL Irql,
>  IN ULONG Irq,
>  OUT PKIRQL OldIrql)
> @@ -996,7 +996,7 @@
>  }
>
>  BOOLEAN
> -REGISTERCALL
> +NTAPI
>  HalpDismissIrq15Level(IN KIRQL Irql,
>IN ULONG Irq,
>OUT PKIRQL OldIrql)
>