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

["Saloni Kasbekar" <saloni.kasbekar@intel.com>]

Hi Nickle,

That makes sense. Thanks for the clarification.

Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>

Thanks,
Saloni

-----Original Message-----
From: Nickle Wang <nicklew@nvidia.com> 
Sent: Wednesday, June 28, 2023 3:30 PM
To: Kasbekar, Saloni <saloni.kasbekar@intel.com>; 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: RE: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver binding start issue.

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