Re: [edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol

["Mike Beaton" <mjsbeaton@gmail.com>]

On Sat, 20 Apr 2024 at 17:31, Michael Brown <mcb30@ipxe.org> wrote:
>
> On 19/04/2024 11:02, Mike Beaton wrote:
> > Dear Michael,
> >
> > I don't know if you had time to answer one follow-up question.
> >
> > Obviously one thing that someone might want to do is to notify on
> > protocol installs and trap installs of this protocol - e.g. so that
> > something other than UefiBootManagerLib can manage and monitor HTTP
> > boot, but still allowing the original callback to occur, by hooking it.
> > Not sure if this counts as 'supported' or not (possibly not...) though I
> > think it may count as 'quite likely to happen'. However, one could hook
> > in such a way that the uninstall would succeed anyway, assuming that the
> > function pointer within the original installed protocol is writeable.
> >
> > My question is: was the above is roughly what you were thinking of, that
> > might cause the assert to fail, or, if not, if you had the time to give
> > a very brief sketch of what else it might be (just a plausible, very
> > rough example)? Certainly not saying you're wrong, just that it would be
> > helpful (to me!) to understand what sort of thing you were thinking of!
>
> I don't have a specific use case in mind for why someone might want to
> have opened this particular protocol in a way that would subsequently
> cause UninstallMultipleProtocolInterfaces() to fail (e.g. opening with
> BY_CHILD_CONTROLLER attributes).  Just that, as a general rule, there
> exists a design flaw in the UEFI specification that means that
> operations that should have been chosen at the design stage to be
> conceptually impossible to fail (such as freeing memory or uninstalling
> protocols) are instead allowed to return a failure status.
>
> This design issue manifests itself as extremely unreliable behaviour on
> the removal or shutdown paths of many UEFI drivers.  For example: many
> drivers will simply deadlock the system if disconnected from their
> underlying controllers (e.g. via the UEFI shell "disconnect" command).
>
> In the case of UninstallMultipleProtocolInterfaces(), the failure mode
> is particularly problematic since the specification dictates that the
> firmware must do the absolutely worst thing possible by *reinstalling*
> any protocol instances that it had managed to uninstall, and
> consequently retriggering driver Start() method calls.  This generally
> leads to chaos and confusion (and use-after-free bugs that could
> probably be fairly easily extended to obtain a Secure Boot exploit).
>
> There's nothing that you really need to do specifically in HttpBootDxe
> to work around this design flaw.  But it's definitely worth removing the
> unjustified ASSERT(), since that ASSERT() may cause a crash in a system
> that could otherwise continue to operate successfully.
>
> Hope that helps,
>
> Michael
>

It does help. Thank you for a useful and clear explanation - I was
already aware of some (but certainly not all) of it.

I have already posted a revised patch with the ASSERT removed - but am
now more clear that I really had better stick with that, not try to
argue against it. ;)

Thanks again,

Mike


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118052): https://edk2.groups.io/g/devel/message/118052
Mute This Topic: https://groups.io/mt/105368366/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-