> On Tue, Sep 27, 2016 at 3:24 PM, Frediano Ziglio < fziglio at redhat.com >
> wrote:
> > Disable execution bit on mapping improving security.
>
> > MmMapIoSpaceEx is available only in Windows 10 thus
>
> > the macros are used.
>
> > Based on a patch by Sandy Stutsman < sstutsma at redhat.com >
>
> > Signed-off-by: Sameeh Jubran < sameeh at daynix.com >
>
> > ---
>
> > qxldod/QxlDod.cpp | 16 +++++++------
>
> > qxldod/compat.cpp | 52 +++++++++++++++++++++++++++++++++++++++++++
>
> > qxldod/compat.h | 10 +++++++++
>
> > qxldod/qxldod.vcxproj | 2 ++
>
> > qxldod/qxldod.vcxproj.filters | 6 +++++
>
> > 5 files changed, 79 insertions(+), 7 deletions(-)
>
> > create mode 100755 qxldod/compat.cpp
>
> > create mode 100755 qxldod/compat.h
>
> > I send this as a RFC.
>
> > Changes from v5:
>
> > - detect function dynamically and call the proper one.
>
> > This avoid to have to maintain 2 binaries for Windows 8 and Windows 10.
>
> > Perhaps the way dynamic detect is done is a bit overkilling as
>
> > setting up when driver is initialized and a simple if (pMmMapIoSpaceEx)
>
> > could make the function easier.
>
> > I like to have compatibility code in separate files.
>
> > License headers for new files are missing.
>
> > This code should go in the paged area.
>
> > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
>
> > index 0d91f92..3542ab4 100755
>
> > --- a/qxldod/QxlDod.cpp
>
> > +++ b/qxldod/QxlDod.cpp
>
> > @@ -1,6 +1,7 @@
>
> > #include "driver.h"
>
> > #include "qxldod.h"
>
> > #include "qxl_windows.h"
>
> > +#include "compat.h"
>
> > #pragma code_seg(push)
>
> > #pragma code_seg()
>
> > @@ -1998,17 +1999,18 @@ MapFrameBuffer(
>
> > return STATUS_INVALID_PARAMETER;
>
> > }
>
> > - *VirtualAddress = MmMapIoSpace(PhysicalAddress,
>
> > - Length,
>
> > - MmWriteCombined);
>
> > + *VirtualAddress = MapIoSpace(PhysicalAddress,
>
> > + Length,
>
> > + MmWriteCombined,
>
> > + PAGE_WRITECOMBINE | PAGE_READWRITE);
>
> > if (*VirtualAddress == NULL)
>
> > {
>
> > // The underlying call to MmMapIoSpace failed. This may be because,
> > MmWriteCombined
>
> > // isn't supported, so try again with MmNonCached
>
> > -
>
> > - *VirtualAddress = MmMapIoSpace(PhysicalAddress,
>
> > - Length,
>
> > - MmNonCached);
>
> > + *VirtualAddress = MapIoSpace(PhysicalAddress,
>
> > + Length,
>
> > + MmNonCached,
>
> > + PAGE_NOCACHE | PAGE_READWRITE);
>
> > if (*VirtualAddress == NULL)
>
> > {
>
> > DbgPrint(TRACE_LEVEL_ERROR, ("MmMapIoSpace returned a NULL buffer when
> > trying
> > to allocate %lu bytes", Length));
>
> > diff --git a/qxldod/compat.cpp b/qxldod/compat.cpp
>
> > new file mode 100755
>
> > index 0000000..7ca7ae5
>
> > --- /dev/null
>
> > +++ b/qxldod/compat.cpp
>
> > @@ -0,0 +1,52 @@
>
> > +#include "driver.h"
>
> > +#include "compat.h"
>
> > +
>
> > +static MapIoSpaceFunc DetectMapIoSpace;
>
> > +MapIoSpaceFunc *MapIoSpace = DetectMapIoSpace;
>
> > +
>
> > +typedef NTKERNELAPI PVOID MmMapIoSpaceExFunc(
>
> > + _In_ PHYSICAL_ADDRESS PhysicalAddress,
>
> > + _In_ SIZE_T NumberOfBytes,
>
> > + _In_ ULONG Protect
>
> > +);
>
> > +static MmMapIoSpaceExFunc *pMmMapIoSpaceEx;
>
> > +
>
> > +static PVOID OldMapIoSpace(
>
> > + _In_ PHYSICAL_ADDRESS PhysicalAddress,
>
> > + _In_ SIZE_T NumberOfBytes,
>
> > + _In_ MEMORY_CACHING_TYPE CacheType,
>
> > + _In_ ULONG Protect
>
> > +)
>
> > +{
>
> > + if (NumberOfBytes == 0) return NULL;
>
This was only for debuggin, I'll remove it.
> > + return MmMapIoSpace(PhysicalAddress, NumberOfBytes, CacheType);
>
> > +}
>
> > +
>
> > +static PVOID NewMapIoSpace(
>
> > + _In_ PHYSICAL_ADDRESS PhysicalAddress,
>
> > + _In_ SIZE_T NumberOfBytes,
>
> > + _In_ MEMORY_CACHING_TYPE CacheType,
>
> > + _In_ ULONG Protect
>
> > +)
>
> > +{
>
> > + return pMmMapIoSpaceEx(PhysicalAddress, NumberOfBytes, Protect);
>
> > +}
>
> > +
>
> > +static PVOID DetectMapIoSpace(
>
> > + _In_ PHYSICAL_ADDRESS PhysicalAddress,
>
> > + _In_ SIZE_T NumberOfBytes,
>
> > + _In_ MEMORY_CACHING_TYPE CacheType,
>
> > + _In_ ULONG Protect
>
> > +)
>
> > +{
>
> > + UNICODE_STRING name;
>
> > + RtlInitUnicodeString(&name, L"MmMapIoSpaceEx");
>
> > +
>
> > + pMmMapIoSpaceEx = (MmMapIoSpaceExFunc*)MmGetSystemRoutineAddress(&name);
>
> I think it is better to save this pointer instead of calling
> MmGetSystemRoutineAddress each time
> we call this function.
> Other than that:
> Acked-by: Sameeh Jubran < sameeh at daynix.com >
Yes, but this function is called just once.
The function is called only using MapIoSpace pointer which initially is DetectMapIoSpace.
However DetectMapIoSpace change MapIoSpace pointer to be NewMapIoSpace or OldMapIoSpace so
next time you'll call one of these functions.
Somebody could argue that if two thread call MapIoSpace you could enter the function twice but
this is not an issue as the pointer you are going to write to MapIoSpace is the same.
I use this way as usually I replace a new function with a function that detect and implement it if not
available. In this way once is detected is faster as calling the API directly. This as the API import store
a pointer into a variable so you always have the indirect call. This actually could even be faster as some
import way could produce a direct call to an indirect jump.
Yes, pure paranoia!
> > + if (pMmMapIoSpaceEx) {
>
> > + MapIoSpace = NewMapIoSpace;
>
> > + } else {
>
> > + MapIoSpace = OldMapIoSpace;
>
> > + }
>
> > + return MapIoSpace(PhysicalAddress, NumberOfBytes, CacheType, Protect);
>
> > +}
>
> > diff --git a/qxldod/compat.h b/qxldod/compat.h
>
> > new file mode 100755
>
> > index 0000000..3f20b81
>
> > --- /dev/null
>
> > +++ b/qxldod/compat.h
>
> > @@ -0,0 +1,10 @@
>
> > +#pragma once
>
> > +#include "BaseObject.h"
>
> > +
>
> > +typedef PVOID MapIoSpaceFunc(
>
> > + _In_ PHYSICAL_ADDRESS PhysicalAddress,
>
> > + _In_ SIZE_T NumberOfBytes,
>
> > + _In_ MEMORY_CACHING_TYPE CacheType,
>
> > + _In_ ULONG Protect
>
> > +);
>
> > +extern MapIoSpaceFunc *MapIoSpace;
>
> > diff --git a/qxldod/qxldod.vcxproj b/qxldod/qxldod.vcxproj
>
> > index 37d2b38..2c10158 100755
>
> > --- a/qxldod/qxldod.vcxproj
>
> > +++ b/qxldod/qxldod.vcxproj
>
> > @@ -272,12 +272,14 @@
>
> > </ItemGroup>
>
> > <ItemGroup>
>
> > <ClInclude Include="BaseObject.h" />
>
> > + <ClInclude Include="compat.h" />
>
> > <ClInclude Include="driver.h" />
>
> > <ClInclude Include="QxlDod.h" />
>
> > <ClInclude Include="resource.h" />
>
> > </ItemGroup>
>
> > <ItemGroup>
>
> > <ClCompile Include="BaseObject.cpp" />
>
> > + <ClCompile Include="compat.cpp" />
>
> > <ClCompile Include="driver.cpp" />
>
> > <ClCompile Include="mspace.c" />
>
> > <ClCompile Include="QxlDod.cpp" />
>
> > diff --git a/qxldod/qxldod.vcxproj.filters b/qxldod/qxldod.vcxproj.filters
>
> > index bb9daa9..1e86aa6 100755
>
> > --- a/qxldod/qxldod.vcxproj.filters
>
> > +++ b/qxldod/qxldod.vcxproj.filters
>
> > @@ -25,6 +25,9 @@
>
> > <ClInclude Include="resource.h">
>
> > <Filter>Header Files</Filter>
>
> > </ClInclude>
>
> > + <ClInclude Include="compat.h">
>
> > + <Filter>Header Files</Filter>
>
> > + </ClInclude>
>
> > <ClInclude Include="driver.h">
>
> > <Filter>Header Files</Filter>
>
> > </ClInclude>
>
> > @@ -36,6 +39,9 @@
>
> > <ClCompile Include="BaseObject.cpp">
>
> > <Filter>Source Files</Filter>
>
> > </ClCompile>
>
> > + <ClCompile Include="compat.cpp">
>
> > + <Filter>Source Files</Filter>
>
> > + </ClCompile>
>
> > <ClCompile Include="driver.cpp">
>
> > <Filter>Source Files</Filter>
>
> > </ClCompile>
>
I'll send an update adding PAGED_CODE() "calls".
Frediano
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160928/ddcdd9cc/attachment-0001.html>