libretro-common/vfs: fix three real bugs + regression test

Three independent bugs in vfs_implementation.c, bundled with a
regression test (true discriminator for bug 1) and the CI hook to
run it.

vfs: fix mmap read integer overflow in retro_vfs_file_read_impl
  Pre-patch:
      if (stream->mappos + len > stream->mapsize)
          len = stream->mapsize - stream->mappos;
      memcpy(s, &stream->mapped[stream->mappos], len);

  mappos and len are uint64_t.  When len is attacker-controlled
  (the VFS read interface is exposed to cores), picking len such
  that (mappos + len) overflows uint64_t past zero defeats the
  bound check: the wrapped small value is not > mapsize, the
  clamp is skipped, and memcpy reads the full unclamped len bytes
  off the end of the mapped region.

  Concrete repro on a 16-byte file with mappos seeked to 10:
  pick len = UINT64_MAX - 9.  Sum = UINT64_MAX + 1 = 0 in
  uint64_t.  0 > 16 is false, clamp bypassed, memcpy reads
  (UINT64_MAX - 9) bytes from mapped[10].  Crash / OOB.  ASan
  reports "negative-size-param: (size=-10)" in memcpy.

  Fix: clamp len via unsigned subtraction against the remaining
  bytes (`remaining = mapsize - mappos; if (len > remaining) len
  = remaining;`) before computing the sum.  This cannot wrap.
  Also tighten the at-EOF predicate from > to >= and return 0
  (successful read at EOF) rather than -1.

  Reachability: requires HAVE_MMAP + the frequent-access hint.
  Linux builds, and any other build with mmap support, are
  affected.  Windows (VC6 and every other MSVC) is NOT affected
  since HAVE_MMAP is not defined there.

vfs: saturate retro_vfs_stat_impl size on files > 2 GiB
  Pre-patch *size = (int32_t)size64 silently truncated files
  > 2 GiB.  Worse, for files in (INT32_MAX, UINT32_MAX] the high
  bit propagated and callers saw a negative size, which many
  interpret as an error.  Post-patch saturates to INT_MAX before
  the cast.

  INT_MAX from <limits.h> is used instead of INT32_MAX from
  <stdint.h> for C89 / VC6 portability: INT32_MAX is a C99
  addition and not available in VC6's stdint shim.  int is
  32-bit on all MSVC targets including VC6, so the two macros
  are equal everywhere this code runs.

vfs: fix retro_vfs_file_truncate_impl silent truncation on Windows
  _chsize takes a long (32-bit on Windows) and silently
  truncates lengths > LONG_MAX.  This affects every Windows CRT
  including VC6.  The Secure CRT (VS 2005+, _MSC_VER >= 1400)
  added _chsize_s which takes __int64.

  Post-patch:
    _MSC_VER >= 1400: use _chsize_s with the full 64-bit length
    _MSC_VER  < 1400 (including VC6) and non-MSVC Windows:
      reject lengths > LONG_MAX and fall back to _chsize for
      shorter values.  Returning an error for the unsupported
      case is strictly better than silently truncating the file.

  The _MSC_VER >= 1400 gate preserves VC6 (_MSC_VER = 1200)
  buildability while still giving modern MSVC the correct
  semantics.

Regression test: libretro-common/samples/file/vfs/
  A true discriminator for bug #1.  Creates a 16-byte mmap'd
  file, seeks to offset 10, then calls the read function with
  len = UINT64_MAX - 9.

    unpatched: SEGV (or under ASan:
      "negative-size-param: (size=-10)" in memcpy)
    patched:   both reads return sane clamped values; ASan
      clean; exit 0

  Bugs #2 and #3 have no targeted regression test -- both
  require > 2 GiB input files, impractical for a self-contained
  sample that runs in CI.  The existing nbio_test and
  config_file_test exercise non-huge-file paths through vfs, so
  a regression that breaks the happy path would still surface.

