From fcf481b83e32ee6c18d7938f1c0bacdc106bd07d Mon Sep 17 00:00:00 2001
From: Andre Puschmann <andre@softwareradiosystems.com>
Date: Mon, 11 Jan 2021 10:39:09 +0100
Subject: [PATCH] pdu: add check when unpacking MAC PDUs to not read beyond PDU
 length

we've checked the same when unpacking the subheaders but missed the
case where the payload was read beyond the PDU length, as has been
seen with a malformed RAR PDU.
---
 lib/include/srslte/mac/pdu.h | 24 ++++++++++++++++++++----
 lib/src/mac/pdu.cc           | 11 ++++++-----
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/lib/include/srslte/mac/pdu.h b/lib/include/srslte/mac/pdu.h
index afcd94b56..e4d4d6800 100644
--- a/lib/include/srslte/mac/pdu.h
+++ b/lib/include/srslte/mac/pdu.h
@@ -218,7 +218,7 @@ public:
   void add_sdu(uint32_t sdu_sz) { buffer_tx->N_bytes += sdu_sz; }
 
   // Section 6.1.2
-  void parse_packet(uint8_t* ptr)
+  uint32_t parse_packet(uint8_t* ptr)
   {
     uint8_t* init_ptr = ptr;
     nof_subheaders    = 0;
@@ -230,19 +230,35 @@ public:
       }
 
       if (ret && ((ptr - init_ptr) >= (int32_t)pdu_len)) {
-        // stop processing last subheader indicates another one but all bytes are consume
+        // stop processing last subheader indicates another one but all bytes are consumed
         nof_subheaders = 0;
-        INFO("Corrupted MAC PDU - all bytes have been consumed (pdu_len=%d)\n", pdu_len);
         if (log_h) {
-          log_h->info_hex(
+          log_h->warning_hex(
               init_ptr, pdu_len, "Corrupted MAC PDU - all bytes have been consumed (pdu_len=%d)\n", pdu_len);
+        } else {
+          srslte::console("Corrupted MAC PDU - all bytes have been consumed (pdu_len=%d)\n", pdu_len);
         }
+        return SRSLTE_ERROR;
       }
     } while (ret && (nof_subheaders + 1) < (int)max_subheaders && ((int32_t)pdu_len > (ptr - init_ptr)));
 
     for (int i = 0; i < nof_subheaders; i++) {
       subheaders[i].read_payload(&ptr);
+
+      // stop processing if we read payload beyond the PDU length
+      if ((ptr - init_ptr) > (int32_t)pdu_len) {
+        nof_subheaders = 0;
+
+        if (log_h) {
+          log_h->warning_hex(
+              init_ptr, pdu_len, "Corrupted MAC PDU - all bytes have been consumed (pdu_len=%d)\n", pdu_len);
+        } else {
+          srslte::console("Corrupted MAC PDU - all bytes have been consumed (pdu_len=%d)\n", pdu_len);
+        }
+        return SRSLTE_ERROR;
+      }
     }
+    return SRSLTE_SUCCESS;
   }
 
 protected:
diff --git a/lib/src/mac/pdu.cc b/lib/src/mac/pdu.cc
index 43d3fc3f5..df17a5b68 100644
--- a/lib/src/mac/pdu.cc
+++ b/lib/src/mac/pdu.cc
@@ -218,9 +218,10 @@ void sch_pdu::parse_packet(uint8_t* ptr)
     if (n_sub >= 0) {
       subheaders[nof_subheaders - 1].set_payload_size(n_sub);
     } else {
-      ERROR("Corrupted MAC PDU (read_len=%d, pdu_len=%d)\n", read_len, pdu_len);
       if (log_h) {
-        log_h->info_hex(ptr, pdu_len, "Corrupted MAC PDU (read_len=%d, pdu_len=%d)\n", read_len, pdu_len);
+        log_h->warning_hex(ptr, pdu_len, "Corrupted MAC PDU (read_len=%d, pdu_len=%d)\n", read_len, pdu_len);
+      } else {
+        srslte::console("Corrupted MAC PDU (read_len=%d, pdu_len=%d)\n", read_len, pdu_len);
       }
 
       // reset PDU
@@ -1017,7 +1018,7 @@ uint8_t sch_subh::phr_report_table(float phr_value)
 
 std::string rar_pdu::to_string()
 {
-  std::string msg("MAC PDU for RAR. ");
+  std::string msg("MAC PDU for RAR: ");
   msg += pdu::to_string();
   return msg;
 }
@@ -1072,7 +1073,7 @@ bool rar_pdu::write_packet(uint8_t* ptr)
   int32_t pad_len     = rem_len - payload_len;
   if (pad_len < 0) {
     if (log_h) {
-      log_h->error("Error packing RAR PDU (payload_len=%d, rem_len=%d)\n", payload_len, rem_len);
+      log_h->warning("Error packing RAR PDU (payload_len=%d, rem_len=%d)\n", payload_len, rem_len);
     } else {
       srslte::console("Error packing RAR PDU (payload_len=%d, rem_len=%d)\n", payload_len, rem_len);
     }
@@ -1095,7 +1096,7 @@ std::string rar_subh::to_string()
 
   char tmp[16];
   srslte_vec_sprint_hex(tmp, sizeof(tmp), grant, RAR_GRANT_LEN);
-  ss << tmp << "\n";
+  ss << tmp;
 
   return ss.str();
 }