Skip to content
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

QVM memory access sanitization #524

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

QVM memory access sanitization #524

wants to merge 1 commit into from

Conversation

elasota
Copy link

@elasota elasota commented Oct 8, 2021

Fix #358

Bit of a different approach from #441 that should be more maintainable.

Removes VMA macro. Replacements:
VMA_N: Address must have enough space for multiple of a specified non-array type.
VMA_1: Address must have enough space for 1 of a specified non-array type.
VMA_VEC3: Address must have enough space for a vec3_t.
VMA_STR: Argument is an input string. (Not valid for output strings - use VMA_DYN)
VMA_DYN: Argument is an array with a specified maximum output quantity, and the quantity must not be negative. Also checks for arithmetic overflow.
VMA_HACK_AVAILABLE: Compatibility hack for syscalls that output to unbounded char pointer (only PC_SourceFileAndLine). In this case, the available address range is used for QVM, and a fallback number is used for DLLs.
VMA_DYN_SIZED: Argument is an array of type with specified size that includes extra space (used for SV_LocateGameData). Specified size must be at least the size of the element type.
VMA_UNBOUNDED: Unchecked pointer, only used for UnifyWhiteSpaces, which should not be able to expand the string size.

Updated StringReplaceWords, BotReplaceSynonyms, BotReplaceReplySynonyms. Synonym replacement now stops if the destination string would be expanded by the synonym replacement beyond its expected size.

There are a ton of other string handling safety issues in botlib though so it's really not a great situation.

Tested offline and everything seems OK?

After this I will try to get another change to harden the memory access a bit further by forcing there to be a decommitted guard page 2 pages after the VM memory, which will mean that any attempt to access memory >1 page beyond the VM memory space will just page fault and crash the game immediately instead of doing bad things to valid memory.

@ghost
Copy link

ghost commented Nov 3, 2021

Hi,
Sorry for the late reply, but testing your changes took some time, especially since I also tested your changes while loading/playing some mods (just to make sure your changes don't affect mod compatibility).

In a nutshell: I couldn't find any errors, and all the mods ran flawlessly.

The only thing I noticed were a couple of warnings during compilation, directly related to your changes.
Here they are:
...
DED_CC code/qcommon/unzip.c
DED_CC code/qcommon/ioapi.c
DED_CC code/qcommon/vm.c
code/qcommon/vm.c: In function ‘VM_ArgPtr’:
code/qcommon/vm.c:763:24: warning: unknown conversion type character ‘z’ in format [-Wformat=]
Com_Error(ERR_DROP, "VM invalid memory access %x+%z", addr, requiredSpace);
^
code/qcommon/vm.c:763:24: warning: too many arguments for format [-Wformat-extra-args]
code/qcommon/vm.c: In function ‘VM_ArgPtrDyn’:
code/qcommon/vm.c:774:23: warning: unknown conversion type character ‘z’ in format [-Wformat=]
Com_Error(ERR_DROP, "VM syscall oversized item count %z", szCount);
^
code/qcommon/vm.c:774:23: warning: too many arguments for format [-Wformat-extra-args]
DED_CC code/qcommon/vm_interpreted.c
...

Compiled with Cygwin on Win 8.1 (x86_64).

Unfortunately I'm no expert as far as qvm code is concerned.
It would be nice if experts would have alook at this.
If your code changes makes qvm loading 'cleaner' it would be worth to add these changes, imo.

@elasota
Copy link
Author

elasota commented Nov 4, 2021

Apparently z needs C99 and there isn't a terrifically portable way otherwise... MSVC only added "z" support in VS2017 I think. Changed it to a cast to unsigned int, which should still be large enough for parameters coming from the VM.

@ghost
Copy link

ghost commented Nov 4, 2021

There are no warnings anymore, well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QVM out-of-bounds memory access
1 participant