[ioquake3] 51/136: Allow unaligned load/store in QVM interpreter/x86 compiler constructions like (dataMask & ~3) was used to protect against out-of-bound load/store when address is 4-byte closer to dataMask but at the same time it effectively cut low address bits for ALL load/store operations which is totally wrong in terms of conformance to ALLOWED (i.e. generated by q3lcc from C sources) low-level operations like packed binary data parsing

[Simon McVittie]

This is an automated email from the git hooks/post-receive script.

smcv pushed a commit to branch debian/master
in repository ioquake3.

commit 566fb0edfc6d88efcd161c912c7db025b8c173ce
Author: ec- <tweels at gmail.com>
Date:   Wed Mar 15 11:42:58 2017 +0200

    Allow unaligned load/store in QVM interpreter/x86 compiler
     constructions like (dataMask & ~3) was used to protect against out-of-bound load/store when address is 4-byte closer to dataMask
     but at the same time it effectively cut low address bits for ALL load/store operations which is totally wrong in terms of conformance to ALLOWED (i.e. generated by q3lcc from C sources) low-level operations like packed binary data parsing
---
 code/qcommon/vm.c             |  8 +++++---
 code/qcommon/vm_interpreted.c | 14 +++++++-------
 code/qcommon/vm_local.h       |  1 +
 code/qcommon/vm_x86.c         |  8 ++++----
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/code/qcommon/vm.c b/code/qcommon/vm.c
index e8818a6..dcf5117 100644
--- a/code/qcommon/vm.c
+++ b/code/qcommon/vm.c
@@ -451,13 +451,15 @@ vmHeader_t *VM_LoadQVM( vm_t *vm, qboolean alloc, qboolean unpure)
 	if(alloc)
 	{
 		// allocate zero filled space for initialized and uninitialized data
-		vm->dataBase = Hunk_Alloc(dataLength, h_high);
+		// leave some space beyound data mask so we can secure all mask operations
+		vm->dataAlloc = dataLength + 4;
+		vm->dataBase = Hunk_Alloc(vm->dataAlloc, h_high);
 		vm->dataMask = dataLength - 1;
 	}
 	else
 	{
 		// clear the data, but make sure we're not clearing more than allocated
-		if(vm->dataMask + 1 != dataLength)
+		if(vm->dataAlloc != dataLength + 4)
 		{
 			VM_Free(vm);
 			FS_FreeFile(header.v);
@@ -467,7 +469,7 @@ vmHeader_t *VM_LoadQVM( vm_t *vm, qboolean alloc, qboolean unpure)
 			return NULL;
 		}
 		
-		Com_Memset(vm->dataBase, 0, dataLength);
+		Com_Memset(vm->dataBase, 0, vm->dataAlloc);
 	}
 
 	// copy the intialized data
diff --git a/code/qcommon/vm_interpreted.c b/code/qcommon/vm_interpreted.c
index f630d05..cb86a08 100644
--- a/code/qcommon/vm_interpreted.c
+++ b/code/qcommon/vm_interpreted.c
@@ -436,31 +436,31 @@ nextInstruction2:
 				return 0;
 			}
 #endif
-			r0 = opStack[opStackOfs] = *(int *) &image[r0 & dataMask & ~3 ];
+			r0 = opStack[opStackOfs] = *(int *) &image[ r0 & dataMask ];
 			goto nextInstruction2;
 		case OP_LOAD2:
-			r0 = opStack[opStackOfs] = *(unsigned short *)&image[ r0&dataMask&~1 ];
+			r0 = opStack[opStackOfs] = *(unsigned short *)&image[ r0 & dataMask ];
 			goto nextInstruction2;
 		case OP_LOAD1:
-			r0 = opStack[opStackOfs] = image[ r0&dataMask ];
+			r0 = opStack[opStackOfs] = image[ r0 & dataMask ];
 			goto nextInstruction2;
 
 		case OP_STORE4:
