Re: [edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
On Sat, 20 Apr 2024 at 17:31, Michael Brown 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
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! Many thanks in advance for any time you might have to reply. Mike Beaton -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118016): https://edk2.groups.io/g/devel/message/118016 Mute This Topic: https://groups.io/mt/105368366/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
On 06/04/2024 16:53, Mike Beaton wrote: The existing uninstall call was passing the wrong handle (parent object, not the correct child object) and additionally passing the address of a pointer to the interface to be removed rather than the pointer itself, so always failed with EFI_NOT_FOUND. After altering these, we add an ASSERT which confirms that the modified uninstall is succeeding. Cc: Maciej Rabeda Cc: Jiaxin Wu Cc: Siyuan Fu Signed-off-by: Mike Beaton --- NetworkPkg/HttpBootDxe/HttpBootImpl.c | 21 - 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c b/NetworkPkg/HttpBootDxe/HttpBootImpl.c index b4c61925b9..100b721ad4 100644 --- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c +++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c @@ -77,12 +77,23 @@ HttpBootUninstallCallback ( IN HTTP_BOOT_PRIVATE_DATA *Private ) { + EFI_STATUS Status; + EFI_HANDLE ControllerHandle; + if (Private->HttpBootCallback == >LoadFileCallback) { -gBS->UninstallProtocolInterface ( - Private->Controller, - , - >HttpBootCallback - ); +if (!Private->UsingIpv6) { + ControllerHandle = Private->Ip4Nic->Controller; +} else { + ControllerHandle = Private->Ip6Nic->Controller; +} + +Status = gBS->UninstallProtocolInterface ( +ControllerHandle, +, +Private->HttpBootCallback +); +ASSERT_EFI_ERROR (Status); This assertion is not necessarily safe. Uninstallation of protocol interfaces is allowed to fail under the UEFI model. (This is arguably insane, but that's a separate discussion.) This could therefore potentially result in a perfectly valid sequence of events leading to an ASSERT() failure. Thanks, Michael -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117469): https://edk2.groups.io/g/devel/message/117469 Mute This Topic: https://groups.io/mt/105368366/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol
The existing uninstall call was passing the wrong handle (parent object, not the correct child object) and additionally passing the address of a pointer to the interface to be removed rather than the pointer itself, so always failed with EFI_NOT_FOUND. After altering these, we add an ASSERT which confirms that the modified uninstall is succeeding. Cc: Maciej Rabeda Cc: Jiaxin Wu Cc: Siyuan Fu Signed-off-by: Mike Beaton --- NetworkPkg/HttpBootDxe/HttpBootImpl.c | 21 - 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c b/NetworkPkg/HttpBootDxe/HttpBootImpl.c index b4c61925b9..100b721ad4 100644 --- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c +++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c @@ -77,12 +77,23 @@ HttpBootUninstallCallback ( IN HTTP_BOOT_PRIVATE_DATA *Private ) { + EFI_STATUS Status; + EFI_HANDLE ControllerHandle; + if (Private->HttpBootCallback == >LoadFileCallback) { -gBS->UninstallProtocolInterface ( - Private->Controller, - , - >HttpBootCallback - ); +if (!Private->UsingIpv6) { + ControllerHandle = Private->Ip4Nic->Controller; +} else { + ControllerHandle = Private->Ip6Nic->Controller; +} + +Status = gBS->UninstallProtocolInterface ( +ControllerHandle, +, +Private->HttpBootCallback +); +ASSERT_EFI_ERROR (Status); + Private->HttpBootCallback = NULL; } } -- 2.44.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117468): https://edk2.groups.io/g/devel/message/117468 Mute This Topic: https://groups.io/mt/105368366/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-