MdePkg: Fix overflow issue in PeCoffLoaderRelocateImageForRuntime #10617
Conversation
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
github-actions
bot
added
the
impact:security
tianocore-assign-reviewers
bot
requested review from
LiuZhiguang001,
lgao4 and
mdkinney
Member
|
The SafeIntLib would be a better choice to help do this check. Also, the failure condition you have added is silent. An invalid relocation that overflows would be skipped with no messages. Should a DEBUG() message be added and should the entire relocation operation fail if this overflow condition is present? |
sachinami
force-pushed
the
Sachin/BasePeCoffLib
branch
from
0943cc0 to
1361505
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mdkinney
reviewed
Jan 23, 2025
RelocDir->Size is a UINT32 value, and RelocDir->VirtualAddress is also a UINT32 value. The current code in PeCoffLoaderRelocateImageForRuntime does not check for overflow when adding RelocDir->Size to RelocDir->VirtualAddress. This patch uses SafeIntLib to ensure that the addition does not overflow. Signed-off-by: Sachin Ganesh <sachinganesh@ami.com>
Used SafeIntLib to handle the overflow check in PeCoffLoaderRelocateImage Signed-off-by: Sachin Ganesh <sachinganesh@ami.com>
sachinami
force-pushed
the
Sachin/BasePeCoffLib
branch
from
1361505 to
b0b3af2
Compare
Member
|
For the build failures, I recommend adding a mapping of |
SafeIntLib has been added to UnitTestFrameworkPkg Common Includes DSC for usage in host and target based tests. Signed-off-by: Sachin Ganesh <sachinganesh@ami.com>
Contributor
Author
|
Thank you for your guidance @mdkinney |
mdkinney
approved these changes
Jan 24, 2025
makubacki
approved these changes
Jan 24, 2025
Member
|
@mergify refresh |
✅ Pull request refreshed |
Member
|
This series breaks the build on Clang: Please provide a fix asap |
3 tasks
Contributor
Author
Fixed in #10689 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Description
Similar to #6249
RelocDir->Size is a UINT32 value, and RelocDir->VirtualAddress is also a UINT32 value. The current code in
PeCoffLoaderRelocateImageForRuntime() does not check for overflow when adding RelocDir->Size to RelocDir->VirtualAddress. This patch adds a check using SafeIntLib to ensure that the addition does not overflow.
Also added SafeIntLib to UnitTestFrameworkPkg/UnitTestFrameworkPkgCommon.dsc.inc for usage in target and host based tests.
How This Was Tested
The fix has been tested in real platform and the image is confirmed to be booting fine.
Integration Instructions
N/A