Fix MTRR buffer overflow, improve compatibility with VS Preview

This issue was privately reported by Petr Beneš (@PetrBenes).
tandasat · Jul 23, 2018 · e28bec2 · e28bec2
1 parent 97d6ccc
commit e28bec2
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 18 deletions.
diff --git a/HyperPlatform/HyperPlatform.ruleset b/HyperPlatform/HyperPlatform.ruleset
@@ -0,0 +1,81 @@
+<?xml version="1.0" encoding="utf-8"?>
+<RuleSet Name="Rules for HyperPlatform" Description="Code analysis rules for HyperPlatform.vcxproj." ToolsVersion="15.0">
+  <IncludeAll Action="Warning" />
+  <Rules AnalyzerId="Microsoft.Analyzers.NativeCodeAnalysis" RuleNamespace="Microsoft.Rules.Native">
+    <Rule Id="C26400" Action="None" />
+    <Rule Id="C26401" Action="None" />
+    <Rule Id="C26402" Action="None" />
+    <Rule Id="C26403" Action="None" />
+    <Rule Id="C26404" Action="None" />
+    <Rule Id="C26405" Action="None" />
+    <Rule Id="C26406" Action="None" />
+    <Rule Id="C26407" Action="None" />
+    <Rule Id="C26408" Action="None" />
+    <Rule Id="C26409" Action="None" />
+    <Rule Id="C26410" Action="None" />
+    <Rule Id="C26411" Action="None" />
+    <Rule Id="C26414" Action="None" />
+    <Rule Id="C26415" Action="None" />
+    <Rule Id="C26416" Action="None" />
+    <Rule Id="C26417" Action="None" />
+    <Rule Id="C26418" Action="None" />
+    <Rule Id="C26426" Action="None" />
+    <Rule Id="C26427" Action="None" />
+    <Rule Id="C26429" Action="None" />
+    <Rule Id="C26430" Action="None" />
+    <Rule Id="C26431" Action="None" />
+    <Rule Id="C26432" Action="None" />
+    <Rule Id="C26433" Action="None" />
+    <Rule Id="C26434" Action="None" />
+    <Rule Id="C26435" Action="None" />
+    <Rule Id="C26436" Action="None" />
+    <Rule Id="C26437" Action="None" />
+    <Rule Id="C26438" Action="None" />
+    <Rule Id="C26439" Action="None" />
+    <Rule Id="C26440" Action="None" />
+    <Rule Id="C26441" Action="None" />
+    <Rule Id="C26443" Action="None" />
+    <Rule Id="C26444" Action="None" />
+    <Rule Id="C26445" Action="None" />
+    <Rule Id="C26446" Action="None" />
+    <Rule Id="C26447" Action="None" />
+    <Rule Id="C26448" Action="None" />
+    <Rule Id="C26449" Action="None" />
+    <Rule Id="C26450" Action="None" />
+    <Rule Id="C26451" Action="None" />
+    <Rule Id="C26452" Action="None" />
+    <Rule Id="C26453" Action="None" />
+    <Rule Id="C26454" Action="None" />
+    <Rule Id="C26459" Action="None" />
+    <Rule Id="C26460" Action="None" />
+    <Rule Id="C26461" Action="None" />
+    <Rule Id="C26462" Action="None" />
+    <Rule Id="C26463" Action="None" />
+    <Rule Id="C26464" Action="None" />
+    <Rule Id="C26465" Action="None" />
+    <Rule Id="C26466" Action="None" />
+    <Rule Id="C26471" Action="None" />
+    <Rule Id="C26472" Action="None" />
+    <Rule Id="C26473" Action="None" />
+    <Rule Id="C26474" Action="None" />
+    <Rule Id="C26475" Action="None" />
+    <Rule Id="C26476" Action="None" />
+    <Rule Id="C26477" Action="None" />
+    <Rule Id="C26481" Action="None" />
+    <Rule Id="C26482" Action="None" />
+    <Rule Id="C26483" Action="None" />
+    <Rule Id="C26485" Action="None" />
+    <Rule Id="C26486" Action="None" />
+    <Rule Id="C26487" Action="None" />
+    <Rule Id="C26489" Action="None" />
+    <Rule Id="C26490" Action="None" />
+    <Rule Id="C26491" Action="None" />
+    <Rule Id="C26492" Action="None" />
+    <Rule Id="C26493" Action="None" />
+    <Rule Id="C26494" Action="None" />
+    <Rule Id="C26495" Action="None" />
+    <Rule Id="C26496" Action="None" />
+    <Rule Id="C26497" Action="None" />
+    <Rule Id="C26498" Action="None" />
+  </Rules>
+</RuleSet>
diff --git a/HyperPlatform/HyperPlatform.vcxproj b/HyperPlatform/HyperPlatform.vcxproj
@@ -71,28 +71,29 @@
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <DebuggerFlavor>DbgengKernelDebugger</DebuggerFlavor>
     <Inf2CatUseLocalTime>true</Inf2CatUseLocalTime>
