-
-
Notifications
You must be signed in to change notification settings - Fork 521
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.
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
validate memory access in qvm syscalls #441
base: main
Are you sure you want to change the base?
Conversation
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.
|
Okay, I think the Maybe add a // 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 |
this doesn't apply to ioq3
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 |
No description provided.