From b9337dc5787aadf71eee9d676d6b7f1181ac0787 Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Wed, 23 Feb 2022 00:56:00 +0800 Subject: [PATCH] [vslib]: Fix MACsec bug in SCI and XPN (#1003) --- tests/aspell.en.pws | 2 ++ unittest/vslib/Makefile.am | 3 +- unittest/vslib/TestMACsecManager.cpp | 29 ++++++++++++++++++++ unittest/vslib/TestSwitchStateBaseMACsec.cpp | 2 ++ vslib/MACsecManager.cpp | 5 ++++ vslib/SwitchStateBaseMACsec.cpp | 9 ++++-- 6 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 unittest/vslib/TestMACsecManager.cpp diff --git a/tests/aspell.en.pws b/tests/aspell.en.pws index 51b892f5043c..05c891b08330 100644 --- a/tests/aspell.en.pws +++ b/tests/aspell.en.pws @@ -370,6 +370,8 @@ splitted src SRC ss +ssci +SSCI stateful stdint stdlib diff --git a/unittest/vslib/Makefile.am b/unittest/vslib/Makefile.am index 42e4039a1ba0..ce3b19719e23 100644 --- a/unittest/vslib/Makefile.am +++ b/unittest/vslib/Makefile.am @@ -41,7 +41,8 @@ tests_SOURCES = main.cpp \ TestSwitchMLNX2700.cpp \ TestSwitchBCM56850.cpp \ TestSwitchBCM81724.cpp \ - TestSwitchStateBaseMACsec.cpp + TestSwitchStateBaseMACsec.cpp \ + TestMACsecManager.cpp tests_CXXFLAGS = $(DBGFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS_COMMON) -fno-access-control tests_LDADD = $(LDADD_GTEST) $(top_srcdir)/vslib/libSaiVS.a -lhiredis -lswsscommon -lnl-genl-3 -lnl-nf-3 -lnl-route-3 -lnl-3 \ diff --git a/unittest/vslib/TestMACsecManager.cpp b/unittest/vslib/TestMACsecManager.cpp new file mode 100644 index 000000000000..b1d8264b1646 --- /dev/null +++ b/unittest/vslib/TestMACsecManager.cpp @@ -0,0 +1,29 @@ +#include "MACsecAttr.h" +#include "MACsecManager.h" + +#include + +#include + +using namespace saivs; + +TEST(MACsecManager, create_macsec_ingress_sa) +{ + // This is a system call that may not be valid in the test environment, + // So, this case is just for the testing coverage checking. + + MACsecManager manager; + + MACsecAttr attr; + attr.m_vethName = "eth0"; + attr.m_macsecName = "macsec_eth0"; + attr.m_sci = "02:42:ac:11:00:03"; + attr.m_an = 0; + attr.m_pn = 1; + attr.m_cipher = MACsecAttr::CIPHER_NAME_GCM_AES_XPN_128; + attr.m_ssci = 0x1; + attr.m_salt = ""; + attr.m_authKey = ""; + attr.m_sak = ""; + manager.create_macsec_ingress_sa(attr); +} diff --git a/unittest/vslib/TestSwitchStateBaseMACsec.cpp b/unittest/vslib/TestSwitchStateBaseMACsec.cpp index 597a950f18ee..623119cde0c2 100644 --- a/unittest/vslib/TestSwitchStateBaseMACsec.cpp +++ b/unittest/vslib/TestSwitchStateBaseMACsec.cpp @@ -30,6 +30,8 @@ TEST(SwitchStateBase, loadMACsecAttrFromMACsecSA) attr.id = SAI_MACSEC_SC_ATTR_MACSEC_CIPHER_SUITE; attr.value.s32 = sai_macsec_cipher_suite_t::SAI_MACSEC_CIPHER_SUITE_GCM_AES_128; attrs.push_back(attr); + attr.id = SAI_MACSEC_SC_ATTR_MACSEC_EXPLICIT_SCI_ENABLE; + attrs.push_back(attr); EXPECT_EQ( SAI_STATUS_SUCCESS, ss.create_internal( diff --git a/vslib/MACsecManager.cpp b/vslib/MACsecManager.cpp index 2fdd56e6b5a2..0d3809c10161 100644 --- a/vslib/MACsecManager.cpp +++ b/vslib/MACsecManager.cpp @@ -362,6 +362,7 @@ bool MACsecManager::create_macsec_egress_sc( << " sci " << attr.m_sci << " encrypt " << (attr.m_encryptionEnable ? " on " : " off ") << " cipher " << attr.m_cipher + << " send_sci " << (attr.m_sendSci ? " on " : " off ") << " && ip link set dev " << shellquote(attr.m_macsecName) << " up"; @@ -451,6 +452,10 @@ bool MACsecManager::create_macsec_ingress_sa( << attr.m_an << " pn " << attr.m_pn + << ( attr.is_xpn() ? " ssci " : "" ) + << ( attr.is_xpn() ? std::to_string(attr.m_ssci) : "" ) + << ( attr.is_xpn() ? " salt " : "" ) + << ( attr.is_xpn() ? attr.m_salt : "" ) << " on key " << attr.m_authKey << " " diff --git a/vslib/SwitchStateBaseMACsec.cpp b/vslib/SwitchStateBaseMACsec.cpp index 1f248996dddf..e34b5dd9b60c 100644 --- a/vslib/SwitchStateBaseMACsec.cpp +++ b/vslib/SwitchStateBaseMACsec.cpp @@ -11,6 +11,7 @@ #include #include +#include #include using namespace saivs; @@ -589,11 +590,12 @@ sai_status_t SwitchStateBase::loadMACsecAttrFromMACsecSA( SAI_METADATA_GET_ATTR_BY_ID(attr, SAI_MACSEC_SA_ATTR_SC_ID, attrCount, attrList); // Find MACsec SC attributes - std::vector attrs(4); + std::vector attrs(5); attrs[0].id = SAI_MACSEC_SC_ATTR_FLOW_ID; attrs[1].id = SAI_MACSEC_SC_ATTR_MACSEC_SCI; attrs[2].id = SAI_MACSEC_SC_ATTR_ENCRYPTION_ENABLE; attrs[3].id = SAI_MACSEC_SC_ATTR_MACSEC_CIPHER_SUITE; + attrs[4].id = SAI_MACSEC_SC_ATTR_MACSEC_EXPLICIT_SCI_ENABLE; CHECK_STATUS(get(SAI_OBJECT_TYPE_MACSEC_SC, attr->value.oid, static_cast(attrs.size()), attrs.data())); @@ -609,6 +611,7 @@ sai_status_t SwitchStateBase::loadMACsecAttrFromMACsecSA( std::stringstream sciHexStr; macsecAttr.m_encryptionEnable = attrs[2].value.booldata; bool is_sak_128_bit = (attrs[3].value.s32 == SAI_MACSEC_CIPHER_SUITE_GCM_AES_128 || attrs[3].value.s32 == SAI_MACSEC_CIPHER_SUITE_GCM_AES_XPN_128); + macsecAttr.m_sendSci = attrs[4].value.booldata; sciHexStr << std::setw(MACSEC_SCI_LENGTH) << std::setfill('0'); @@ -679,7 +682,9 @@ sai_status_t SwitchStateBase::loadMACsecAttrFromMACsecSA( { SAI_METADATA_GET_ATTR_BY_ID(attr, SAI_MACSEC_SA_ATTR_MACSEC_SSCI, attrCount, attrList); - macsecAttr.m_ssci = attr->value.u32; + // The Linux kernel directly uses ssci to XOR with the salt that is network order, + // So, this conversion is useful to convert SSCI from the host order to network order. + macsecAttr.m_ssci = htonl(attr->value.u32); SAI_METADATA_GET_ATTR_BY_ID(attr, SAI_MACSEC_SA_ATTR_SALT, attrCount, attrList);