-    <CodeAnalysisRuleSet>AllRules.ruleset</CodeAnalysisRuleSet>
+    <CodeAnalysisRuleSet>HyperPlatform.ruleset</CodeAnalysisRuleSet>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
     <DebuggerFlavor>DbgengKernelDebugger</DebuggerFlavor>
     <Inf2CatUseLocalTime>true</Inf2CatUseLocalTime>
-    <CodeAnalysisRuleSet>AllRules.ruleset</CodeAnalysisRuleSet>
+    <CodeAnalysisRuleSet>HyperPlatform.ruleset</CodeAnalysisRuleSet>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
     <DebuggerFlavor>DbgengKernelDebugger</DebuggerFlavor>
     <Inf2CatUseLocalTime>true</Inf2CatUseLocalTime>
-    <CodeAnalysisRuleSet>AllRules.ruleset</CodeAnalysisRuleSet>
+    <CodeAnalysisRuleSet>HyperPlatform.ruleset</CodeAnalysisRuleSet>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
     <DebuggerFlavor>DbgengKernelDebugger</DebuggerFlavor>
     <Inf2CatUseLocalTime>true</Inf2CatUseLocalTime>
-    <CodeAnalysisRuleSet>AllRules.ruleset</CodeAnalysisRuleSet>
+    <CodeAnalysisRuleSet>HyperPlatform.ruleset</CodeAnalysisRuleSet>
   </PropertyGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <ClCompile>
       <WppRecorderEnabled>true</WppRecorderEnabled>
       <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">trace.h</WppScanConfigurationData>
       <WppKernelMode>true</WppKernelMode>
+      <SupportJustMyCode>false</SupportJustMyCode>
     </ClCompile>
     <Link />
   </ItemDefinitionGroup>
@@ -109,6 +110,7 @@
       <WppRecorderEnabled>true</WppRecorderEnabled>
       <WppScanConfigurationData Condition="'%(ClCompile.ScanConfigurationData)' == ''">trace.h</WppScanConfigurationData>
       <WppKernelMode>true</WppKernelMode>
+      <SupportJustMyCode>false</SupportJustMyCode>
     </ClCompile>
     <Link />
   </ItemDefinitionGroup>
@@ -168,6 +170,9 @@
   <ItemGroup>
     <Inf Include="HyperPlatform.inf" />
   </ItemGroup>
+  <ItemGroup>
+    <None Include="HyperPlatform.ruleset" />
+  </ItemGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
   <ImportGroup Label="ExtensionTargets">
   </ImportGroup>

diff --git a/HyperPlatform/HyperPlatform.vcxproj.filters b/HyperPlatform/HyperPlatform.vcxproj.filters
@@ -113,4 +113,7 @@
       <Filter>Source Files</Filter>
     </MASM>
   </ItemGroup>
+  <ItemGroup>
+    <None Include="HyperPlatform.ruleset" />
+  </ItemGroup>
 </Project>
diff --git a/HyperPlatform/ept.cpp b/HyperPlatform/ept.cpp
@@ -54,9 +54,11 @@ static const auto kEptpNumberOfPreallocatedEntries = 50;
 // Architecture defined number of variable range MTRRs
 static const auto kEptpNumOfMaxVariableRangeMtrrs = 255;
 
