From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mx.groups.io with SMTP id smtpd.web12.12851.1600174908494701241 for ; Tue, 15 Sep 2020 06:01:48 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 134.134.136.65, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: K5lvZmwuXF2GzMHXfG/LuXkYE6vzrHUtAi91VRe6wui0bJ5s18+JPNkRQCBb4VyRLKeiiY8UNt tZB6R8NSr3PA== X-IronPort-AV: E=McAfee;i="6000,8403,9744"; a="159297632" X-IronPort-AV: E=Sophos;i="5.76,430,1592895600"; d="scan'208,217";a="159297632" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Sep 2020 06:01:34 -0700 IronPort-SDR: f75mOFFlAtXntSd6ICycAoh5eHuch0DplHdDO/p0uM6zJh4IBc5Q052RM8jYpsmy2rwuMyKloI pwCr+0cctUhw== X-IronPort-AV: E=Sophos;i="5.76,430,1592895600"; d="scan'208,217";a="335639068" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.21.135]) ([10.213.21.135]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Sep 2020 06:01:32 -0700 Subject: Re: [edk2-devel] [PATCH] NetworkPkg/HttpDxe: Clear TlsChildHandle during cleanup To: devel@edk2.groups.io, scott@scott.ph, Laszlo Ersek Cc: Rebecca Cran References: <20200905011540.6847-1-scott@scott.ph> From: "Maciej Rabeda" Message-ID: Date: Tue, 15 Sep 2020 15:01:27 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------8FE5F99E935B8E7D262E6CA5" Content-Language: pl --------------8FE5F99E935B8E7D262E6CA5 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hi Scott, Thanks for submitting the patch - I am about to approve the patch, however - a couple of small remarks to the commit message. 1. Please remove the "From:" line 2. Please add "Cc:" lines before "Signed-off-by". Cc people are added based on appropriate EDK2 package maintainer & reviewer list: https://github.com/tianocore/edk2/blob/master/Maintainers.txt Example of a patch from NetworkPkg: https://github.com/tianocore/edk2/commit/0716b2390f005e84961cb98af28bd16cdcc5db42 Thanks, Maciej On 08-Sep-20 06:50, D Scott Phillips wrote: > On Monday, September 7, 2020 4:33 AM, Laszlo Ersek wrote: > >> Hi Scott, >> >> (+Rebecca) >> >> On 09/05/20 03:15, D Scott Phillips wrote: >> >>> *From: D Scott Phillips d.scott.phillips@amperecomputing.com* >>> Leaving TlsChildHandle with the stale handle causes later use of https >>> with the http instance to incorrectly skip tls reconfiguration, use >>> the stale handle, and eventually call a garbage function pointer. >>> >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1917 >>> Signed-off-by: D Scott Phillips d.scott.phillips@amperecomputing.com >>> >>> ------------------------------------------------------------------------------------------------------------------------------ >>> >>> NetworkPkg/HttpDxe/HttpProto.c | 1 + >>> 1 file changed, 1 insertion(+) >>> diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c >>> index 3c7c6ff9f0..afc7db5a72 100644 >>> --- a/NetworkPkg/HttpDxe/HttpProto.c >>> +++ b/NetworkPkg/HttpDxe/HttpProto.c >>> @@ -873,6 +873,7 @@ HttpCleanProtocol ( >>> // Destroy the TLS instance. >>> // >>> HttpInstance->TlsSb->DestroyChild (HttpInstance->TlsSb, HttpInstance->TlsChildHandle); >>> >>> - HttpInstance->TlsChildHandle = NULL; >>> } >>> >>> >>> if (HttpInstance->Tcp4ChildHandle != NULL) { >> thanks a lot for tracking this down! >> >> I've reopened BZ#1917, and linked your patch email in a new comment. >> >> But, I'd also like to assign the BZ to you, if that's OK with you. Can >> you please register in the TianoCore bugzilla instance for that? > Certainly, account created and assignment taken. Thanks Laszlo. > > Scott > > > > --------------8FE5F99E935B8E7D262E6CA5 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit Hi Scott,

Thanks for submitting the patch - I am about to approve the patch, however - a couple of small remarks to the commit message.
1. Please remove the "From:" line
2. Please add "Cc:" lines before "Signed-off-by". Cc people are added based on appropriate EDK2 package maintainer & reviewer list: https://github.com/tianocore/edk2/blob/master/Maintainers.txt
Example of a patch from NetworkPkg: https://github.com/tianocore/edk2/commit/0716b2390f005e84961cb98af28bd16cdcc5db42

Thanks,
Maciej

On 08-Sep-20 06:50, D Scott Phillips wrote:
On Monday, September 7, 2020 4:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:


Hi Scott,

(+Rebecca)

On 09/05/20 03:15, D Scott Phillips wrote:


From: D Scott Phillips d.scott.phillips@amperecomputing.com
Leaving TlsChildHandle with the stale handle causes later use of https
with the http instance to incorrectly skip tls reconfiguration, use
the stale handle, and eventually call a garbage function pointer.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1917
Signed-off-by: D Scott Phillips d.scott.phillips@amperecomputing.com

------------------------------------------------------------------------------------------------------------------------------

NetworkPkg/HttpDxe/HttpProto.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
index 3c7c6ff9f0..afc7db5a72 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -873,6 +873,7 @@ HttpCleanProtocol (
// Destroy the TLS instance.
//
HttpInstance->TlsSb->DestroyChild (HttpInstance->TlsSb, HttpInstance->TlsChildHandle);

-   HttpInstance->TlsChildHandle = NULL;
    }


if (HttpInstance->Tcp4ChildHandle != NULL) {

thanks a lot for tracking this down!

I've reopened BZ#1917, and linked your patch email in a new comment.

But, I'd also like to assign the BZ to you, if that's OK with you. Can
you please register in the TianoCore bugzilla instance for that?

Certainly, account created and assignment taken. Thanks Laszlo.

Scott

--------------8FE5F99E935B8E7D262E6CA5--