From 1b2c9502e2bca38854fbaf098be8a535b22b2af2 Mon Sep 17 00:00:00 2001
From: Pedro Alvarez <pedro.alvarez@softwareradiosystems.com>
Date: Fri, 13 May 2022 16:57:39 +0100
Subject: [PATCH] lib,pdcp_nr: fix order in which header discard was done in
 PDCP NR.

---
 lib/src/pdcp/pdcp_entity_base.cc |  6 +++---
 lib/src/pdcp/pdcp_entity_nr.cc   | 20 ++++++++++++++------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/lib/src/pdcp/pdcp_entity_base.cc b/lib/src/pdcp/pdcp_entity_base.cc
index 729d1c3d9..647a50dbe 100644
--- a/lib/src/pdcp/pdcp_entity_base.cc
+++ b/lib/src/pdcp/pdcp_entity_base.cc
@@ -108,18 +108,18 @@ bool pdcp_entity_base::integrity_verify(uint8_t* msg, uint32_t msg_len, uint32_t
   if (sec_cfg.integ_algo != INTEGRITY_ALGORITHM_ID_EIA0) {
     for (uint8_t i = 0; i < 4; i++) {
       if (mac[i] != mac_exp[i]) {
-        logger.error(mac_exp, 4, "MAC mismatch (expected)");
-        logger.error(mac, 4, "MAC mismatch (found)");
         is_valid = false;
         break;
       }
     }
     srslog::log_channel& channel = is_valid ? logger.debug : logger.warning;
-    channel("Integrity check input: COUNT %" PRIu32 ", Bearer ID %d, Direction %s",
+    channel("Integrity check input - COUNT %" PRIu32 ", Bearer ID %d, Direction %s",
             count,
             cfg.bearer_id,
             cfg.rx_direction == SECURITY_DIRECTION_DOWNLINK ? "Downlink" : "Uplink");
     channel(k_int, 32, "Integrity check key:");
+    channel(mac_exp, 4, "MAC %s (expected):", is_valid ? "match" : "mismatch");
+    channel(mac, 4, "MAC %s (found):", is_valid ? "match" : "mismatch");
     channel(msg, msg_len, "Integrity check input msg (Bytes=%" PRIu32 "):", msg_len);
   }
 
diff --git a/lib/src/pdcp/pdcp_entity_nr.cc b/lib/src/pdcp/pdcp_entity_nr.cc
index 8d3a94467..a7707dc2a 100644
--- a/lib/src/pdcp/pdcp_entity_nr.cc
+++ b/lib/src/pdcp/pdcp_entity_nr.cc
@@ -180,11 +180,11 @@ void pdcp_entity_nr::write_pdu(unique_byte_buffer_t pdu)
 
   // Extract RCVD_SN from header
   uint32_t rcvd_sn = read_data_header(pdu);
-  discard_data_header(pdu); // TODO: Check wheather the header is part of integrity check.
 
-  // Extract MAC
+  // Extract MAC-I
+  // Allways extract from SRBs, only extract from DRBs if integrity is enabled
   uint8_t mac[4] = {};
-  if (is_drb() && (integrity_direction == DIRECTION_TX || integrity_direction == DIRECTION_TXRX)) {
+  if (is_srb() || (is_drb() && (integrity_direction == DIRECTION_TX || integrity_direction == DIRECTION_TXRX))) {
     extract_mac(pdu, mac);
   }
 
@@ -201,11 +201,16 @@ void pdcp_entity_nr::write_pdu(unique_byte_buffer_t pdu)
 
   logger.debug("RCVD_HFN=%u, RCVD_SN=%u, RCVD_COUNT=%u", rcvd_hfn, rcvd_sn, rcvd_count);
 
-  // Decripting
+  // TS 38.323, section 5.8: Deciphering
+  // The data unit that is ciphered is the MAC-I and the
+  // data part of the PDCP Data PDU except the
+  // SDAP header and the SDAP Control PDU if included in the PDCP SDU.
   cipher_decrypt(pdu->msg, pdu->N_bytes, rcvd_count, pdu->msg);
 
-  // Integrity check
-  if (is_drb() && (integrity_direction == DIRECTION_TX || integrity_direction == DIRECTION_TXRX)) {
+  // TS 38.323, section 5.9: Integrity verification
+  // The data unit that is integrity protected is the PDU header
+  // and the data part of the PDU before ciphering.
+  if (integrity_direction == DIRECTION_TX || integrity_direction == DIRECTION_TXRX) {
     bool is_valid = integrity_verify(pdu->msg, pdu->N_bytes, rcvd_count, mac);
     if (!is_valid) {
       logger.error(pdu->msg, pdu->N_bytes, "%s Dropping PDU", rb_name.c_str());
@@ -216,6 +221,9 @@ void pdcp_entity_nr::write_pdu(unique_byte_buffer_t pdu)
     }
   }
 
+  // After checking the integrity, we can discard the header.
+  discard_data_header(pdu);
+
   // Check valid rcvd_count
   if (rcvd_count < rx_deliv) {
     logger.debug("Out-of-order after time-out, duplicate or COUNT wrap-around");