-// Architecture defined number of fixed range MTRRs (1 for 64k, 2 for 16k, 8
-// for 4k)
-static const auto kEptpNumOfFixedRangeMtrrs = 1 + 2 + 8;
+// Architecture defined number of fixed range MTRRs. 1 register for 64k, 2
+// registers for 16k, 8 registers for 4k, and each register has 8 ranges as per
+// "Fixed Range MTRRs" states.
+static const auto kEptpNumOfFixedRangeMtrrs =
+    (1 + 2 + 8) * RTL_NUMBER_OF_FIELD(Ia32MtrrFixedRangeMsr, fields.types);
 
 // A size of array to store all possible MTRRs
 static const auto kEptpMtrrEntriesSize =
@@ -105,11 +107,10 @@ _When_(ept_data == nullptr,
 static void EptpDestructTables(_In_ EptCommonEntry *table,
                                _In_ ULONG table_level);
 
-_Must_inspect_result_
-    _When_(ept_data == nullptr,
-           __drv_allocatesMem(Mem)
-           _IRQL_requires_max_(DISPATCH_LEVEL)) static EptCommonEntry
-        *EptpAllocateEptEntry(_In_opt_ EptData *ept_data);
+_Must_inspect_result_ _When_(ept_data == nullptr,
+                             __drv_allocatesMem(Mem) _IRQL_requires_max_(
+                                 DISPATCH_LEVEL)) static EptCommonEntry
+    *EptpAllocateEptEntry(_In_opt_ EptData *ept_data);
 
 static EptCommonEntry *EptpAllocateEptEntryFromPreAllocated(
     _In_ EptData *ept_data);
@@ -203,7 +204,8 @@ _Use_decl_annotations_ void EptInitializeMtrrEntries() {
   Ia32MtrrCapabilitiesMsr mtrr_capabilities = {
       UtilReadMsr64(Msr::kIa32MtrrCap)};
   HYPERPLATFORM_LOG_DEBUG(
-      "MTRR Default=%llu, VariableCount=%llu, FixedSupported=%llu, FixedEnabled=%llu",
+      "MTRR Default=%llu, VariableCount=%llu, FixedSupported=%llu, "
+      "FixedEnabled=%llu",
       default_type.fields.default_mtemory_type,
       mtrr_capabilities.fields.variable_range_count,
       mtrr_capabilities.fields.fixed_range_supported,

diff --git a/HyperPlatform/perf_counter.h b/HyperPlatform/perf_counter.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2015-2017, Satoshi Tanda. All rights reserved.
+// Copyright (c) 2015-2018, Satoshi Tanda. All rights reserved.
 // Use of this source code is governed by a MIT-style license that can be
 // found in the LICENSE file.
 
@@ -247,7 +247,7 @@ class PerfCollector {
   /// room to add a new entry.
   ULONG GetPerfDataIndex(_In_ const char* key) {
     if (!key) {
-      return false;
+      return kInvalidDataIndex;
     }
 
     for (auto i = 0ul; i < kMaxNumberOfDataEntries; i++) {

diff --git a/HyperPlatform/vm.cpp b/HyperPlatform/vm.cpp
@@ -802,8 +802,8 @@ _Use_decl_annotations_ static ULONG VmpGetSegmentAccessRight(
   PAGED_CODE();
 
   VmxRegmentDescriptorAccessRight access_right = {};
-  const SegmentSelector ss = {segment_selector};
   if (segment_selector) {
+    const SegmentSelector ss = {segment_selector};
     auto native_access_right = AsmLoadAccessRightsByte(ss.all);
     native_access_right >>= 8;
     access_right.all = static_cast<ULONG>(native_access_right);

diff --git a/HyperPlatform/vmm.cpp b/HyperPlatform/vmm.cpp
@@ -26,7 +26,7 @@ extern "C" {
 //
 
 // Whether VM-exit recording is enabled
-static const long kVmmpEnableRecordVmExit = false;
+static const bool kVmmpEnableRecordVmExit = false;
 
 // How many events should be recorded per a processor
 static const long kVmmpNumberOfRecords = 100;
@@ -968,7 +968,7 @@ _Use_decl_annotations_ static void VmmpHandleCrAccess(
 
           // The MOV to CR3 does not modify the bit63 of CR3. Emulate this
           // behavior.
-          // See: MOVMove to/from Control Registers
+          // See: MOV - Move to/from Control Registers
           UtilVmWrite(VmcsField::kGuestCr3, (*register_used & ~(1ULL << 63)));
           break;
         }