menu/materialui: fix three heap-safety issues found during audit pass

Three independent issues found during a focused audit of menu/
drivers/materialui.c.  One is a real heap overflow in the status-bar
metadata builder driven by user-controllable input strings; one is
an OOB write/read on power-user-sized playlist counts; one is a
TOCTOU OOB read in the touch-tap handler.  Same audit class as the
recent xmb (9a27e66) and ozone (d56c728) commits, grouped together
because they share an audit context and none is large enough to
stand alone.

* status-bar metadata: heap overflow via _len underflow in
  strlcpy chain (menu/drivers/materialui.c)

  materialui_render_process_entry_playlist_desktop builds the
  status-bar metadata string ("Core: <name> ⠀ <runtime> ⠀ <last
  played>") through a chain of six

      _len += strlcpy(buf + _len, src, sizeof(buf) - _len);

  calls.  This pattern is NOT self-bounding: strlcpy returns
  strlen(src), not bytes-actually-written, so when any intermediate
  call truncates -- because the source is longer than the remaining
  buffer space -- _len overshoots sizeof(status_bar.str).  The next
  iteration's `sizeof(...) - _len` underflows size_t to ~SIZE_MAX,
  the strlcpy treats the destination as essentially infinite, and
  writes proceed past status_bar.str into adjacent struct fields
  (runtime_fallback_str, last_played_fallback_str, then whatever
  follows in materialui_handle_t on the heap).

  Reachable via any path that produces a long source string: a
  crafted playlist entry with an outsize core_name, a long runtime
  log entry, or a verbose locale translation of
  MENU_ENUM_LABEL_VALUE_PLAYLIST_SUBLABEL_CORE.

  Convert the chain to strlcpy_append, the bound-checked replacement
  added in 78c52ab and applied across the codebase in 25ade82 /
  e446242.  After truncation strlcpy_append clamps *pos to len-1 so
  the chain short-circuits cleanly instead of propagating an
  underflow.

* playlist_selection[]: OOB write/read for users with too many
  playlists (menu/drivers/materialui.c)

  mui->playlist_selection is sized [NAME_MAX_LENGTH], which is the
  *file/dir name length cap* (128 on small builds, 256 on default)
  -- the wrong constant for what's actually a per-playlist-tab
  remembered-selection cache.  materialui_navigation_set writes

      mui->playlist_selection[mui->playlist_selection_ptr] = selection;

  with no bound check on the index, where the index is the user's
  row in the playlists tab (also stored unchecked one branch
  above).  A user with > NAME_MAX_LENGTH playlists -- not implausible
  for someone with FBNeo, MAME, and every console they own --
  navigating to row N and then entering any playlist will OOB-write
  at offset N into adjacent struct fields and the heap beyond.  The
  read site in materialui_populate_entries has the matching OOB
  read.

  Two-part fix.  Replace NAME_MAX_LENGTH with a purpose-built
  MUI_PLAYLIST_SELECTION_MAX (1024 -- comfortable headroom for any
  plausible setup, ~8 KB cache on mui).  Bound-check both the write
  and read against that constant.  Selections in slots beyond
  MUI_PLAYLIST_SELECTION_MAX simply aren't remembered; falling
  through to the existing default-selection behaviour is
  preferable to a heap corruption.

* materialui_pointer_up TAP path: OOB read on stale ptr
  (menu/drivers/materialui.c)

  The TAP-gesture branch does

      list = MENU_LIST_GET_SELECTION(menu_list, 0);
      if (!list)
         break;
      if (!(node = (materialui_node_t*)list->list[ptr].userdata))
         break;

  with no bound check on ptr against list->size.  ptr was set in a
  prior render frame's hit-test loop, where it was bounded by that
  frame's entries_end.  Between render and event delivery the list
  can be repopulated -- search filter applied, navigation back/
  forward, async list rebuild -- leaving ptr stale and possibly
  past the new list end.

  The neighbouring LONG_PRESS branch (line ~11199) and the swipe
  handler at materialui_pointer_up_swipe_horz_default (line ~10874)
  both already guard with `ptr < entries_end`; the TAP path missed
  the same defence.  Add it.
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre committed 2026-04-28 22:31:03 +02:00
1 parent 14a6f8fe07
commit d5e598aedc
1 file changed
+64 -27
+64 -27
View File
@@ -151,6 +151,15 @@
#define MUI_BATTERY_PERCENT_MAX_LENGTH 12
#define MUI_TIMEDATE_MAX_LENGTH 255
/* Maximum number of playlist tabs whose last-selected
* entry index can be cached in mui->playlist_selection[].
* Indexed by the user's row in the playlists tab. Power
* users with many cores can have hundreds of playlists --
* 1024 covers any plausible setup with comfortable margin
* while still bounding mui's heap footprint to ~8 KB for
* this cache. */
#define MUI_PLAYLIST_SELECTION_MAX 1024
/* Allow force enabling secondary thumbnail */
#define MUI_FORCE_ENABLE_SECONDARY 0
@@ -677,7 +686,7 @@ typedef struct materialui_handle
uint32_t flags;
uint32_t context_generation;
size_t playlist_selection[NAME_MAX_LENGTH];
size_t playlist_selection[MUI_PLAYLIST_SELECTION_MAX];
size_t playlist_selection_ptr;
uint8_t mainmenu_selection_ptr;
uint8_t settings_selection_ptr;
@@ -3742,28 +3751,36 @@ static bool materialui_render_process_entry_playlist_desktop(
if (!last_played_str || !*last_played_str)
last_played_str = mui->status_bar.last_played_fallback_str;
/* Generate metadata string */
_len = strlcpy(mui->status_bar.str,
msg_hash_to_str(MENU_ENUM_LABEL_VALUE_PLAYLIST_SUBLABEL_CORE),
sizeof(mui->status_bar.str));
_len += strlcpy(mui->status_bar.str + _len,
" ",
sizeof(mui->status_bar.str) - _len);
_len += strlcpy(mui->status_bar.str + _len,
core_name,
sizeof(mui->status_bar.str) - _len);
_len += strlcpy(mui->status_bar.str + _len,
MUI_TICKER_SPACER,
sizeof(mui->status_bar.str) - _len);
_len += strlcpy(mui->status_bar.str + _len,
runtime_str,
sizeof(mui->status_bar.str) - _len);
_len += strlcpy(mui->status_bar.str + _len,
MUI_TICKER_SPACER,
sizeof(mui->status_bar.str) - _len);
strlcpy(mui->status_bar.str + _len,
last_played_str,
sizeof(mui->status_bar.str) - _len);
/* Generate metadata string.
*
* Pre-conversion this used the unsafe pattern
* _len += strlcpy(buf + _len, src, sizeof(buf) - _len)
* which is NOT self-bounding: strlcpy returns
* strlen(src), not bytes-actually-written, so on a
* truncating call (long core_name from a crafted
* playlist entry, long runtime / last-played strings,
* verbose locale, ...) _len overshoots
* sizeof(status_bar.str), the next len-_len subtraction
* underflows size_t to ~SIZE_MAX, and the subsequent
* strlcpy treats the destination as essentially
* infinite. Adjacent struct fields
* (runtime_fallback_str, last_played_fallback_str,
* ...) get corrupted on the heap. */
_len = 0;
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
&_len, msg_hash_to_str(MENU_ENUM_LABEL_VALUE_PLAYLIST_SUBLABEL_CORE));
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
&_len, " ");
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
&_len, core_name);
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
&_len, MUI_TICKER_SPACER);
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
&_len, runtime_str);
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
&_len, MUI_TICKER_SPACER);
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
&_len, last_played_str);
/* All metadata is cached */
mui->flags |= MUI_FLAG_STATUSBAR_CACHED;
@@ -9414,7 +9431,14 @@ static void materialui_navigation_set(void *data, bool scroll)
return;
if (mui->flags & MUI_FLAG_IS_PLAYLIST)
mui->playlist_selection[mui->playlist_selection_ptr] = selection;
{
/* Bound playlist_selection_ptr against the cache size:
* users with > MUI_PLAYLIST_SELECTION_MAX playlists would
* otherwise OOB-write into adjacent struct fields when
* navigating any deep playlist. */
if (mui->playlist_selection_ptr < MUI_PLAYLIST_SELECTION_MAX)
mui->playlist_selection[mui->playlist_selection_ptr] = selection;
}
else if (mui->flags & MUI_FLAG_IS_PLAYLISTS_TAB)
mui->playlist_selection_ptr = selection;
else if (string_is_equal(mui->menu_title, msg_hash_to_str(MENU_ENUM_LABEL_VALUE_MAIN_MENU)))
@@ -9830,8 +9854,9 @@ static void materialui_populate_entries(void *data, const char *path,
if ( mui->flags & MUI_FLAG_IS_PLAYLIST
&& !string_is_equal(label, MENU_ENUM_LABEL_LOAD_CONTENT_HISTORY_STR))
{
if ( remember_selection == MENU_REMEMBER_SELECTION_ALWAYS
if ( (remember_selection == MENU_REMEMBER_SELECTION_ALWAYS
|| remember_selection == MENU_REMEMBER_SELECTION_PLAYLISTS)
&& mui->playlist_selection_ptr < MUI_PLAYLIST_SELECTION_MAX)
menu_state_get_ptr()->selection_ptr = mui->playlist_selection[mui->playlist_selection_ptr];
}
else if (mui->flags & MUI_FLAG_IS_PLAYLISTS_TAB)
@@ -11112,9 +11137,21 @@ static int materialui_pointer_up(void *userdata,
}
/* Get node (entry) associated with current
* pointer item */
* pointer item.
*
* Bound-check ptr against the live list->size:
* ptr was set by the render-frame hit-test loop,
* which guarantees ptr < entries_end at the point
* it ran -- but the list can be repopulated (search
* filter, navigation, async list rebuild) before
* this event handler executes, leaving ptr stale
* and possibly past the new list end. The
* LONG_PRESS case below and the swipe handler at
* materialui_pointer_up_swipe_horz_default already
* guard with `ptr < entries_end`; mirror that
* defence here. */
list = menu_list ? MENU_LIST_GET_SELECTION(menu_list, 0) : NULL;
if (!list)
if (!list || ptr >= list->size)
break;
if (!(node = (materialui_node_t*)list->list[ptr].userdata))