CI: libretro-common samples workflow updated
  vfs_read_overflow_test added to RUN_TARGETS.  Full local
  dry-run under the GHA shell contract reports:
    Built: 13  Ran: 12  Failed: 0
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre committed 2026-04-19 20:36:54 +02:00
1 parent 53a26bca09
commit cf73f1ac55
4 files changed
+236 -11

No files matched your search

@@ -52,6 +52,7 @@ jobs:
rpng
rzip_chunk_size_test
net_ifinfo
vfs_read_overflow_test
)
# Per-binary run command (overrides ./<binary> if present).
+35
View File
@@ -0,0 +1,35 @@
TARGET := vfs_read_overflow_test
LIBRETRO_COMM_DIR := ../../..
SOURCES := \
vfs_read_overflow_test.c \
$(LIBRETRO_COMM_DIR)/compat/fopen_utf8.c \
$(LIBRETRO_COMM_DIR)/compat/compat_strl.c \
$(LIBRETRO_COMM_DIR)/encodings/encoding_utf.c \
$(LIBRETRO_COMM_DIR)/file/file_path.c \
$(LIBRETRO_COMM_DIR)/file/file_path_io.c \
$(LIBRETRO_COMM_DIR)/streams/file_stream.c \
$(LIBRETRO_COMM_DIR)/time/rtime.c \
$(LIBRETRO_COMM_DIR)/vfs/vfs_implementation.c
OBJS := $(SOURCES:.c=.o)
# -DHAVE_MMAP is required for the test to exercise the patched
# code path; without it the implementation falls back to buffered
# fread and the test becomes a smoke test that can't discriminate
# the overflow case.
CFLAGS += -Wall -pedantic -std=gnu99 -g -DHAVE_MMAP -I$(LIBRETRO_COMM_DIR)/include
all: $(TARGET)
%.o: %.c
$(CC) -c -o $@ $< $(CFLAGS)
$(TARGET): $(OBJS)
$(CC) -o $@ $^ $(LDFLAGS)
clean:
rm -f $(TARGET) $(OBJS)
.PHONY: clean
@@ -0,0 +1,149 @@
/* Regression test for mmap-read integer overflow in
* retro_vfs_file_read_impl (libretro-common/vfs/vfs_implementation.c).
*
* Pre-patch, the function contained:
*
* if (stream->mappos + len > stream->mapsize)
* len = stream->mapsize - stream->mappos;
* memcpy(s, &stream->mapped[stream->mappos], len);
*
* mappos and len are both uint64_t. When `len` is attacker-chosen
* and near UINT64_MAX, the addition `mappos + len` wraps past zero
* and the bound check `> mapsize` evaluates FALSE on the small
* wrapped value -- the clamp is skipped and memcpy reads `len`
* bytes off the end of the mapped region.
*
* Post-patch the clamp is done as an unsigned subtraction
* (`remaining = mapsize - mappos; if (len > remaining) len =
* remaining;`) which cannot wrap.
*
* The test mmaps a small file, then directly invokes
* retro_vfs_file_read_impl with a len value engineered to trigger
* the wrap (mappos=10, len=UINT64_MAX-10, sum wraps to 0). Post-
* patch the function returns at most 0 (no bytes left after mappos
* advanced to mapsize) and does not corrupt memory.
*
* ASan gives the strongest signal: pre-patch runs it fires
* "heap-buffer-overflow"; post-patch runs complete cleanly.
*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>
#include <unistd.h>
#include <vfs/vfs.h>
#include <vfs/vfs_implementation.h>
/* These constants are defined in libretro.h; redeclare the subset
* we need so the sample doesn't depend on that header being on the
* include path. */
#ifndef RETRO_VFS_FILE_ACCESS_READ
#define RETRO_VFS_FILE_ACCESS_READ (1 << 0)
#endif
#ifndef RETRO_VFS_FILE_ACCESS_HINT_NONE
#define RETRO_VFS_FILE_ACCESS_HINT_NONE 0
#endif
#ifndef RETRO_VFS_FILE_ACCESS_HINT_FREQUENT_ACCESS
#define RETRO_VFS_FILE_ACCESS_HINT_FREQUENT_ACCESS (1 << 0)
#endif
static int failures = 0;
static void test_mmap_read_overflow(void)
{
libretro_vfs_implementation_file *stream;
const char *tmp_path = "vfs_overflow_test.bin";
const char payload[16] = "ABCDEFGHIJKLMNOP";
char buf[64];
int64_t rc;
FILE *fp;
/* Create a 16-byte file. */
fp = fopen(tmp_path, "wb");
if (!fp) { printf("[ERROR] fopen failed\n"); failures++; return; }
fwrite(payload, 1, sizeof(payload), fp);
fclose(fp);
/* Open with frequent-access hint so the implementation uses the
* mmap code path. (If HAVE_MMAP is not compiled in, this
* falls back to buffered reads and the test becomes a smoke
* test rather than a true discriminator.) */
stream = retro_vfs_file_open_impl(tmp_path,
RETRO_VFS_FILE_ACCESS_READ,
RETRO_VFS_FILE_ACCESS_HINT_FREQUENT_ACCESS);
if (!stream)
{
printf("[ERROR] retro_vfs_file_open_impl failed\n");
failures++;
remove(tmp_path);
return;
}
/* Normal read to establish baseline behaviour. */
rc = retro_vfs_file_read_impl(stream, buf, 4);
if (rc != 4 || memcmp(buf, payload, 4) != 0)
{
printf("[ERROR] baseline read: rc=%lld, want 4\n", (long long)rc);
failures++;
}
else
printf("[SUCCESS] baseline read returned 4 bytes\n");
/* Seek to offset 10. mappos is now 10. */
if (retro_vfs_file_seek_impl(stream, 10, 0 /*SEEK_SET*/) != 10)
{
printf("[ERROR] seek to 10 failed\n");
failures++;
}
/* Crafted len chosen so that (mappos + len) wraps uint64_t past 0
* and lands at or below mapsize (=16), bypassing the pre-patch
* bound check and causing an unclamped memcpy off the end.
*
* With mappos=10, mapsize=16:
* naive check: mappos + len > mapsize
* We need (mappos + len) to wrap past zero in uint64_t.
* Wrap happens when sum >= 2^64, i.e. len >= 2^64 - mappos.
* With mappos=10, pick len = UINT64_MAX - 9 so that sum =
* UINT64_MAX + 1 wraps to 0. Check "0 > 16" is FALSE; the
* clamp is SKIPPED. Memcpy then reads UINT64_MAX - 9 bytes
* starting at mapped[10]. Crash / OOB.
*
* Post-patch: remaining = mapsize - mappos = 6. len (very
* large) > remaining, so len clamps to 6. memcpy reads 6 bytes
* from mapped[10..15]. Safe. */
{
uint64_t evil_len = (uint64_t)-1 - 9; /* UINT64_MAX - 9 */
rc = retro_vfs_file_read_impl(stream, buf, evil_len);
/* Post-patch contract: rc is at most mapsize - mappos = 6.
* Pre-patch would either return a huge number or crash. */
if (rc < 0 || rc > 6)
{
printf("[ERROR] overflow read returned rc=%lld (want 0..6)\n",
(long long)rc);
failures++;
}
else
printf("[SUCCESS] overflow read clamped to rc=%lld\n",
(long long)rc);
}
retro_vfs_file_close_impl(stream);
remove(tmp_path);
}
int main(void)
{
test_mmap_read_overflow();
if (failures)
{
printf("\n%d vfs test(s) failed\n", failures);
return 1;
}
printf("\nAll vfs regression tests passed.\n");
return 0;
}
+51 -11
View File
@@ -24,6 +24,7 @@
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <limits.h> /* INT_MAX, LONG_MAX -- both C89 */
#include <sys/types.h>
#ifdef HAVE_CONFIG_H
@@ -711,11 +712,28 @@ int64_t retro_vfs_file_size_impl(libretro_vfs_implementation_file *stream)
int64_t retro_vfs_file_truncate_impl(libretro_vfs_implementation_file *stream, int64_t len)
{
#ifdef _WIN32
if (stream && stream->fp && _chsize(_fileno(stream->fp), len) == 0)
/* _chsize takes a long and silently truncates lengths > LONG_MAX
* (2 GiB on Windows) -- present on all Windows CRTs including
* VC6. _chsize_s takes __int64 and was added in the Secure CRT
* (VS 2005, _MSC_VER 1400). Prefer the 64-bit variant when
* available, and on older MSVC / MinGW with legacy msvcrt fall
* back to _chsize only for lengths that fit in long -- return
* an error for larger lengths rather than silently truncating
* the file. */
#if defined(_MSC_VER) && _MSC_VER >= 1400
if (stream && stream->fp && _chsize_s(_fileno(stream->fp), len) == 0)
{
stream->size = len;
return 0;
}
#else
if (stream && stream->fp && len >= 0 && len <= (int64_t)LONG_MAX
&& _chsize(_fileno(stream->fp), (long)len) == 0)
{
stream->size = len;
return 0;
}
#endif
#elif !defined(VITA) && !defined(PSP) && !defined(PS2) && !defined(ORBIS) && (!defined(SWITCH) || defined(HAVE_LIBNX))
if (stream && stream->fp && ftruncate(fileno(stream->fp), (off_t)len) == 0)
{
@@ -792,16 +810,31 @@ int64_t retro_vfs_file_read_impl(libretro_vfs_implementation_file *stream,
#ifdef HAVE_MMAP
if (stream->hints & RETRO_VFS_FILE_ACCESS_HINT_FREQUENT_ACCESS)
{
if (stream->mappos > stream->mapsize)
if (stream->mappos >= stream->mapsize)
{
/* At or past EOF: 0 bytes is the correct return for
* fread-style semantics on a legitimate read that reached
* EOF, -1 if we were already past EOF (corrupt state). */
if (stream->mappos == stream->mapsize)
return 0;
return -1;
}
if (stream->mappos + len > stream->mapsize)
len = stream->mapsize - stream->mappos;
/* Clamp len against the remaining mapped bytes. Done as an
* unsigned subtraction *before* computing mappos+len to avoid
* integer overflow: mappos+len can wrap past mapsize when
* both operands are large uint64_t values, defeating the
* naive "mappos + len > mapsize" bound check. */
{
uint64_t remaining = stream->mapsize - stream->mappos;
if (len > remaining)
len = remaining;
}
memcpy(s, &stream->mapped[stream->mappos], len);
memcpy(s, &stream->mapped[stream->mappos], (size_t)len);
stream->mappos += len;
return len;
return (int64_t)len;
}
#endif
@@ -1186,12 +1219,19 @@ int retro_vfs_stat_impl(const char *path, int32_t *size)
int64_t size64 = 0;
int ret = retro_vfs_stat_64_impl(path, size ? &size64 : NULL);
/* if a file is larger than 2 GB, size64 will hold the correct value
* but the cast to int32_t will truncate it.
* new code should migrate to retro_vfs_stat_64_t
*/
/* If a file is larger than 2 GiB, size64 holds the correct value
* but a naked (int32_t) cast would truncate -- worse, on files in
* (INT32_MAX, UINT32_MAX] the high bit wraps and callers see a
* negative size that they may interpret as an error. Saturate to
* INT_MAX so a caller using the legacy API gets a clamped-large
* value rather than a corrupted one, and migrate to
* retro_vfs_stat_64_impl for files that need the real size.
* INT_MAX is used instead of INT32_MAX for C89 / VC6 portability
* (stdint.h's INT32_MAX is a C99 addition; INT_MAX is C89). int
* is 32-bit on all MSVC targets including VC6, so INT_MAX ==
* INT32_MAX everywhere this code runs. */
if (size)
*size = (int32_t)size64;
*size = (size64 > (int64_t)INT_MAX) ? INT_MAX : (int32_t)size64;
return ret;
}