The current implementation doesn't handle the relationship between SPD and SAD well, which may introduce some security and connection issue after SPD updated. For SPD SetData policy: A) When delete the existed SPD entry, its related SAs also should be removed from its Sas list(SadEntry->BySpd). If the SA entry is established by IKE, we can remove it from global SAD list(SadEntry->List) and then free it directly since its SpdEntry will be freed later. B) SPD SetData operation should do some setting date validity-check. For example, whether the SaId specified by setting Data is valid. If the setting date is invalid, EFI_INVALID_PARAMETER should be returned.
Cc: Ye Ting <ting...@intel.com> Cc: Fu Siyuan <siyuan...@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jiaxin Wu <jiaxin...@intel.com> --- NetworkPkg/IpSecDxe/IpSecConfigImpl.c | 68 +++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 22 deletions(-) diff --git a/NetworkPkg/IpSecDxe/IpSecConfigImpl.c b/NetworkPkg/IpSecDxe/IpSecConfigImpl.c index 8c7724c..e1b24e4 100644 --- a/NetworkPkg/IpSecDxe/IpSecConfigImpl.c +++ b/NetworkPkg/IpSecDxe/IpSecConfigImpl.c @@ -1,9 +1,9 @@ /** @file The implementation of IPSEC_CONFIG_PROTOCOL. - Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2009 - 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. @@ -209,11 +209,11 @@ CompareSpdSelector ( IsMatch = FALSE; return IsMatch; } // - // Compare the all LocalAddress fields in the two Spdselectors. + // Compare the all LocalAddress and RemoteAddress fields in the two Spdselectors. // First, SpdSel1->LocalAddress to SpdSel2->LocalAddress && Compare // SpdSel1->RemoteAddress to SpdSel2->RemoteAddress. If all match, return // TRUE. // for (Index = 0; Index < SpdSel1->LocalAddressCount; Index++) { @@ -370,11 +370,11 @@ IsSubSpdSelector ( ) { IsMatch = FALSE; } // - // Compare the all LocalAddress fields in the two Spdselectors. + // Compare the all LocalAddress and RemoteAddress fields in the two Spdselectors. // First, SpdSel1->LocalAddress to SpdSel2->LocalAddress && Compare // SpdSel1->RemoteAddress to SpdSel2->RemoteAddress. If all match, return // TRUE. // if (IsMatch) { @@ -427,13 +427,13 @@ IsSubSpdSelector ( IsMatch = FALSE; return IsMatch; } // - // Compare the all LocalAddress fields in the two Spdselectors. - // First, SpdSel1->LocalAddress to SpdSel2->LocalAddress && Compare - // SpdSel1->RemoteAddress to SpdSel2->RemoteAddress. If all match, return + // Compare the all LocalAddress and RemoteAddress fields in the two Spdselectors. + // First, SpdSel1->LocalAddress to SpdSel2->RemoteAddress && Compare + // SpdSel1->RemoteAddress to SpdSel2->LocalAddress. If all match, return // TRUE. // for (Index = 0; Index < SpdSel1->LocalAddressCount; Index++) { if (!IsInAddressInfoList ( &SpdSel1->LocalAddress[Index], @@ -1016,10 +1016,12 @@ UnfixPadEntry ( and its policy is not NULL. - The Action of Data is Protected, its policy mode is Tunnel, and its tunnel option is NULL. - The Action of Data is protected and its policy mode is not Tunnel and it tunnel option is not NULL. + - SadEntry requied to be set into new SpdEntry's Sas has + been found but it is invalid. @retval EFI_OUT_OF_RESOURCED The required system resource could not be allocated. @retval EFI_SUCCESS The specified configuration data was obtained successfully. **/ EFI_STATUS @@ -1037,10 +1039,11 @@ SetSpdEntry ( LIST_ENTRY *SpdSas; LIST_ENTRY *EntryInsertBefore; LIST_ENTRY *Entry; LIST_ENTRY *Entry2; LIST_ENTRY *NextEntry; + LIST_ENTRY *NextEntry2; IPSEC_SPD_ENTRY *SpdEntry; IPSEC_SAD_ENTRY *SadEntry; UINTN SpdEntrySize; UINTN Index; @@ -1095,15 +1098,26 @@ SetSpdEntry ( // Update the reverse ref of SAD entry in the SPD.sas list. // SpdSas = &SpdEntry->Data->Sas; // - // TODO: Deleted the related SAs. + // Remove the related SAs from Sas(SadEntry->BySpd). If the SA entry is established by + // IKE, remove from mConfigData list(SadEntry->List) and then free it directly since its + // SpdEntry will be freed later. // - NET_LIST_FOR_EACH (Entry2, SpdSas) { - SadEntry = IPSEC_SAD_ENTRY_FROM_SPD (Entry2); - SadEntry->Data->SpdEntry = NULL; + NET_LIST_FOR_EACH_SAFE (Entry2, NextEntry2, SpdSas) { + SadEntry = IPSEC_SAD_ENTRY_FROM_SPD (Entry2); + + if (SadEntry->Data->SpdEntry != NULL) { + RemoveEntryList (&SadEntry->BySpd); + SadEntry->Data->SpdEntry = NULL; + } + + if (!(SadEntry->Data->ManualSet)) { + RemoveEntryList (&SadEntry->List); + FreePool (SadEntry); + } } // // Free the existed SPD entry // @@ -1192,24 +1206,34 @@ SetSpdEntry ( SadList = &mConfigData[IPsecConfigDataTypeSad]; NET_LIST_FOR_EACH (Entry, SadList) { SadEntry = IPSEC_SAD_ENTRY_FROM_LIST (Entry); - for (Index = 0; Index < SpdData->SaIdCount; Index++) { - - if (CompareSaId ( - (EFI_IPSEC_CONFIG_SELECTOR *) &SpdData->SaId[Index], - (EFI_IPSEC_CONFIG_SELECTOR *) SadEntry->Id - )) { - if (SadEntry->Data->SpdEntry != NULL) { - RemoveEntryList (&SadEntry->BySpd); + for (Index = 0; Index < SpdData->SaIdCount; Index++) { + if (CompareSaId ( + (EFI_IPSEC_CONFIG_SELECTOR *) &SpdData->SaId[Index], + (EFI_IPSEC_CONFIG_SELECTOR *) SadEntry->Id + )) { + // + // Check whether the found SadEntry is vaild. + // + if (IsSubSpdSelector ( + (EFI_IPSEC_CONFIG_SELECTOR *) SadEntry->Data->SpdSelector, + (EFI_IPSEC_CONFIG_SELECTOR *) SpdEntry->Selector + )) { + if (SadEntry->Data->SpdEntry != NULL) { + RemoveEntryList (&SadEntry->BySpd); + } + InsertTailList (&SpdEntry->Data->Sas, &SadEntry->BySpd); + SadEntry->Data->SpdEntry = SpdEntry; + } else { + return EFI_INVALID_PARAMETER; + } } - InsertTailList (&SpdEntry->Data->Sas, &SadEntry->BySpd); - SadEntry->Data->SpdEntry = SpdEntry; - } - } + } } + // // Insert the new SPD entry. // InsertTailList (EntryInsertBefore, &SpdEntry->List); -- 1.9.5.msysgit.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
