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

["Michael Brown" <mcb30@ipxe.org>]

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118048): https://edk2.groups.io/g/devel/message/118048
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]
-=-=-=-=-=-=-=-=-=-=-=-