DfciPkg: make manager + settings drivers dispatch-order resilient#369
Draft
kat-perez wants to merge 1 commit into
Draft
DfciPkg: make manager + settings drivers dispatch-order resilient#369kat-perez wants to merge 1 commit into
kat-perez wants to merge 1 commit into
Conversation
1173270 to
1289545
Compare
DfciManager currently calls LocateProtocol() at entry for the three
gDfciApply*ProtocolGuid producers (Identity, Permissions, Settings).
This is guarded only by [Depex], which BdsDxe honors but alternate
dispatchers do not. If the consumer runs before the producers, the
static pointers stay NULL and a later ProcessMailBoxes call null-derefs.
This commit makes DfciManager dispatch-order resilient without changing
semantics under EDK2 BdsDxe:
* Replace the three entry-time LocateProtocol calls with
RegisterProtocolNotify. Each callback populates its static when
the producer installs; when all three are present, a new
DfciManagerCompleteInit() runs the original sequence
(AllocateManagerData -> EndOfDxe callback registration ->
QueueMailboxAtSettingAccess). Under EDK2 the [Depex] still
ensures producers are present before this driver dispatches, so
the notifies fire synchronously when registered and CompleteInit
runs before entry returns - byte-identical observable behavior.
Under a dispatcher that ignores [Depex], CompleteInit runs when
the producers actually arrive.
* Fix two latent NULL derefs exposed when ProcessMailBoxes is
re-entered after FreeManagerData() (e.g. if a second SettingAccess
notify fires from a duplicate registration):
- Top-of-ProcessMailBoxes early-exit when
mManagerData[MGR_IDENTITY].Data is NULL.
- Split the existing combined `if (Data == NULL || Data->Packet
== NULL)` in ProcessPacket so the Data == NULL branch doesn't
reach the `Data->MailboxName` dereference inside its DEBUG.
[Depex] is left intact - belt and suspenders under EDK2, harmless
elsewhere.
SettingsManagerDxe is intentionally left unchanged in this commit.
Its gDfciStartOfBdsNotifyGuid trigger is correct for EDK2 BdsDxe and
moving it to gEfiEndOfDxeEventGroupGuid introduces re-entrant notify
dispatch during EndOfDxe that the EDK2 DxeCore notify iterator does
not survive (SemmManager's CloseEvent-from-callback pattern corrupts
the iterator). Alternate dispatchers that need SettingAccess
publication should signal gDfciStartOfBdsNotifyGuid explicitly in
their BDS phase, after EndOfDxe completes.
1289545 to
d6978df
Compare
|
@kat-perez please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
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.
Description
Makes
DfciManagerwork under any DXE dispatcher that ignores[Depex], without changing observable behavior under EDK2 BdsDxe:Replace the three entry-time
LocateProtocolcalls forgDfciApply*ProtocolGuid(Identity, Permissions, Settings) withRegisterProtocolNotify. Each callback populates its static when the producer installs; when all three are present, a newDfciManagerCompleteInit()runs the original sequence (AllocateManagerData→ EndOfDxe callback →QueueMailboxAtSettingAccess).Under EDK2 +
[Depex], producers are already present when this driver dispatches, so all three notifies fire synchronously on registration andCompleteInitruns before entry returns — byte-identical observable behavior.Under a dispatcher that ignores
[Depex],CompleteInitruns whenever the producers actually arrive.Also fixes two latent NULL derefs that become reachable when
ProcessMailBoxesis re-entered afterFreeManagerData()has cleared state (e.g. if a duplicategDfciSettingAccessProtocolGuidnotify registration causes the callback to fire twice):ProcessMailBoxesearly-exit whenmManagerData[MGR_IDENTITY].Datais NULL.if (Data == NULL || Data->Packet == NULL)inProcessPacketso theData == NULLbranch doesn't reach theData->MailboxNamedereference inside itsDEBUGmessage.[Depex]entries are left intact — belt-and-suspenders under EDK2, harmless elsewhere.How This Was Tested
Built
Msft900MaaPkg(DEBUG + RELEASE) under standard EDK2BdsDxeand verified normal boot throughbootmgfw.efiis byte-equivalent to the pre-change firmware. Override-hash validation against a downstream SEMM consumer (which usesDfciManager's[Sources]as its#Overridebaseline) passes after a one-line hash refresh on the consumer's.inf.The
ProcessMailBoxesre-entry NULL-deref was originally observed on a non-EDK2 dispatcher that registered duplicategDfciSettingAccessProtocolGuidnotifies — the top-of-function guard makes the second invocation safe regardless of dispatcher.Integration Instructions
N/A — backward compatible with all existing EDK2 BdsDxe consumers.
Note:
SettingsManagerDxe'sgDfciStartOfBdsNotifyGuidtrigger is intentionally left unchanged. Earlier iterations of this work tried moving it togEfiEndOfDxeEventGroupGuidfor universality, but that introduces re-entrant notify dispatch during EndOfDxe that the standardMdeModulePkg/Core/Dxe/DxeMainnotify iterator does not survive (downstream consumers like SEMM thatCloseEventfrom inside the callback corrupt the iterator). Alternate dispatchers that needgDfciSettingAccessProtocolGuidpublication should signalgDfciStartOfBdsNotifyGuidexplicitly from their own BDS phase after EndOfDxe completes.