Independently spotted by both @Dentomologist and me while reviewing
PR #14805
The previous limit was correct for valid VPBs, but an invalid VPB
controlled by a malicious game could contain a non-fractional value
in current_pos_frac, which would allow writing an extra 15 samples
(30 bytes) into the stack. With AFC encoding, this is rounded up to 16
samples, but with much less control over which bytes are written.
Maybe we should be doing some validation, or bounds checking, but I'm
pretty sure this issue was copied from the original ucode, and we kinda
want to stay compatible.
The simpler fix is to just increase the size of raw_input_samples.
I've checked other code paths, and 0x514 samples seems to be the limit.
glMemoryBarrier comes from ARB_shader_image_load_store, so on a desktop context
below GL 4.2 the pointer is null. bSupportsBBox is set from
bSupportsFragmentStoresAndAtomics, which such a driver can still have, so saving
a state reads the bounding box and calls a null pointer.
UsePersistentStagingBuffers() in OGLTexture.cpp already guards the same call with
bSupportsImageLoadStore.
msg_in and msg_out were ending up with only 4-byte alignment, which
is undefined behaviour, since they contain 64-bit fields. Probably
doesn't cause any issues, but it showed up in ubsan.
A malicious GCZ file could probably force this to be negative.
Shouldn't cause any issues other than file read failures, but need to fix
because it is causing errors on MSVC.
SectorReader::ReadChunk does do some validation on it, but it only
checks against the original disc size (reported by the GCZ file).
It has no idea how many blocks the header claimed the disc had.
A maliciously crafted GCZ file could trigger read overflows off the end
of the m_block_pointers/m_hashes arrays.
A malicious dol could theoretically use integer wraparound to bypass
bounds checking and cause DolReader to read past the end of m_bytes.
Could result in crashes, wasting large amounts of memory, or even the
disclosure of heap memory contents.
We already had code to close open host files when reading or writing a
savestate, but due to d35fe1b we also need to close open guest files
when reading a savestate, otherwise DoStateRead fails to delete them.
I was considering an alternative solution where instead of copying and
clearing m_handles, we just set `handle.opened = false;` for each handle
before reading a savestate (but not before writing a savestate).
However, this wouldn't solve the problem of DoStateWriteOrMeasure's
calls to OpenFile failing due to all handles being open. I'm not aware
of any games that have that many handles open, though.
JosJuice
Dr. Dystopia
OatmealDome
Scott Mansell
Tillmann Karras
Dentomologist
Andy1210
Sepalani
Joshua Vandaële
slick-daddy