admin/ci-libretro-fceumm
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

core: savestate hardening - reject malformed states, bound APU table indices

Browse Source 
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.
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre
2026-05-04 11:35:52 +02:00
parent 7939d92b3e
commit a3467f6af9
2 changed files with 124 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
src/sound.c
View File

@@ -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++;
}

85
src/state.c
View File

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