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

["Nickle Wang" <nicklew@nvidia.com>]

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@intel.com>
> Sent: Thursday, June 29, 2023 3:07 AM
> To: devel@edk2.groups.io; Nickle Wang <nicklew@nvidia.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu
> <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy
> <igork@ami.com>; Nick Ramirez <nramirez@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Nickle Wang
> via groups.io
> Sent: Tuesday, June 27, 2023 5:56 PM
> To: devel@edk2.groups.io; Nickle Wang <nicklew@nvidia.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu
> <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy
> <igork@ami.com>; Nick Ramirez <nramirez@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Nickle
> > Wang via groups.io
> > Sent: Friday, February 10, 2023 8:34 PM
> > To: devel@edk2.groups.io
> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu
> > <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>; Igor
> > Kulchytskyy <igork@ami.com>; Nick Ramirez <nramirez@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@nvidia.com>
> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > Cc: Abner Chang <abner.chang@amd.com>
> > Cc: Igor Kulchytskyy <igork@ami.com>
> > Cc: Nick Ramirez <nramirez@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
> >
> >
> >
> >
> >
> 
> 
> 
> 
>