[lib][uefi] fix a heap overflow, uninitialized con_out, and stale event list links - #531
Merged
zhangxp1998 merged 4 commits intoAug 31, 2026
Merged
Conversation
efi_core_new_debug_image_info_entry allocated sizeof(union EfiDebugImageInfo), the size of a single pointer, for the normal_image entry, then wrote a struct EfiDebugImageInfoNormal through it, overflowing the pool allocation by 16 bytes on every image load. Allocate the size of the structure actually stored there.
get_text_output_protocol returned a protocol struct with every member except output_string left uninitialized, so an application calling reset, clear_screen, or query_mode, or dereferencing the mode pointer, would jump through stack garbage. Zero-initialize the struct, provide benign implementations for the remaining callbacks, and point mode at a static 80x25 mode description.
process_pending_events moved ready events onto a stack-local list and invoked their callbacks with the nodes still chained to that local list head. After the function returned, each completed event's node kept pointing into the dead stack frame, so a later close_event or check_event calling delete_if_in_list would pass the non-null check and write through the dangling pointers, corrupting whatever occupied that stack memory. Pop each event off the local list, which clears its links, before invoking its callback.
There was a problem hiding this comment.
Pull request overview
This PR applies targeted memory-safety and correctness fixes in lib/uefi around debug image table allocation, console output protocol initialization, and event processing/wait behavior to prevent heap overflows, use of uninitialized function pointers/state, and stale list-node linkage.
Changes:
- Fix debug image info entry allocation to use the correct structure size.
- Fully initialize
EfiSimpleTextOutputProtocolcallbacks and provide a stablemodeobject. - Prevent stale list-node links in completed event processing and ensure
wait_for_event()stores the signaled event index.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/uefi/text_protocol.cpp | Initializes all Simple Text Output protocol callbacks and provides a static mode descriptor. |
| lib/uefi/events.cpp | Unlinks completed events before callbacks and sets the output index on wait completion. |
| lib/uefi/debug_support.cpp | Fixes pool allocation size for debug image info “normal” entries to avoid overflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
102
to
+106
| auto status = event_wait_timeout(&ev->ev, 200); | ||
| if (status == ERR_TIMED_OUT) { | ||
| continue; | ||
| } | ||
| *index = i; |
Contributor
Author
There was a problem hiding this comment.
Right — in the current single-threaded app model the only values that come back are NO_ERROR and ERR_TIMED_OUT (nothing else can destroy the event while the app is blocked here), but there's no reason to bake that assumption in. Updated: any other status now returns EFI_STATUS_DEVICE_ERROR instead of reporting a signaled event.
When event_wait_timeout completed without timing out, wait_for_event returned EFI_STATUS_SUCCESS without storing which event finished the wait, so callers read an uninitialized index. Store the index of the event that completed, and report EFI_STATUS_DEVICE_ERROR instead of success if the wait itself failed (e.g. the event was destroyed while being waited on).
zhangxp1998
force-pushed
the
uefi-memory-safety-fixes
branch
from
August 29, 2026 18:41
fdb40e6 to
254a1f1
Compare
travisg
approved these changes
Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four independent memory-safety fixes in lib/uefi, found while auditing the loader:
allocate the right size for debug image info entries:
efi_core_new_debug_image_info_entry()allocatedsizeof(union EfiDebugImageInfo)(8 bytes, a single pointer) for thenormal_imageentry and then wrote a 24-bytestruct EfiDebugImageInfoNormalthrough it, a 16-byte pool overflow on every image load.fully initialize the simple text output protocol:
get_text_output_protocol()returned a struct with every member exceptoutput_stringuninitialized. An application callingreset,clear_screen, orquery_mode, or dereferencingmode, jumped through stack garbage. All callbacks now have benign implementations andmodepoints at a static 80x25 description.unlink completed events before invoking callbacks:
process_pending_events()moved ready events onto a stack-local list and left the nodes chained there after returning. A laterclose_event/check_eventpasseddelete_if_in_list's non-null check and wrote through pointers into the dead stack frame. Events are now popped off the local list (which clears their links) before their callbacks run.set the output index when wait_for_event succeeds: the
event_wait_timeoutsuccess path returned without storing which event finished the wait, so callers read an uninitialized index.Validation (qemu-virt-arm64-test, Clang/LLD, WERROR=1):
helloworld_aa64.efiloads and runs (return code 0),ut allpasses 36/36, no faults in the loguefi_loadruns both succeed with PMMfree_countidentical before/after,ut all36/36The series is based on current master and is independent of #530; the two touch different hunks and apply cleanly in either order.