Various Fixes of Potential Undefined Behavior (LLM Assisted) (5/N) - #2083
Merged
Merged
Conversation
`SimpleNetwork::mode` returns a reference into the mode structure owned by the firmware, while every method that calls into the firmware took `&self`. Start, Stop, Initialize, Shutdown, ReceiveFilters, StationAddress and GetStatus are specified to update that structure, so a caller could hold the reference from `mode` across a call that overwrites the memory behind it. That is undefined behavior, and Miri rejects it under both aliasing models. Take `&mut self` in these methods, plus `reset`, which reinitializes the adapter. The borrow checker then rejects holding the mode reference across them. `transmit`, `receive`, the statistics and NVRAM accessors and `mcast_ip_to_mac` do not update the mode and keep `&self`. Found with Miri using a mocked firmware.
`Shell::current_dir` and `Shell::var` return references to strings that the shell owns, while `set_current_dir` and `set_var` took `&self`. The EDK2 shell frees and reallocates that storage when the value changes, so a string obtained before the setter dangled afterwards while still being usable from safe code. Take `&mut self` in the two setters so that the borrow checker rejects holding such a string across them. This also covers the `Vars` iterator, which borrows the shell for as long as it is alive. Found by inspection of the EDK2 shell implementation.
The specification declares `This` as an input parameter of `EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.QueryMode()`, and the implementations in EDK2 only read from it. The `uefi` wrapper nevertheless had to cast the shared reference behind `Output::current_mode` and `Output::modes` to `*mut` to satisfy the function pointer type. Writing through such a pointer would be undefined behavior, so the type should not invite it. Declare the parameter as `*const Self` and pass the shared reference directly, as was done for `SimplePointerProtocol::get_state` in commit 3078571.
`with_stdin`, `with_stdout` and `with_stderr` create an exclusive reference to the protocol behind the raw pointer in the system table for the duration of the closure. A nested call, which is easy to make by accident through `println!` or the `log` macros from inside a `with_stdout` closure, creates a second exclusive reference to the same protocol while the first one is still in use. That is undefined behavior under the Stacked Borrows model, and Miri reports it. Preventing this at runtime would need a reentrancy guard on every call. Document the requirement instead.
`CStr8::from_ptr`, `CStr16::from_ptr` and `DevicePath::from_ffi_ptr` determine the length of the data by scanning for the NUL character or the end-entire node. Nothing bounds that scan, because the firmware interfaces that hand out such pointers do not report a length. The safety sections did not say that a missing terminator is undefined behavior rather than an error, nor that the memory must stay unchanged for the returned lifetime. Spell out both. `DevicePathNode::from_ffi_ptr` reads the length from the node header, so it needs readable memory for that many bytes.
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
1 participant
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.
Checklist