-			*(int *)&image[ r1&(dataMask & ~3) ] = r0;
+			*(int *)&image[ r1 & dataMask ] = r0;
 			opStackOfs -= 2;
 			goto nextInstruction;
 		case OP_STORE2:
-			*(short *)&image[ r1&(dataMask & ~1) ] = r0;
+			*(short *)&image[ r1 & dataMask ] = r0;
 			opStackOfs -= 2;
 			goto nextInstruction;
 		case OP_STORE1:
-			image[ r1&dataMask ] = r0;
+			image[ r1 & dataMask ] = r0;
 			opStackOfs -= 2;
 			goto nextInstruction;
 
 		case OP_ARG:
 			// single byte offset from programStack
-			*(int *)&image[ (codeImage[programCounter] + programStack)&dataMask&~3 ] = r0;
+			*(int *)&image[ (codeImage[programCounter] + programStack) & dataMask ] = r0;
 			opStackOfs--;
 			programCounter += 1;
 			goto nextInstruction;
diff --git a/code/qcommon/vm_local.h b/code/qcommon/vm_local.h
index 76b1a4b..07e8967 100644
--- a/code/qcommon/vm_local.h
+++ b/code/qcommon/vm_local.h
@@ -170,6 +170,7 @@ struct vm_s {
 
 	byte		*dataBase;
 	int			dataMask;
+	int			dataAlloc;			// actually allocated
 
 	int			stackBottom;		// if programStack < stackBottom, error
 
diff --git a/code/qcommon/vm_x86.c b/code/qcommon/vm_x86.c
index 9ee2679..2435a6e 100644
--- a/code/qcommon/vm_x86.c
+++ b/code/qcommon/vm_x86.c
@@ -790,7 +790,7 @@ qboolean ConstOptimize(vm_t *vm, int callProcOfsSyscall)
 		return qtrue;
 
 	case OP_STORE4:
-		EmitMovEAXStack(vm, (vm->dataMask & ~3));
+		EmitMovEAXStack(vm, vm->dataMask);
 #if idx64
 		EmitRexString(0x41, "C7 04 01");		// mov dword ptr [r9 + eax], 0x12345678
 		Emit4(Constant4());
@@ -805,7 +805,7 @@ qboolean ConstOptimize(vm_t *vm, int callProcOfsSyscall)
 		return qtrue;
 
 	case OP_STORE2:
-		EmitMovEAXStack(vm, (vm->dataMask & ~1));
+		EmitMovEAXStack(vm, vm->dataMask);
 #if idx64
 		Emit1(0x66);					// mov word ptr [r9 + eax], 0x1234
 		EmitRexString(0x41, "C7 04 01");
@@ -1377,7 +1377,7 @@ void VM_Compile(vm_t *vm, vmHeader_t *header)
 		case OP_STORE4:
 			EmitMovEAXStack(vm, 0);	
 			EmitString("8B 54 9F FC");			// mov edx, dword ptr -4[edi + ebx * 4]
-			MASK_REG("E2", vm->dataMask & ~3);		// and edx, 0x12345678
+			MASK_REG("E2", vm->dataMask);		// and edx, 0x12345678
 #if idx64
 			EmitRexString(0x41, "89 04 11");		// mov dword ptr [r9 + edx], eax
 #else
@@ -1389,7 +1389,7 @@ void VM_Compile(vm_t *vm, vmHeader_t *header)
 		case OP_STORE2:
 			EmitMovEAXStack(vm, 0);	
 			EmitString("8B 54 9F FC");			// mov edx, dword ptr -4[edi + ebx * 4]
-			MASK_REG("E2", vm->dataMask & ~1);		// and edx, 0x12345678
+			MASK_REG("E2", vm->dataMask);		// and edx, 0x12345678
 #if idx64
 			Emit1(0x66);					// mov word ptr [r9 + edx], eax
 			EmitRexString(0x41, "89 04 11");

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-games/ioquake3.git