Commit bac6894
committed
gfx/video_shader_parse: clamp replace_text length and prefix+_len in wildcard replacement
video_shader_replace_wildcards_impl built each replacement
value into a 256-byte stack buffer:
char replace_text[256];
_len = strlcpy(replace_text, source, sizeof(replace_text));
then unbounded:
memcpy(dst + prefix, replace_text, _len);
strlcpy(dst + prefix + _len, found + token_len,
PATH_MAX_LENGTH - prefix - _len);
Two related primitives were exposed:
1. strlcpy returns strlen(source) regardless of destination
capacity. When source resolved to a $CONTENT-DIR$ /
$PRESET-DIR$ / $CORE$ / $GAME$ value longer than 256
bytes (DIR_MAX_LENGTH is 4096, library_name and
path_basename are platform-dependent and routinely
exceed 256), _len was strlen(source). The memcpy then
read past the end of the 256-byte replace_text array
into adjacent stack memory.
2. The same _len fed (size_t)(PATH_MAX_LENGTH - prefix -
_len) on the next line. When prefix + _len exceeded
PATH_MAX_LENGTH the subtraction underflowed size_t to a
huge value and the strlcpy walked off the end of dst.
Even with the _len clamp from (1), a wildcard token
placed near the end of the input path (legitimate for
an attacker-supplied .slangp/.glslp) lets prefix +
REPLACE_BUF_SZ exceed PATH_MAX_LENGTH on its own.
The snprintf-chained branches (CORE_REQUESTED_ROTATION,
VIDEO_USER_ROTATION, VIDEO_FINAL_ROTATION,
SCREEN_ORIENTATION) had a sibling problem: snprintf returns
the would-have-written length, so _len += snprintf(...) on
a saturated buffer pushes _len past sizeof(replace_text)
even when the destination ate the entire size budget.
Reachable via shader presets the user opens. These come
from the online updater (typically slang-shaders or
common-shaders), the user's own preset directory, and from
content-bundled presets distributed alongside ROMs.
Fix: clamp at the use site so all the upstream branches are
covered in one place.
if (_len >= sizeof(replace_text))
_len = sizeof(replace_text) - 1;
if (prefix >= PATH_MAX_LENGTH || _len >= PATH_MAX_LENGTH - prefix)
break;
Adds samples/tasks/video_shader_parse/video_shader_wildcard_test
as a regression test, registered in
.github/workflows/Linux-samples-tasks.yml as an ASan-enabled
step. Four cases:
- short replacement produces correct output
- long source (600 chars) clamped without OOB read
- huge source (4000 chars) clamped without strlcpy underflow
- prefix near PATH_MAX_LENGTH correctly bails out
Test follows the existing samples/tasks pattern (verbatim
copy of the post-fix arithmetic block, ASan as the
discriminator) used by archive_name_safety_test and
http_method_match_test. Verified to fire under ASan when
the clamps are removed: stack-buffer-overflow READ of size
600 at offset 288 in replace_text (a 256-byte buffer).
With the clamps the test passes clean.
Note on test scope: the test exercises a verbatim copy of
the post-replacement arithmetic block from
video_shader_parse.c lines 344-358, not the full
video_shader_replace_wildcards_impl. The full function
depends on settings_t / runloop_state / video driver
globals that aren't tractable to stub for a unit test --
following the convention used by archive_name_safety_test
("if task_decompress.c amends the predicate, the copy here
must follow"), the test's verbatim block must track the
production block. A comment in the test points at the
production lines explicitly.1 parent 854c9f3 commit bac6894
4 files changed
Lines changed: 423 additions & 0 deletions
File tree
- .github/workflows
- gfx
- samples/tasks/video_shader_parse
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
100 | 100 | | |
101 | 101 | | |
102 | 102 | | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
345 | 345 | | |
346 | 346 | | |
347 | 347 | | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
348 | 371 | | |
349 | 372 | | |
350 | 373 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
0 commit comments