[vslib]: Fix MACsec bug in SCI and XPN (#1003)

sonic-net · Feb 22, 2022 · b9337dc · b9337dc
1 parent edbceb9
commit b9337dc
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 3 deletions.
diff --git 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
@@ -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
@@ -0,0 +1,29 @@
+#include "MACsecAttr.h"
+#include "MACsecManager.h"
+
+#include <gtest/gtest.h>
+
+#include <vector>
+
+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
@@ -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
@@ -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
@@ -11,6 +11,7 @@
 #include <regex>
 
 #include <net/if.h>
+#include <arpa/inet.h>
 #include <byteswap.h>
 
 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<sai_attribute_t> attrs(4);
+    std::vector<sai_attribute_t> 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<uint32_t>(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);