[edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver binding start issue.

[Nickle Wang via groups.io]

Hi Saloni,

Thanks for your review. 

When uninstall fails, per UEFI specification, the protocol will be installed again and will be visible to UEFI drivers.

Page 190, UEFI spec. 2.10:
"If any errors are generated while the
protocol interfaces are being uninstalled, then the protocols uninstalled prior to the error will be reinstalled with the
boot service EFI_BOOT_SERVICES.InstallProtocolInterface() and the status code EFI_INVALID_PARAMETER is
returned."

In this case, if we do FreePool while driver still can locate gEfiHttpServiceBindingProtocolGuid. Driver will access to the 
memory that is released to system. Memory issue may happen.

Regards,
Nickle

> -----Original Message-----
> From: Kasbekar, Saloni <saloni.kasbekar at intel.com>
> Sent: Thursday, June 29, 2023 3:07 AM
> To: devel at edk2.groups.io; Nickle Wang <nicklew at nvidia.com>
> Cc: Maciej Rabeda <maciej.rabeda at linux.intel.com>; Siyuan Fu
> <siyuan.fu at intel.com>; Abner Chang <abner.chang at amd.com>; Igor Kulchytskyy
> <igork at ami.com>; Nick Ramirez <nramirez at nvidia.com>
> Subject: RE: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver binding
> start issue.
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Nickle,
> 
> We would want to do the FreePool even if the Uninstall fails (like in the case
> where we failed to install the multiple protocol interfaces and then went to
> ON_ERROR). Do you think it's better if we change it to -
> 
>   if (HttpService != NULL) {
>     HttpCleanService (HttpService, UsingIpv6);
>     Status = gBS->UninstallMultipleProtocolInterfaces (
>                     &ControllerHandle,
>                     &gEfiHttpServiceBindingProtocolGuid,
>                     &HttpService->ServiceBinding,
>                     NULL
>                     );
>     if ((HttpService->Tcp4ChildHandle == NULL) && (HttpService->Tcp6ChildHandle
> == NULL)) {
>         FreePool (HttpService);
>     }
>   }
> 
> Thanks,
> Saloni
> 
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Nickle Wang
> via groups.io
> Sent: Tuesday, June 27, 2023 5:56 PM
> To: devel at edk2.groups.io; Nickle Wang <nicklew at nvidia.com>
> Cc: Maciej Rabeda <maciej.rabeda at linux.intel.com>; Siyuan Fu
> <siyuan.fu at intel.com>; Abner Chang <abner.chang at amd.com>; Igor Kulchytskyy
> <igork at ami.com>; Nick Ramirez <nramirez at nvidia.com>
> Subject: Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver binding
> start issue.
> 
> May I know if someone can help to review this patch?
> 
> Thanks,
> Nickle
> 
> > -----Original Message-----
> > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Nickle
> > Wang via groups.io
> > Sent: Friday, February 10, 2023 8:34 PM
> > To: devel at edk2.groups.io
> > Cc: Maciej Rabeda <maciej.rabeda at linux.intel.com>; Siyuan Fu
> > <siyuan.fu at intel.com>; Abner Chang <abner.chang at amd.com>; Igor
> > Kulchytskyy <igork at ami.com>; Nick Ramirez <nramirez at nvidia.com>
> > Subject: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver
> > binding start issue.
> >
> > External email: Use caution opening links or attachments
> >
> >
> > When failure happens in HttpDxeStart, the error handling code release
> > the memory buffer but it does not uninstall HTTP service bindnig
> > protocol. As the result, application can still locate this protocol
> > and invoke service binding fucntions in released memory pool.
> >
> > Signed-off-by: Nickle Wang <nicklew at nvidia.com>
> > Cc: Maciej Rabeda <maciej.rabeda at linux.intel.com>
> > Cc: Siyuan Fu <siyuan.fu at intel.com>
> > Cc: Abner Chang <abner.chang at amd.com>
> > Cc: Igor Kulchytskyy <igork at ami.com>
> > Cc: Nick Ramirez <nramirez at nvidia.com>
> > ---
> >  NetworkPkg/HttpDxe/HttpDriver.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/NetworkPkg/HttpDxe/HttpDriver.c
> > b/NetworkPkg/HttpDxe/HttpDriver.c index 5d918d3c4d..f6d1263cad 100644
> > --- a/NetworkPkg/HttpDxe/HttpDriver.c
> > +++ b/NetworkPkg/HttpDxe/HttpDriver.c
> > @@ -3,6 +3,7 @@
> >
> >    Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
> >    (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> > +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -464,8 +465,16 @@ ON_ERROR:
> >
> >    if (HttpService != NULL) {
> >      HttpCleanService (HttpService, UsingIpv6);
> > -    if ((HttpService->Tcp4ChildHandle == NULL) && (HttpService-
> > >Tcp6ChildHandle == NULL)) {
> > -      FreePool (HttpService);
> > +    Status = gBS->UninstallMultipleProtocolInterfaces (
> > +                    &ControllerHandle,
> > +                    &gEfiHttpServiceBindingProtocolGuid,
> > +                    &HttpService->ServiceBinding,
> > +                    NULL
> > +                    );
> > +    if (!EFI_ERROR (Status)) {
> > +      if ((HttpService->Tcp4ChildHandle == NULL) && (HttpService-
> > >Tcp6ChildHandle == NULL)) {
> > +        FreePool (HttpService);
> > +      }
> >      }
> >    }
> >
> > --
> > 2.39.1.windows.1
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106488): https://edk2.groups.io/g/devel/message/106488
Mute This Topic: https://groups.io/mt/99821789/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-