From a3467f6af9d44a1c6de51202b35524f31cc27a8e Mon Sep 17 00:00:00 2001 From: "U-DESKTOP-SPFP6AQ\\twistedtechre" Date: Mon, 4 May 2026 11:35:52 +0200 Subject: [PATCH] core: savestate hardening - reject malformed states, bound APU table indices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pass 10 audit of the savestate path against malformed input under AddressSanitizer + UndefinedBehaviorSanitizer. retro_unserialize is attacker-controlled in any frontend that loads .state files from disk, so any out-of-bounds read/write or DoS in the parser is a real attack surface. Real OOB-read crash caught by ASAN savestate fuzzing ==================================================== Random-byte mutation of a valid savestate hits a SEGV in RDoTriangleNoisePCMLQ at sound.c. The LQ Tri/Noise/PCM mixer indexes wlookup2[scaled_tcout + noiseout + scaled_dmc] with all three operands derived from state fields that are loaded straight from the savestate (lq_tcout as raw uint32_t, EnvUnits[2].decvolume as uint8_t, RawDALatch as uint8_t). The static table is only 203 entries; under normal play the channel-write paths keep the operands inside [0, 202] (lq_tcout ≤ 45, noiseout ≤ 30, RawDALatch ≤ 127), but a malformed state can drive the index to any value, producing a heap-buffer-overflow read in the audio inner loop. Same class of bug at four more wlookup2 sites in the same function, plus the HQ wlookup2[(b >> 16) & 255] in WriteHi (mask is 8 bits, but the table is only 203 entries so the upper 53 indices are OOB), and wlookup1[b >> 24] (mask 8 bits, table only 32 entries). A separate but related issue at the LQ Squares mixer: wlookup1 is indexed by ttable[0][RectDutyCount[0]] + ttable[1][RectDutyCount[1]], where RectDutyCount[] is loaded from savestate as int32_t and may be > 7. ttable is int[2][8], so the indirection itself reads adjacent memory before feeding it into wlookup1. Crashes also caught by targeted-input testing on master: an all-zero state, an all-0xFF state, or any state with a corrupted magic header all crash because the chunk loop enters with totalsize parsed as INT_MIN and never exits cleanly. Changes ======= src/state.c: * ReadStateChunk: 4-byte 'toa' read now requires exactly 4 bytes returned (was <= 0, allowing 1- to 3-byte short reads to leave toa[] partially uninitialised before strncmp consumed it). Same for the per-chunk payload memstream_read into tmp->v. A failed memstream_seek on an unknown chunk is now a parse error - the loop previously kept spinning at the same offset on out-of-range seeks because the return was ignored. * ReadStateChunks: chunk size + 5 housekeeping is now overflow- checked before being subtracted from totalsize. The previous 'totalsize -= size + 5' computed size + 5 as uint32_t, so a chunk size of e.g. 0xFFFFFFFB wrapped to 0 and the outer loop failed to make progress. * FCEUSS_Load_Mem: zero-initialise the 16-byte stack header so short reads see deterministic zeros. memstream_read return now checked. NULL-check memstream_open. Read totalsize as uint32_t and reject any value > INT32_MAX rather than silently casting to a negative signed int that makes the chunk loop never enter. src/sound.c: * New helper wl2(idx) clamps the index to wlookup2[]'s size before reading. Used at all five LQ Tri/Noise/PCM call sites and the HQ WriteHi pass. * HQ wlookup1[b >> 24] in WriteHi now masks index by & 31 to match the 32-entry table. * LQ Squares mixer: RectDutyCount[x] masked & 7 at every ttable[x][RectDutyCount[x]] call site. The inner-loop increments already mask the value but the first call after savestate load did not. Verification ============ Random-byte fuzz: 50,000 iterations of a baseline savestate with 1-32 random byte mutations each, run through retro_unserialize + retro_run. Master crashes within ~50 iterations (sound.c SEGV in RDoTriangleNoisePCMLQ); pass 10 completes 50,000 / 50,000 iterations without any ASAN-reported issue. Targeted edge cases (wrong magic, totalsize overflow, chunk-size + 5 wrap, all-zero state, all-0xFF state): every input that crashed master now rejects cleanly on pass 10. Bit-exact regression: master vs pass 10 on 9 valid test ROMs (silent, idle, active, 5-channel max-volume stress, plus the per-APU-channel ROMs from the earlier APU pass) at 1500 audio frames each - byte- identical audio output. Video output identical at 60 frames each. Cross-version compatibility: states saved on master load on pass 10 and vice versa; no format change. Pre-existing issue noted but not fixed in this pass: retro_unserialize unconditionally returns true if the size matches, regardless of whether FCEUSS_Load_Mem actually accepted the state. The frontend has no way to know a malformed savestate was rejected. Worth a follow-up to plumb a return code through FCEUSS_Load_Mem to retro_unserialize. --- src/sound.c | 58 ++++++++++++++++++++++++++++++------ src/state.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 124 insertions(+), 19 deletions(-) diff --git a/src/sound.c b/src/sound.c index 108b54e..e1c6438 100644 --- a/src/sound.c +++ b/src/sound.c @@ -33,6 +33,34 @@ static uint32_t wlookup1[32]; static uint32_t wlookup2[203]; +/* Helper: clamp wlookup2's combined index to the table size. + * + * The LQ Tri/Noise/PCM mix sums lq_tcout, noiseout, and RawDALatch (with + * per-channel volume scalars applied) and uses the result as the index + * into wlookup2[203]. Each input is bounded under normal emulation: + * lq_tcout is (tristep & 0xF) * 3 (max 45), noiseout is the noise + * envelope decvolume << 1 (max 30 with 0..15 envelope range), and + * RawDALatch is the $4011 DAC latch (writes mask off the top bit, so + * 0..127 in the running emulator). Sum max ~202 fits the table. + * + * After loading a savestate, however, every one of those state fields + * is whatever the file said, so EnvUnits[2].decvolume (loaded as raw + * uint8_t) can be 0..255, RawDALatch can be 0..255, lq_tcout (loaded + * as uint32_t) can be anything, and the sum can overflow the table by + * a wide margin - a heap-buffer-overflow read that AddressSanitizer + * surfaces in retro_run after retro_unserialize on a malformed state. + * + * Clamp at the access site so the protection holds regardless of which + * piece of state was tampered with. Out-of-range values play wrong + * audio for one frame until DoEnv / channel writes restore sane state, + * but the emulator stays alive. */ +static INLINE uint32_t wl2(uint32_t idx) +{ + if (idx >= sizeof(wlookup2) / sizeof(wlookup2[0])) + idx = sizeof(wlookup2) / sizeof(wlookup2[0]) - 1; + return wlookup2[idx]; +} + int32_t Wave[2048 + 512]; int32_t WaveHi[40000]; int32_t WaveFinal[2048 + 512]; @@ -646,7 +674,17 @@ static void RDoSQLQ(void) { freq[x] <<= 17; } - totalout = wlookup1[ ttable[0][RectDutyCount[0]] + ttable[1][RectDutyCount[1]] ]; + /* RectDutyCount[] is reloaded from savestate as raw int32_t and may + * hold any value at this point - the inner-loop increments later mask + * it back to 0..7, but this first lookup hits the table before any + * such mask. Without bounds protection, ttable[0..7] (int[8]) reads + * garbage memory and feeds it into wlookup1[32], which then OOB-reads + * - reachable via a malformed savestate. Mask both indices defensively + * here and at every other ttable/wlookup1 call site below. The mask + * is one instruction and runs only at the chunk boundaries, not per + * inner-loop sample. */ + totalout = wlookup1[(ttable[0][RectDutyCount[0] & 7] + + ttable[1][RectDutyCount[1] & 7]) & 31]; if (!inie[0] && !inie[1]) { /* Both squares silent: amp[x] was forced to 0 above (line @@ -676,7 +714,8 @@ static void RDoSQLQ(void) { sqacc[0] += freq[0]; RectDutyCount[0] = (RectDutyCount[0] + 1) & 7; if (sqacc[0] <= 0) goto rea; - totalout = wlookup1[ ttable[0][RectDutyCount[0]] + ttable[1][RectDutyCount[1]] ]; + totalout = wlookup1[(ttable[0][RectDutyCount[0]] + + ttable[1][RectDutyCount[1] & 7]) & 31]; } if (sqacc[1] <= 0) { @@ -684,7 +723,8 @@ static void RDoSQLQ(void) { sqacc[1] += freq[1]; RectDutyCount[1] = (RectDutyCount[1] + 1) & 7; if (sqacc[1] <= 0) goto rea2; - totalout = wlookup1[ ttable[0][RectDutyCount[0]] + ttable[1][RectDutyCount[1]] ]; + totalout = wlookup1[(ttable[0][RectDutyCount[0] & 7] + + ttable[1][RectDutyCount[1]]) & 31]; } } } @@ -790,7 +830,7 @@ static void RDoTriangleNoisePCMLQ(void) { ? ((RawDALatch * FSettings.PCMVolume) / 256) : RawDALatch; - totalout = wlookup2[scaled_tcout + noiseout + scaled_dmc]; + totalout = wl2(scaled_tcout + noiseout + scaled_dmc); if (inie[0] && inie[1]) { for (V = start; V < end; V++) { @@ -810,7 +850,7 @@ static void RDoTriangleNoisePCMLQ(void) { scaled_tcout = (tri_vol != 256) ? ((lq_tcout * tri_vol) / 256) : lq_tcout; - totalout = wlookup2[scaled_tcout + noiseout + scaled_dmc]; + totalout = wl2(scaled_tcout + noiseout + scaled_dmc); } if (lq_noiseacc <= 0) { @@ -826,7 +866,7 @@ static void RDoTriangleNoisePCMLQ(void) { nreg &= 0x7fff; noiseout = amptab[(nreg >> 0xe) & 1]; if (lq_noiseacc <= 0) goto rea2; - totalout = wlookup2[scaled_tcout + noiseout + scaled_dmc]; + totalout = wl2(scaled_tcout + noiseout + scaled_dmc); } /* noiseacc<=0 */ } /* for(V=... */ } else if (inie[0]) { @@ -846,7 +886,7 @@ static void RDoTriangleNoisePCMLQ(void) { scaled_tcout = (tri_vol != 256) ? ((lq_tcout * tri_vol) / 256) : lq_tcout; - totalout = wlookup2[scaled_tcout + noiseout + scaled_dmc]; + totalout = wl2(scaled_tcout + noiseout + scaled_dmc); } } } else if (inie[1]) { @@ -866,7 +906,7 @@ static void RDoTriangleNoisePCMLQ(void) { nreg &= 0x7fff; noiseout = amptab[(nreg >> 0xe) & 1]; if (lq_noiseacc <= 0) goto area2; - totalout = wlookup2[scaled_tcout + noiseout + scaled_dmc]; + totalout = wl2(scaled_tcout + noiseout + scaled_dmc); } /* noiseacc<=0 */ } } else { @@ -989,7 +1029,7 @@ int FlushEmulateSound(void) { for (x = sound_timestamp; x; x--) { uint32_t b = *tmpo; - *tmpo = (b & 65535) + wlookup2[(b >> 16) & 255] + wlookup1[b >> 24]; + *tmpo = (b & 65535) + wl2((b >> 16) & 255) + wlookup1[(b >> 24) & 31]; tmpo++; } diff --git a/src/state.c b/src/state.c index 8c748e1..1255e09 100644 --- a/src/state.c +++ b/src/state.c @@ -200,14 +200,25 @@ static int ReadStateChunk(memstream_t *mem, SFORMAT *sf, int size) { uint32_t tsize; char toa[4]; - if(memstream_read(mem, toa, 4) <= 0) + /* memstream_read returns the number of bytes read, or 0/negative on + * error. Treat anything less than the requested four bytes as a + * truncation; the previous <= 0 check let a 1- to 3-byte short read + * leave toa[] partially uninitialised before strncmp consumed it. */ + if (memstream_read(mem, toa, 4) != 4u) return 0; - read32le_mem(&tsize, mem); + if (!read32le_mem(&tsize, mem)) + return 0; if((tmp = CheckS(sf, tsize, toa))) { - memstream_read(mem, (char *)tmp->v, sf_size(tmp->s)); + /* sf_size(tmp->s) was already validated to equal tsize by + * CheckS, and tmp->v points at the fixed-size in-memory buffer + * for this state field, so the read is bounded. Still treat a + * short read here as a parse failure rather than leaving the + * buffer in a half-updated state. */ + if (memstream_read(mem, (char *)tmp->v, sf_size(tmp->s)) != (uint64_t)sf_size(tmp->s)) + return 0; #ifdef MSB_FIRST if(tmp->s & RLSB) @@ -215,7 +226,17 @@ static int ReadStateChunk(memstream_t *mem, SFORMAT *sf, int size) #endif } else - memstream_seek(mem, tsize, SEEK_CUR); + { + /* Skip past the unknown chunk's payload. memstream_seek already + * bounds-checks against the stream end and returns -1 on + * out-of-range, but the caller previously ignored the return, + * leaving the read loop spinning at the same offset and re- + * parsing whatever bytes happened to follow. Treat a failed + * skip as a parse error so we don't silently load a truncated + * state. */ + if (memstream_seek(mem, tsize, SEEK_CUR) < 0) + return 0; + } } return 1; } @@ -233,7 +254,21 @@ static int ReadStateChunks(memstream_t *st, int32_t totalsize) break; if (!read32le_mem(&size, st)) break; - totalsize -= size + 5; + /* size came straight off disk and is attacker-controlled. The + * arithmetic below was previously + * + * totalsize -= size + 5; + * + * but `size + 5` is computed as uint32_t and can wrap to a small + * value (e.g. size = 0xFFFFFFFB makes size + 5 == 0), so totalsize + * fails to decrement and the outer loop spins through the rest of + * the stream. Reject any size that would either (a) overflow the + * +5 housekeeping bytes or (b) exceed the remaining declared + * payload, which is the only legitimate range. */ + if (size > (uint32_t)0x7FFFFFFFu - 5 + || (uint32_t)totalsize < size + 5) + break; + totalsize -= (int32_t)(size + 5); switch(t) { @@ -315,22 +350,52 @@ void FCEUSS_Load_Mem(void) { memstream_t *mem = memstream_open(0); - uint8_t header[16]; + uint8_t header[16] = {0}; int stateversion; - int totalsize; + uint32_t totalsize_u; + int32_t totalsize; int x; - memstream_read(mem, header, 16); + /* memstream_open can't legitimately return NULL in the libretro path + * (the buffer is set by retro_unserialize via memstream_set_buffer), + * but treat NULL defensively rather than dereferencing. */ + if (!mem) + return; + + /* If the buffer is shorter than the 16-byte header the read returns + * fewer bytes; the magic-string memcmp would otherwise compare against + * a stack-uninitialised tail. Zeroed header[] above plus the read- + * length check below leave the comparisons well-defined. */ + if (memstream_read(mem, header, 16) != 16u) + { + memstream_close(mem); + return; + } if (memcmp(header, "FCS", 3) != 0) + { + memstream_close(mem); return; + } if (header[3] == 0xFF) stateversion = FCEU_de32lsb(header + 8); else stateversion = header[3] * 100; - - totalsize = FCEU_de32lsb(header + 4); + + /* totalsize on disk is unsigned 32-bit. The previous code stored it + * straight into a signed int, so a malicious header with bit 31 set + * would silently produce a negative totalsize - the chunk loop would + * then never enter and the savestate would "load" without applying + * any of its data. Read into a uint32_t and reject obviously bogus + * sizes that exceed INT32_MAX or what's actually in the buffer. */ + totalsize_u = FCEU_de32lsb(header + 4); + if (totalsize_u > 0x7FFFFFFFu) + { + memstream_close(mem); + return; + } + totalsize = (int32_t)totalsize_u; x = ReadStateChunks(mem, totalsize);