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

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

[Mike Beaton]

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 <maciej.rabeda@linux.intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>
---
 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 == &Private->LoadFileCallback) {
-    gBS->UninstallProtocolInterface (
-           Private->Controller,
-           &gEfiHttpBootCallbackProtocolGuid,
-           &Private->HttpBootCallback
-           );
+    if (!Private->UsingIpv6) {
+      ControllerHandle = Private->Ip4Nic->Controller;
+    } else {
+      ControllerHandle = Private->Ip6Nic->Controller;
+    }
+
+    Status = gBS->UninstallProtocolInterface (
+                    ControllerHandle,
+                    &gEfiHttpBootCallbackProtocolGuid,
+                    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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Michael Brown]

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 <maciej.rabeda@linux.intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>
> ---
>   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 == &Private->LoadFileCallback) {
> -    gBS->UninstallProtocolInterface (
> -           Private->Controller,
> -           &gEfiHttpBootCallbackProtocolGuid,
> -           &Private->HttpBootCallback
> -           );
> +    if (!Private->UsingIpv6) {
> +      ControllerHandle = Private->Ip4Nic->Controller;
> +    } else {
> +      ControllerHandle = Private->Ip6Nic->Controller;
> +    }
> +
> +    Status = gBS->UninstallProtocolInterface (
> +                    ControllerHandle,
> +                    &gEfiHttpBootCallbackProtocolGuid,
> +                    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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Mike Beaton]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 1910 bytes --]

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

[Michael Brown]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Mike Beaton]

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]
-=-=-=-=-=-=-=-=-=-=-=-