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);