Skip to content

Conversation

@zurcher
Copy link
Contributor

@zurcher zurcher commented Mar 3, 2025

Description

Add a NULL check in IsValidCapsuleHeader before dereferencing CapsuleHeader

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?
  • Backport to release branch?

How This Was Tested

Published HOB with EFI_HOB_TYPE_UEFI_CAPSULE and BaseAddress = 0.
Confirmed IsValidCapsuleHeader returned FALSE instead of crashing.

Integration Instructions

N/A

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2025

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Please upload report for BASE (dev/202502@8264a11). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...ModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c 0.00% 4 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev/202502    #1290   +/-   ##
=============================================
  Coverage              ?    0.64%           
=============================================
  Files                 ?      614           
  Lines                 ?   222504           
  Branches              ?      370           
=============================================
  Hits                  ?     1432           
  Misses                ?   221059           
  Partials              ?       13           
Flag Coverage Δ
MdeModulePkg 0.64% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zurcher
Copy link
Contributor Author

zurcher commented Mar 3, 2025

Build failure is not in my change:

ERROR - Compiler #error from /__w/1/s/UnitTestFrameworkPkg/Library/Posix/MemoryAllocationLibPosix/MemoryAllocationLibPosix.c variable ‘Length’ set but not used [-Werror=unused-but-set-variable]

@makubacki makubacki requested a review from apop5 March 4, 2025 00:21
@makubacki makubacki enabled auto-merge (squash) March 4, 2025 00:50
@makubacki makubacki disabled auto-merge March 4, 2025 00:50
@makubacki makubacki enabled auto-merge (rebase) March 4, 2025 00:50
auto-merge was automatically disabled March 12, 2025 17:30

Head branch was pushed to by a user without write access

@zurcher
Copy link
Contributor Author

zurcher commented Mar 12, 2025

Test failure:

Address 0x7f78a4c0b078 is located in stack of thread T0 at offset 120 in frame
    #0 0x40318f in HostTestRecordAssertion /__w/1/s/MdeModulePkg/Universal/Variable/RuntimeDxe/RuntimeDxeUnitTest/SctShim.c:200

  This frame has 5 object(s):
    [32, 52) 'AssertionType' (line 204)
    [96, 120) 'Marker' (line 201) <== Memory access at offset 120 overflows this variable
    [160, 2208) 'AssertionDetail' (line 203)
    [2336, 2352) 'EventId' (line 173)
    [2368, 3392) 'AsciiBuffer' (line 202)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /__w/1/s/MdePkg/Library/BasePrintLib/PrintLibInternal.c:1037 in BasePrintLibSPrintMarker

@apop5 apop5 merged commit 68d85db into microsoft:dev/202502 Mar 13, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants