Skip to content

Commit d5e598a

Browse files
committed
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.
1 parent 14a6f8f commit d5e598a

1 file changed

Lines changed: 64 additions & 27 deletions

File tree

menu/drivers/materialui.c

Lines changed: 64 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,15 @@
151151
#define MUI_BATTERY_PERCENT_MAX_LENGTH 12
152152
#define MUI_TIMEDATE_MAX_LENGTH 255
153153

154+
/* Maximum number of playlist tabs whose last-selected
155+
* entry index can be cached in mui->playlist_selection[].
156+
* Indexed by the user's row in the playlists tab. Power
157+
* users with many cores can have hundreds of playlists --
158+
* 1024 covers any plausible setup with comfortable margin
159+
* while still bounding mui's heap footprint to ~8 KB for
160+
* this cache. */
161+
#define MUI_PLAYLIST_SELECTION_MAX 1024
162+
154163
/* Allow force enabling secondary thumbnail */
155164
#define MUI_FORCE_ENABLE_SECONDARY 0
156165

@@ -677,7 +686,7 @@ typedef struct materialui_handle
677686
uint32_t flags;
678687
uint32_t context_generation;
679688

680-
size_t playlist_selection[NAME_MAX_LENGTH];
689+
size_t playlist_selection[MUI_PLAYLIST_SELECTION_MAX];
681690
size_t playlist_selection_ptr;
682691
uint8_t mainmenu_selection_ptr;
683692
uint8_t settings_selection_ptr;
@@ -3742,28 +3751,36 @@ static bool materialui_render_process_entry_playlist_desktop(
37423751
if (!last_played_str || !*last_played_str)
37433752
last_played_str = mui->status_bar.last_played_fallback_str;
37443753

3745-
/* Generate metadata string */
3746-
_len = strlcpy(mui->status_bar.str,
3747-
msg_hash_to_str(MENU_ENUM_LABEL_VALUE_PLAYLIST_SUBLABEL_CORE),
3748-
sizeof(mui->status_bar.str));
3749-
_len += strlcpy(mui->status_bar.str + _len,
3750-
" ",
3751-
sizeof(mui->status_bar.str) - _len);
3752-
_len += strlcpy(mui->status_bar.str + _len,
3753-
core_name,
3754-
sizeof(mui->status_bar.str) - _len);
3755-
_len += strlcpy(mui->status_bar.str + _len,
3756-
MUI_TICKER_SPACER,
3757-
sizeof(mui->status_bar.str) - _len);
3758-
_len += strlcpy(mui->status_bar.str + _len,
3759-
runtime_str,
3760-
sizeof(mui->status_bar.str) - _len);
3761-
_len += strlcpy(mui->status_bar.str + _len,
3762-
MUI_TICKER_SPACER,
3763-
sizeof(mui->status_bar.str) - _len);
3764-
strlcpy(mui->status_bar.str + _len,
3765-
last_played_str,
3766-
sizeof(mui->status_bar.str) - _len);
3754+
/* Generate metadata string.
3755+
*
3756+
* Pre-conversion this used the unsafe pattern
3757+
* _len += strlcpy(buf + _len, src, sizeof(buf) - _len)
3758+
* which is NOT self-bounding: strlcpy returns
3759+
* strlen(src), not bytes-actually-written, so on a
3760+
* truncating call (long core_name from a crafted
3761+
* playlist entry, long runtime / last-played strings,
3762+
* verbose locale, ...) _len overshoots
3763+
* sizeof(status_bar.str), the next len-_len subtraction
3764+
* underflows size_t to ~SIZE_MAX, and the subsequent
3765+
* strlcpy treats the destination as essentially
3766+
* infinite. Adjacent struct fields
3767+
* (runtime_fallback_str, last_played_fallback_str,
3768+
* ...) get corrupted on the heap. */
3769+
_len = 0;
3770+
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
3771+
&_len, msg_hash_to_str(MENU_ENUM_LABEL_VALUE_PLAYLIST_SUBLABEL_CORE));
3772+
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
3773+
&_len, " ");
3774+
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
3775+
&_len, core_name);
3776+
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
3777+
&_len, MUI_TICKER_SPACER);
3778+
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
3779+
&_len, runtime_str);
3780+
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
3781+
&_len, MUI_TICKER_SPACER);
3782+
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
3783+
&_len, last_played_str);
37673784

37683785
/* All metadata is cached */
37693786
mui->flags |= MUI_FLAG_STATUSBAR_CACHED;
@@ -9414,7 +9431,14 @@ static void materialui_navigation_set(void *data, bool scroll)
94149431
return;
94159432

94169433
if (mui->flags & MUI_FLAG_IS_PLAYLIST)
9417-
mui->playlist_selection[mui->playlist_selection_ptr] = selection;
9434+
{
9435+
/* Bound playlist_selection_ptr against the cache size:
9436+
* users with > MUI_PLAYLIST_SELECTION_MAX playlists would
9437+
* otherwise OOB-write into adjacent struct fields when
9438+
* navigating any deep playlist. */
9439+
if (mui->playlist_selection_ptr < MUI_PLAYLIST_SELECTION_MAX)
9440+
mui->playlist_selection[mui->playlist_selection_ptr] = selection;
9441+
}
94189442
else if (mui->flags & MUI_FLAG_IS_PLAYLISTS_TAB)
94199443
mui->playlist_selection_ptr = selection;
94209444
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,
98309854
if ( mui->flags & MUI_FLAG_IS_PLAYLIST
98319855
&& !string_is_equal(label, MENU_ENUM_LABEL_LOAD_CONTENT_HISTORY_STR))
98329856
{
9833-
if ( remember_selection == MENU_REMEMBER_SELECTION_ALWAYS
9857+
if ( (remember_selection == MENU_REMEMBER_SELECTION_ALWAYS
98349858
|| remember_selection == MENU_REMEMBER_SELECTION_PLAYLISTS)
9859+
&& mui->playlist_selection_ptr < MUI_PLAYLIST_SELECTION_MAX)
98359860
menu_state_get_ptr()->selection_ptr = mui->playlist_selection[mui->playlist_selection_ptr];
98369861
}
98379862
else if (mui->flags & MUI_FLAG_IS_PLAYLISTS_TAB)
@@ -11112,9 +11137,21 @@ static int materialui_pointer_up(void *userdata,
1111211137
}
1111311138

1111411139
/* Get node (entry) associated with current
11115-
* pointer item */
11140+
* pointer item.
11141+
*
11142+
* Bound-check ptr against the live list->size:
11143+
* ptr was set by the render-frame hit-test loop,
11144+
* which guarantees ptr < entries_end at the point
11145+
* it ran -- but the list can be repopulated (search
11146+
* filter, navigation, async list rebuild) before
11147+
* this event handler executes, leaving ptr stale
11148+
* and possibly past the new list end. The
11149+
* LONG_PRESS case below and the swipe handler at
11150+
* materialui_pointer_up_swipe_horz_default already
11151+
* guard with `ptr < entries_end`; mirror that
11152+
* defence here. */
1111611153
list = menu_list ? MENU_LIST_GET_SELECTION(menu_list, 0) : NULL;
11117-
if (!list)
11154+
if (!list || ptr >= list->size)
1111811155
break;
1111911156

1112011157
if (!(node = (materialui_node_t*)list->list[ptr].userdata))

0 commit comments

Comments
 (0)