Skip to content

validate memory access in qvm syscalls #441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kvanderlaag wants to merge 3 commits into
base: main
Choose a base branch
from
Open

validate memory access in qvm syscalls #441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
566879e
validate memory accesses in qvm system calls
kvanderlaag Feb 4, 2020
e7bb739
allocate additional space for effective load checks
kvanderlaag Feb 5, 2020
a4a4841
Enable check to avoid crash in DLLs; signed->unsigned cast; code form…
kvanderlaag Feb 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions code/client/cl_cgame.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,22 +427,27 @@ intptr_t CL_CgameSystemCalls( intptr_t *args ) {
Cvar_SetSafe( VMA(1), VMA(2) );
return 0;
case CG_CVAR_VARIABLESTRINGBUFFER:
VM_CHECKBOUNDS( cgvm, args[2], args[3] );
Cvar_VariableStringBuffer( VMA(1), VMA(2), args[3] );
return 0;
case CG_ARGC:
return Cmd_Argc();
case CG_ARGV:
VM_CHECKBOUNDS( cgvm, args[2], args[3] );
Cmd_ArgvBuffer( args[1], VMA(2), args[3] );
return 0;
case CG_ARGS:
VM_CHECKBOUNDS( cgvm, args[1], args[2] );
Cmd_ArgsBuffer( VMA(1), args[2] );
return 0;
case CG_FS_FOPENFILE:
return FS_FOpenFileByMode( VMA(1), VMA(2), args[3] );
case CG_FS_READ:
VM_CHECKBOUNDS( cgvm, args[1], args[2] );
FS_Read( VMA(1), args[2], args[3] );
return 0;
case CG_FS_WRITE:
VM_CHECKBOUNDS( cgvm, args[1], args[2] );
FS_Write( VMA(1), args[2], args[3] );
return 0;
case CG_FS_FCLOSEFILE:
Expand Down Expand Up @@ -577,9 +582,11 @@ intptr_t CL_CgameSystemCalls( intptr_t *args ) {
case CG_R_LERPTAG:
return re.LerpTag( VMA(1), args[2], args[3], args[4], VMF(5), VMA(6) );
case CG_GETGLCONFIG:
VM_CHECKBOUNDS( cgvm, args[1], sizeof(glconfig_t) );
CL_GetGlconfig( VMA(1) );
return 0;
case CG_GETGAMESTATE:
VM_CHECKBOUNDS( cgvm, args[1], sizeof(gameState_t) );
CL_GetGameState( VMA(1) );
return 0;
case CG_GETCURRENTSNAPSHOTNUMBER:
Expand Down Expand Up @@ -612,12 +619,15 @@ intptr_t CL_CgameSystemCalls( intptr_t *args ) {


case CG_MEMSET:
VM_CHECKBOUNDS( cgvm, args[1], args[3] );
Com_Memset( VMA(1), args[2], args[3] );
return 0;
case CG_MEMCPY:
VM_CHECKBOUNDS2( cgvm, args[1], args[2], args[3] );
Com_Memcpy( VMA(1), VMA(2), args[3] );
return 0;
case CG_STRNCPY:
VM_CHECKBOUNDS2( cgvm, args[1], args[2], args[3] );
strncpy( VMA(1), VMA(2), args[3] );
return args[1];
case CG_SIN:
Expand Down Expand Up @@ -689,6 +699,7 @@ intptr_t CL_CgameSystemCalls( intptr_t *args ) {
return getCameraInfo(args[1], VMA(2), VMA(3));
*/
case CG_GET_ENTITY_TOKEN:
VM_CHECKBOUNDS( cgvm, args[1], args[2] );
return re.GetEntityToken( VMA(1), args[2] );
case CG_R_INPVS:
return re.inPVS( VMA(1), VMA(2) );
Expand Down
18 changes: 18 additions & 0 deletions code/client/cl_ui.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,7 @@ intptr_t CL_UISystemCalls( intptr_t *args ) {
return FloatAsInt( Cvar_VariableValue( VMA(1) ) );

case UI_CVAR_VARIABLESTRINGBUFFER:
VM_CHECKBOUNDS(uivm, args[2], args[3]);
Cvar_VariableStringBuffer( VMA(1), VMA(2), args[3] );
return 0;

Expand All @@ -764,13 +765,15 @@ intptr_t CL_UISystemCalls( intptr_t *args ) {
return 0;

case UI_CVAR_INFOSTRINGBUFFER:
VM_CHECKBOUNDS(uivm, args[2], args[3]);
Cvar_InfoStringBuffer( args[1], VMA(2), args[3] );
return 0;

case UI_ARGC:
return Cmd_Argc();

case UI_ARGV:
VM_CHECKBOUNDS(uivm, args[2], args[3]);
Cmd_ArgvBuffer( args[1], VMA(2), args[3] );
return 0;

Expand All @@ -790,10 +793,12 @@ intptr_t CL_UISystemCalls( intptr_t *args ) {
return FS_FOpenFileByMode( VMA(1), VMA(2), args[3] );

case UI_FS_READ:
VM_CHECKBOUNDS(uivm, args[1], args[2]);
FS_Read( VMA(1), args[2], args[3] );
return 0;

case UI_FS_WRITE:
VM_CHECKBOUNDS(uivm, args[1], args[2]);
FS_Write( VMA(1), args[2], args[3] );
return 0;

Expand All @@ -802,6 +807,7 @@ intptr_t CL_UISystemCalls( intptr_t *args ) {
return 0;

case UI_FS_GETFILELIST:
VM_CHECKBOUNDS(uivm, args[3], args[4]);
return FS_GetFileList( VMA(1), VMA(2), VMA(3), args[4] );

case UI_FS_SEEK:
Expand Down Expand Up @@ -864,10 +870,12 @@ intptr_t CL_UISystemCalls( intptr_t *args ) {
return 0;

case UI_KEY_KEYNUMTOSTRINGBUF:
VM_CHECKBOUNDS(uivm, args[2], args[3]);
Key_KeynumToStringBuf( args[1], VMA(2), args[3] );
return 0;

case UI_KEY_GETBINDINGBUF:
VM_CHECKBOUNDS(uivm, args[2], args[3]);
Key_GetBindingBuf( args[1], VMA(2), args[3] );
return 0;

Expand Down Expand Up @@ -898,6 +906,7 @@ intptr_t CL_UISystemCalls( intptr_t *args ) {
return 0;

case UI_GETCLIPBOARDDATA:
VM_CHECKBOUNDS(uivm, args[1], args[2]);
CL_GetClipboardData( VMA(1), args[2] );
return 0;

Expand All @@ -906,10 +915,12 @@ intptr_t CL_UISystemCalls( intptr_t *args ) {
return 0;

case UI_GETGLCONFIG:
VM_CHECKBOUNDS(uivm, args[1], sizeof(glconfig_t));
CL_GetGlconfig( VMA(1) );
return 0;

case UI_GETCONFIGSTRING:
VM_CHECKBOUNDS(uivm, args[2], args[3]);
return GetConfigString( args[1], VMA(2), args[3] );

case UI_LAN_LOADCACHEDSERVERS:
Expand All @@ -935,21 +946,25 @@ intptr_t CL_UISystemCalls( intptr_t *args ) {
return 0;

case UI_LAN_GETPING:
VM_CHECKBOUNDS(uivm, args[2], args[3]);
LAN_GetPing( args[1], VMA(2), args[3], VMA(4) );
return 0;

case UI_LAN_GETPINGINFO:
VM_CHECKBOUNDS(uivm, args[2], args[3]);
LAN_GetPingInfo( args[1], VMA(2), args[3] );
return 0;

case UI_LAN_GETSERVERCOUNT:
return LAN_GetServerCount(args[1]);

case UI_LAN_GETSERVERADDRESSSTRING:
VM_CHECKBOUNDS(uivm, args[3], args[4]);
LAN_GetServerAddressString( args[1], args[2], VMA(3), args[4] );
return 0;

case UI_LAN_GETSERVERINFO:
VM_CHECKBOUNDS(uivm, args[3], args[4]);
LAN_GetServerInfo( args[1], args[2], VMA(3), args[4] );
return 0;

Expand Down Expand Up @@ -997,14 +1012,17 @@ intptr_t CL_UISystemCalls( intptr_t *args ) {
return 0;

case UI_MEMSET:
VM_CHECKBOUNDS(uivm, args[1], args[3]);
Com_Memset( VMA(1), args[2], args[3] );
return 0;

case UI_MEMCPY:
VM_CHECKBOUNDS2(uivm, args[1], args[2], args[3]);
Com_Memcpy( VMA(1), VMA(2), args[3] );
return 0;

case UI_STRNCPY:
VM_CHECKBOUNDS2(uivm, args[1], args[2], args[3]);
strncpy( VMA(1), VMA(2), args[3] );
return args[1];

Expand Down
14 changes: 14 additions & 0 deletions code/qcommon/qcommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,20 @@ intptr_t QDECL VM_Call( vm_t *vm, int callNum, ... );

void VM_Debug( int level );

/* These functions validate arguments to functions that could access memory
outside the VM's memory space. Defining _IOQ3_INSECURE_QVM will enable
macros which cause them to be omitted, and is definitely unsafe, though
more performant. */
#ifndef _IOQ3_INSECURE_QVM
void VM_CheckBounds( const vm_t* vm, unsigned int address, unsigned int length );
void VM_CheckBounds2( const vm_t* vm, unsigned int addr1, unsigned int addr2, unsigned int length );

#define VM_CHECKBOUNDS VM_CheckBounds
#define VM_CHECKBOUNDS2 VM_CheckBounds2
#else
#define VM_CHECKBOUNDS( a, b )
#define VM_CHECKBOUNDS2( a, b, c )
#endif
void *VM_ArgPtr( intptr_t intValue );
void *VM_ExplicitArgPtr( vm_t *vm, intptr_t intValue );

Expand Down
45 changes: 41 additions & 4 deletions code/qcommon/vm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,39 @@ void VM_Debug( int level ) {
vm_debugLevel = level;
}

/*
==============
VM_CheckBounds
==============
*/
void VM_CheckBounds(const vm_t* vm, unsigned int address, unsigned int length)
{
if (!vm->entryPoint)
{
if ((address | length) > (unsigned)vm->dataMask || (address + length) > vm->dataAlloc)
{
Com_Error(ERR_DROP, "program tried to bypass data segment bounds");
}
}
}


/*
==============
VM_CheckBounds2
==============
*/
void VM_CheckBounds2(const vm_t* vm, unsigned int addr1, unsigned int addr2, unsigned int length)
{
if (!vm->entryPoint)
{
if ((addr1 | addr2 | length) > (unsigned)vm->dataMask || (addr1 + length) > vm->dataAlloc || (addr2 + length) > vm->dataAlloc)
{
Com_Error(ERR_DROP, "program tried to bypass data segment bounds");
}
}
}

/*
==============
VM_Init
Expand Down Expand Up @@ -363,10 +396,11 @@ VM_LoadQVM
Load a .qvm file
=================
*/
vmHeader_t *VM_LoadQVM( vm_t *vm, qboolean alloc, qboolean unpure)
static vmHeader_t *VM_LoadQVM( vm_t *vm, qboolean alloc, qboolean unpure)
{
int dataLength;
int i;
unsigned int dataLength;
unsigned int dataAlloc;
unsigned int i;
char filename[MAX_QPATH];
union {
vmHeader_t *h;
Expand Down Expand Up @@ -448,11 +482,14 @@ vmHeader_t *VM_LoadQVM( vm_t *vm, qboolean alloc, qboolean unpure)
}
dataLength = 1 << i;

// reserve some space for effective LOCAL+LOAD* checks
dataAlloc = dataLength + 1024;

if(alloc)
{
// allocate zero filled space for initialized and uninitialized data
// leave some space beyond data mask so we can secure all mask operations
vm->dataAlloc = dataLength + 4;
vm->dataAlloc = dataAlloc;
vm->dataBase = Hunk_Alloc(vm->dataAlloc, h_high);
vm->dataMask = dataLength - 1;
}
Expand Down
Loading