validate memory access in qvm syscalls

[kvanderlaag]

No description provided.

[zturtleman]

Continuing from #440

Can you explain why "reserve some space for effective LOCAL+LOAD* checks" is necessary? It allocates additional memory but nothing should of previously used it...? (The existing 4 byte padding would end null-terminated strings.) Using another 1kb of memory isn't an issue but I don't understand what it's for and why that size is chosen.

• In VM_LoadQVM vm->dataAlloc is now set to dataAlloc (dataLength + 1024) but the check for VM_Restart() (reloading qagame.qvm between maps) is still if(vm->dataAlloc != dataLength + 4) which will fail.

  • Suggest changing to if(vm->dataAlloc != dataAlloc)

• The (unsigned) cast for comparison in VM_CheckBounds{,2} should be added to vm->dataAlloc (a signed int variable) as well.

• Changing dataLength, dataAlloc, i to unsigned in VM_LoadQVM but not changing vm->dataLength and vm->dataAlloc is odd. Does it need to be unsigned or not? If not, recommend not adding unrelated change.

• Recommend not adding unrelated change of adding static to VM_LoadQVM function.

• Still missing spaces inside parentheses in VM_CheckBounds and VM_CheckBounds2.

[zturtleman]

Okay, I think the dataLength + 1024 ("reserve some space for effective LOCAL+LOAD* checks") is to allow skipping run-time length check for many small structs. It's presumably better for performance but I'm not trilled about derivative games having to track down every struct type passed to a VM syscall to review the size of it. If a struct is over 1024 bytes it could allow read/write out of bounds.

Maybe add a VM_CHECKBOUNDS_PARANOID() macro (same args as VM_CHECKBOUNDS()) for every struct argument in the syscall handlers. Maybe something like:

// Allowed VM syscall argument size in bytes without range check. Must be at least 4.
// Pointers to larger structs or blocks of memory must be checked with VM_CHECKBOUNDS().
#define VM_MINIMUM_ARGUMENT_SIZE 1024

// This should require length to be a constant or sizeof (struct) and fail to compile if length is too long;
// in which case it should be using VM_CHECKBOUNDS() instead.
#define VM_CHECKBOUNDS_PARANOID( vm, addr, length ) \
	{ typedef char checkbounds_paranoid[ ( (length) <= VM_MINIMUM_ARGUMENT_SIZE ) ? 1 : -1 ]; }

For example this will fail to compile since pc_token_t is 1040 bytes in size and should use VM_CHECKBOUNDS(). (As of writing Quake3e does not check BOTLIB_PC_READ_TOKEN.)

	case BOTLIB_PC_READ_TOKEN:
		VM_CHECKBOUNDS_PARANOID( gvm, args[2], sizeof( pc_token_t ) );
		return botlib_export->PC_ReadTokenHandle( args[1], VMA(2) );

I kind of feel cursed to know pc_token_t has a 1024 byte string buffer in it. Of course the fun part of this is to check the arguments for every function to write up VM_CHECKBOUNDS_PARANOID() for each struct passed to a syscall.

[ec-]

this doesn't apply to ioq3

for effective LOCAL+LOAD* checks"

because it doesn't have corresponding compile-time checks (runtime checks used instead) related to different qvm loader but in general - yes, it may be useful if you want to omit bounds check for many smaller structs/arguments

And you may skip if ( vm->entryPoint ) conditions by setting vm->dataMask and vm->dataAlloc to ~0