> -----Original Message-----
> From: Ye, Ting
> Sent: Friday, June 24, 2016 4:25 PM
> To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-devel@lists.01.org
> Cc: Fu, Siyuan <siyuan...@intel.com>; Zhang, Lubo <lubo.zh...@intel.com>
> Subject: RE: [Patch] NetworkPkg: Avoid potential NULL pointer dereference
>
> Looks good to me. I only have one small comment:
>
> + if (EFI_ERROR (Ikev2EncodePacket ((IKEV2_SESSION_COMMON *)
> SessionCommon, IkePacket, IkeType))) {
> + return NULL;
> + }
Ting,
Do you mean any compilers will meet failure or warnings with this one line code?
>
> Do we need divide it into two code lines to satisfy some compilers?
> Status = Ikev2EncodePacket ((IKEV2_SESSION_COMMON *)
> SessionCommon, IkePacket, IkeType)); if (EFI_ERROR (Status)) ...
>
> As long as our supported compilers will not report build errors or warnings, I
> am fine with the update.
>
> Reviewed-by: Ye Ting <ting...@intel.com>
>
>
>
> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Friday, June 24, 2016 3:33 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Zhang,
> Lubo <lubo.zh...@intel.com>
> Subject: [Patch] NetworkPkg: Avoid potential NULL pointer dereference
>
> The commit of 6b16c9e7 removes ASSERT and use error handling in IpSecDxe
> driver, but may cause the potential NULL pointer dereference. So, this patch
> is used to avoid NULL pointer dereference.
>
> Cc: Ye Ting <ting...@intel.com>
> Cc: Fu Siyuan <siyuan...@intel.com>
> Cc: Zhang Lubo <lubo.zh...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiaxin Wu <jiaxin...@intel.com>
> ---
> NetworkPkg/IpSecDxe/IkePacket.c | 6 +-
> NetworkPkg/IpSecDxe/Ikev2/ChildSa.c | 19 ++--
> NetworkPkg/IpSecDxe/Ikev2/Exchange.c | 10 ++-
> NetworkPkg/IpSecDxe/Ikev2/Payload.c | 3 +
> NetworkPkg/IpSecDxe/Ikev2/Sa.c | 163
> ++++++++++++++++++++++++++++++++++-
> 5 files changed, 188 insertions(+), 13 deletions(-)
>
> diff --git a/NetworkPkg/IpSecDxe/IkePacket.c
> b/NetworkPkg/IpSecDxe/IkePacket.c index 8fd395d..d5a938e 100644
> --- a/NetworkPkg/IpSecDxe/IkePacket.c
> +++ b/NetworkPkg/IpSecDxe/IkePacket.c
> @@ -1,9 +1,9 @@
> /** @file
> IKE Packet related operation.
>
> - Copyright (c) 2010, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2010 - 2016, Intel Corporation. All rights
> + reserved.<BR>
>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> http://opensource.org/licenses/bsd-license.php.
> @@ -201,11 +201,13 @@ IkeNetbufFromPacket (
> //
> // Convert Host order to Network order for IKE_PACKET header and
> payloads
> // Encryption payloads if needed
> //
> if (((IKEV2_SESSION_COMMON *) SessionCommon)->IkeVer == 2) {
> - Ikev2EncodePacket ((IKEV2_SESSION_COMMON *) SessionCommon,
> IkePacket, IkeType);
> + if (EFI_ERROR (Ikev2EncodePacket ((IKEV2_SESSION_COMMON *)
> SessionCommon, IkePacket, IkeType))) {
> + return NULL;
> + }
> } else {
> //
> //If IKEv1 support, check it here.
> //
> return NULL;
> diff --git a/NetworkPkg/IpSecDxe/Ikev2/ChildSa.c
> b/NetworkPkg/IpSecDxe/Ikev2/ChildSa.c
> index d3859e2..1f0199b 100644
> --- a/NetworkPkg/IpSecDxe/Ikev2/ChildSa.c
> +++ b/NetworkPkg/IpSecDxe/Ikev2/ChildSa.c
> @@ -1,9 +1,9 @@
> /** @file
> The operations for Child SA.
>
> - Copyright (c) 2010, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2010 - 2016, Intel Corporation. All rights
> + reserved.<BR>
>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> http://opensource.org/licenses/bsd-license.php.
> @@ -37,22 +37,25 @@ Ikev2CreateChildGenerator (
> IKEV2_CHILD_SA_SESSION *ChildSaSession;
> IKEV2_SA_SESSION *IkeSaSession;
> IKE_PACKET *IkePacket;
> IKE_PAYLOAD *NotifyPayload;
> UINT32 *MessageId;
> +
> + NotifyPayload = NULL;
> + MessageId = NULL;
>
> ChildSaSession = (IKEV2_CHILD_SA_SESSION *) SaSession;
> - IkePacket = IkePacketAlloc();
> - MessageId = NULL;
> -
> - if (IkePacket == NULL) {
> + if (ChildSaSession == NULL) {
> return NULL;
> }
> - if (ChildSaSession == NULL) {
> +
> + IkePacket = IkePacketAlloc();
> + if (IkePacket == NULL) {
> return NULL;
> }
>
> +
> if (Context != NULL) {
> MessageId = (UINT32 *) Context;
> }
>
> IkePacket->Header->Version = (UINT8) (2 << 4);
> @@ -111,10 +114,14 @@ Ikev2CreateChildGenerator (
> IKEV2_NOTIFICATION_NO_ADDITIONAL_SAS,
> NULL,
> NULL,
> 0
> );
> + if (NotifyPayload == NULL) {
> + IkePacketFree (IkePacket);
> + return NULL;
> + }
>
> IKE_PACKET_APPEND_PAYLOAD (IkePacket, NotifyPayload);
> //
> // TODO: Support the CREATE_CHILD_SA exchange.
> //
> diff --git a/NetworkPkg/IpSecDxe/Ikev2/Exchange.c
> b/NetworkPkg/IpSecDxe/Ikev2/Exchange.c
> index 9d58ab0a..1eddbfb 100644
> --- a/NetworkPkg/IpSecDxe/Ikev2/Exchange.c
> +++ b/NetworkPkg/IpSecDxe/Ikev2/Exchange.c
> @@ -1,9 +1,9 @@
> /** @file
> The general interfaces of the IKEv2.
>
> - Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2010 - 2016, Intel Corporation. All rights
> + reserved.<BR>
>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> http://opensource.org/licenses/bsd-license.php.
> @@ -493,10 +493,14 @@ Ikev2HandleSa (
> //
> ASSERT (IsListEmpty (&IkeSaSession->ChildSaSessionList) &&
> IsListEmpty (&IkeSaSession->ChildSaEstablishSessionList));
>
> ChildSaSession = Ikev2ChildSaSessionCreate (IkeSaSession, UdpService);
> + if (ChildSaSession == NULL) {
> + goto ON_ERROR;
> + }
> +
> ChildSaCommon = &ChildSaSession->SessionCommon;
> }
>
> //
> // Parse the IKE request packet according to the auth method and current
> state.
> @@ -517,10 +521,14 @@ Ikev2HandleSa (
> //
> ASSERT (IsListEmpty (&IkeSaSession->ChildSaSessionList) &&
> IsListEmpty (&IkeSaSession->ChildSaEstablishSessionList));
>
> ChildSaSession = Ikev2ChildSaSessionCreate (IkeSaSession, UdpService);
> + if (ChildSaSession == NULL) {
> + goto ON_ERROR;
> + }
> +
> ChildSaCommon = &ChildSaSession->SessionCommon;
>
> //
> // Initialize the SA data for Child SA.
> //
> diff --git a/NetworkPkg/IpSecDxe/Ikev2/Payload.c
> b/NetworkPkg/IpSecDxe/Ikev2/Payload.c
> index d5fe1ab..675ecf6 100644
> --- a/NetworkPkg/IpSecDxe/Ikev2/Payload.c
> +++ b/NetworkPkg/IpSecDxe/Ikev2/Payload.c
> @@ -2556,10 +2556,13 @@ Ikev2EncodePacket (
> if (SessionCommon->State >= IkeStateAuth) {
> //
> // Encrypt all payload and transfer IKE packet header from Host order to
> Network order.
> //
> Status = Ikev2EncryptPacket (SessionCommon, IkePacket);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> } else {
> //
> // Fill in the lenght into IkePacket header and transfer Host order to
> Network order.
> //
> IkePacket->Header->Length = (UINT32) (sizeof (IKE_HEADER) + IkePacket-
> >PayloadTotalSize); diff --git a/NetworkPkg/IpSecDxe/Ikev2/Sa.c
> b/NetworkPkg/IpSecDxe/Ikev2/Sa.c index c83d456..74ef79c 100644
> --- a/NetworkPkg/IpSecDxe/Ikev2/Sa.c
> +++ b/NetworkPkg/IpSecDxe/Ikev2/Sa.c
> @@ -443,10 +443,17 @@ Ikev2AuthPskGenerator (
>
>
> IkeSaSession = (IKEV2_SA_SESSION *) SaSession;
> ChildSaSession = IKEV2_CHILD_SA_SESSION_BY_IKE_SA (GetFirstNode
> (&IkeSaSession->ChildSaSessionList));
>
> + IkePacket = NULL;
> + IdPayload = NULL;
> + AuthPayload = NULL;
> + SaPayload = NULL;
> + TsiPayload = NULL;
> + TsrPayload = NULL;
> + NotifyPayload = NULL;
> CpPayload = NULL;
> NotifyPayload = NULL;
>
> //
> // 1. Allocate IKE Packet
> @@ -486,10 +493,13 @@ Ikev2AuthPskGenerator (
> //
> IdPayload = Ikev2GenerateIdPayload (
> &IkeSaSession->SessionCommon,
> IKEV2_PAYLOAD_TYPE_AUTH
> );
> + if (IdPayload == NULL) {
> + goto CheckError;
> + }
>
> //
> // 3. Generate Auth Payload
> // If it is tunnel mode, should create the configuration payload after
> the
> // Auth payload.
> @@ -520,20 +530,31 @@ Ikev2AuthPskGenerator (
> ChildSaSession->IkeSaSession,
> IKEV2_PAYLOAD_TYPE_SA,
> IKEV2_CFG_ATTR_INTERNAL_IP6_ADDRESS
> );
> }
> +
> + if (CpPayload == NULL) {
> + goto CheckError;
> + }
> + }
> +
> + if (AuthPayload == NULL) {
> + goto CheckError;
> }
>
> //
> // 4. Generate SA Payload according to the SA Data in ChildSaSession
> //
> SaPayload = Ikev2GenerateSaPayload (
> ChildSaSession->SaData,
> IKEV2_PAYLOAD_TYPE_TS_INIT,
> IkeSessionTypeChildSa
> );
> + if (SaPayload == NULL) {
> + goto CheckError;
> + }
>
> if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode ==
> EfiIPsecTransport) {
> //
> // Generate Tsi and Tsr.
> //
> @@ -560,10 +581,13 @@ Ikev2AuthPskGenerator (
> IKEV2_NOTIFICATION_USE_TRANSPORT_MODE,
> NULL,
> NULL,
> 0
> );
> + if (NotifyPayload == NULL) {
> + goto CheckError;
> + }
> } else {
> //
> // Generate Tsr for Tunnel mode.
> //
> TsiPayload = Ikev2GenerateTsPayload ( @@ -576,10 +600,14 @@
> Ikev2AuthPskGenerator (
> IKEV2_PAYLOAD_TYPE_NONE,
> FALSE
> );
> }
>
> + if (TsiPayload == NULL || TsrPayload == NULL) {
> + goto CheckError;
> + }
> +
> IKE_PACKET_APPEND_PAYLOAD (IkePacket, IdPayload);
> IKE_PACKET_APPEND_PAYLOAD (IkePacket, AuthPayload);
> if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode == EfiIPsecTunnel) {
> IKE_PACKET_APPEND_PAYLOAD (IkePacket, CpPayload);
> }
> @@ -589,10 +617,45 @@ Ikev2AuthPskGenerator (
> if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode ==
> EfiIPsecTransport) {
> IKE_PACKET_APPEND_PAYLOAD (IkePacket, NotifyPayload);
> }
>
> return IkePacket;
> +
> +CheckError:
> + if (IkePacket != NULL) {
> + IkePacketFree (IkePacket);
> + }
> +
> + if (IdPayload != NULL) {
> + IkePayloadFree (IdPayload);
> + }
> +
> + if (AuthPayload != NULL) {
> + IkePayloadFree (AuthPayload);
> + }
> +
> + if (CpPayload != NULL) {
> + IkePayloadFree (CpPayload);
> + }
> +
> + if (SaPayload != NULL) {
> + IkePayloadFree (SaPayload);
> + }
> +
> + if (TsiPayload != NULL) {
> + IkePayloadFree (TsiPayload);
> + }
> +
> + if (TsrPayload != NULL) {
> + IkePayloadFree (TsrPayload);
> + }
> +
> + if (NotifyPayload != NULL) {
> + IkePayloadFree (NotifyPayload);
> + }
> +
> + return NULL;
> }
>
> /**
> Parses IKE_AUTH packet.
>
> @@ -798,11 +861,15 @@ Ikev2AuthPskParser (
> }
>
> //
> // 5. Generate keymats for IPsec protocol.
> //
> - Ikev2GenerateChildSaKeys (ChildSaSession, NULL);
> + Status = Ikev2GenerateChildSaKeys (ChildSaSession, NULL); if
> + (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> if (IkeSaSession->SessionCommon.IsInitiator) {
> //
> // 6. Change the state of IkeSaSession
> //
> IKEV2_DUMP_STATE (IkeSaSession->SessionCommon.State,
> IkeStateIkeSaEstablished); @@ -932,11 +999,17 @@
> Ikev2AuthCertGenerator (
> }
>
> IkeSaSession = (IKEV2_SA_SESSION *) SaSession;
> ChildSaSession = IKEV2_CHILD_SA_SESSION_BY_IKE_SA (GetFirstNode
> (&IkeSaSession->ChildSaSessionList));
>
> + IkePacket = NULL;
> + IdPayload = NULL;
> + AuthPayload = NULL;
> CpPayload = NULL;
> + SaPayload = NULL;
> + TsiPayload = NULL;
> + TsrPayload = NULL;
> NotifyPayload = NULL;
> CertPayload = NULL;
> CertReqPayload = NULL;
>
> //
> @@ -979,10 +1052,13 @@ Ikev2AuthCertGenerator (
> &IkeSaSession->SessionCommon,
> IKEV2_PAYLOAD_TYPE_CERT,
> (UINT8 *)PcdGetPtr (PcdIpsecUefiCertificate),
> PcdGet32 (PcdIpsecUefiCertificateSize)
> );
> + if (IdPayload == NULL) {
> + goto CheckError;
> + }
>
> //
> // 3. Generate Certificate Payload
> //
> CertPayload = Ikev2GenerateCertificatePayload ( @@ -991,19 +1067,26 @@
> Ikev2AuthCertGenerator (
> (UINT8 *)PcdGetPtr (PcdIpsecUefiCertificate),
> PcdGet32 (PcdIpsecUefiCertificateSize),
> IKEV2_CERT_ENCODEING_X509_CERT_SIGN,
> FALSE
> );
> + if (CertPayload == NULL) {
> + goto CheckError;
> + }
> +
> if (IkeSaSession->SessionCommon.IsInitiator) {
> CertReqPayload = Ikev2GenerateCertificatePayload (
> IkeSaSession,
> IKEV2_PAYLOAD_TYPE_AUTH,
> (UINT8 *)PcdGetPtr (PcdIpsecUefiCertificate),
> PcdGet32 (PcdIpsecUefiCertificateSize),
> IKEV2_CERT_ENCODEING_HASH_AND_URL_OF_X509_CERT,
> TRUE
> );
> + if (CertReqPayload == NULL) {
> + goto CheckError;
> + }
> }
>
> //
> // 4. Generate Auth Payload
> // If it is tunnel mode, should create the configuration payload after
> the
> @@ -1042,20 +1125,31 @@ Ikev2AuthCertGenerator (
> ChildSaSession->IkeSaSession,
> IKEV2_PAYLOAD_TYPE_SA,
> IKEV2_CFG_ATTR_INTERNAL_IP6_ADDRESS
> );
> }
> +
> + if (CpPayload == NULL) {
> + goto CheckError;
> + }
> }
>
> + if (AuthPayload == NULL) {
> + goto CheckError;
> + }
> +
> //
> // 5. Generate SA Payload according to the Sa Data in ChildSaSession
> //
> SaPayload = Ikev2GenerateSaPayload (
> ChildSaSession->SaData,
> IKEV2_PAYLOAD_TYPE_TS_INIT,
> IkeSessionTypeChildSa
> );
> + if (SaPayload == NULL) {
> + goto CheckError;
> + }
>
> if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode ==
> EfiIPsecTransport) {
> //
> // Generate Tsi and Tsr.
> //
> @@ -1082,10 +1176,13 @@ Ikev2AuthCertGenerator (
> IKEV2_NOTIFICATION_USE_TRANSPORT_MODE,
> NULL,
> NULL,
> 0
> );
> + if (NotifyPayload == NULL) {
> + goto CheckError;
> + }
> } else {
> //
> // Generate Tsr for Tunnel mode.
> //
> TsiPayload = Ikev2GenerateTsPayload ( @@ -1098,10 +1195,14 @@
> Ikev2AuthCertGenerator (
> IKEV2_PAYLOAD_TYPE_NONE,
> FALSE
> );
> }
>
> + if (TsiPayload == NULL || TsrPayload == NULL) {
> + goto CheckError;
> + }
> +
> IKE_PACKET_APPEND_PAYLOAD (IkePacket, IdPayload);
> IKE_PACKET_APPEND_PAYLOAD (IkePacket, CertPayload);
> if (IkeSaSession->SessionCommon.IsInitiator) {
> IKE_PACKET_APPEND_PAYLOAD (IkePacket, CertReqPayload);
> }
> @@ -1115,10 +1216,53 @@ Ikev2AuthCertGenerator (
> if (IkeSaSession->Spd->Data->ProcessingPolicy->Mode ==
> EfiIPsecTransport) {
> IKE_PACKET_APPEND_PAYLOAD (IkePacket, NotifyPayload);
> }
>
> return IkePacket;
> +
> +CheckError:
> + if (IkePacket != NULL) {
> + IkePacketFree (IkePacket);
> + }
> +
> + if (IdPayload != NULL) {
> + IkePayloadFree (IdPayload);
> + }
> +
> + if (CertPayload != NULL) {
> + IkePayloadFree (CertPayload);
> + }
> +
> + if (CertReqPayload != NULL) {
> + IkePayloadFree (CertReqPayload);
> + }
> +
> + if (AuthPayload != NULL) {
> + IkePayloadFree (AuthPayload);
> + }
> +
> + if (CpPayload != NULL) {
> + IkePayloadFree (CpPayload);
> + }
> +
> + if (SaPayload != NULL) {
> + IkePayloadFree (SaPayload);
> + }
> +
> + if (TsiPayload != NULL) {
> + IkePayloadFree (TsiPayload);
> + }
> +
> + if (TsrPayload != NULL) {
> + IkePayloadFree (TsrPayload);
> + }
> +
> + if (NotifyPayload != NULL) {
> + IkePayloadFree (NotifyPayload);
> + }
> +
> + return NULL;
> }
>
> /**
> Parses IKE_AUTH packet.
>
> @@ -1338,11 +1482,15 @@ Ikev2AuthCertParser (
> }
>
> //
> // 5. Generat keymats for IPsec protocol.
> //
> - Ikev2GenerateChildSaKeys (ChildSaSession, NULL);
> + Status = Ikev2GenerateChildSaKeys (ChildSaSession, NULL); if
> + (EFI_ERROR (Status)) {
> + goto Exit;
> + }
> +
> if (IkeSaSession->SessionCommon.IsInitiator) {
> //
> // 6. Change the state of IkeSaSession
> //
> IKEV2_DUMP_STATE (IkeSaSession->SessionCommon.State,
> IkeStateIkeSaEstablished); @@ -1539,11 +1687,14 @@ Ikev2GenerateSaKeys
> (
> Status = EFI_SUCCESS;
>
> //
> // Generate Gxy
> //
> - Ikev2GenerateSaDhComputeKey (IkeSaSession->IkeKeys->DhBuffer,
> KePayload);
> + Status = Ikev2GenerateSaDhComputeKey
> + (IkeSaSession->IkeKeys->DhBuffer, KePayload); if (EFI_ERROR (Status)) {
> + goto Exit;
> + }
>
> //
> // Get the key length of Authenticaion, Encryption, PRF, and Integrity.
> //
> SaParams = IkeSaSession->SessionCommon.SaParams;
> @@ -1841,11 +1992,15 @@ Ikev2GenerateChildSaKeys (
>
> if (KePayload != NULL) {
> //
> // Generate Gxy
> //
> - Ikev2GenerateSaDhComputeKey (ChildSaSession->DhBuffer, KePayload);
> + Status = Ikev2GenerateSaDhComputeKey (ChildSaSession->DhBuffer,
> KePayload);
> + if (EFI_ERROR (Status)) {
> + goto Exit;
> + }
> +
> Fragments[0].Data = ChildSaSession->DhBuffer->GxyBuffer;
> Fragments[0].DataSize = ChildSaSession->DhBuffer->GxySize;
> }
>
> Fragments[1].Data = ChildSaSession->NiBlock;
> --
> 1.9.5.msysgit.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
