nr rlc: bugfix: fix NACK with range

We do some sanity checks for incoming rlc control packets. One of them
is to check that so_start is not > so_end when present.

But when 'range' is present and bigger than 1 it means that so_start refers
to one PDU and so_end to another one. So we may well have so_start > so_end.

This commit fixes that and reorganizes a bit the code to do the check before
processing and rejecting the PDU if the values are not correct. (Before this
commit we were NACKing the whole PDUs if so_start > so_end.)

openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c

@@ -650,7 +650,7 @@ control:
   control_decoder = decoder;
   control_e1 = e1;
   while (control_e1) {
-    nr_rlc_pdu_decoder_get_bits(&control_decoder, entity->sn_field_length); R(control_decoder); /* NACK_SN */
+    nack_sn = nr_rlc_pdu_decoder_get_bits(&control_decoder, entity->sn_field_length); R(control_decoder);
     control_e1 = nr_rlc_pdu_decoder_get_bits(&control_decoder, 1); R(control_decoder);
     control_e2 = nr_rlc_pdu_decoder_get_bits(&control_decoder, 1); R(control_decoder);
     control_e3 = nr_rlc_pdu_decoder_get_bits(&control_decoder, 1); R(control_decoder);
@@ -660,17 +660,36 @@ control:
     } else {
       nr_rlc_pdu_decoder_get_bits(&control_decoder, 1); R(control_decoder);
     }
+    /* check range and so_start/so_end consistency */
     if (control_e2) {
-      nr_rlc_pdu_decoder_get_bits(&control_decoder, 16); R(control_decoder); /* SOstart */
+      so_start = nr_rlc_pdu_decoder_get_bits(&control_decoder, 16); R(control_decoder);
-      nr_rlc_pdu_decoder_get_bits(&control_decoder, 16); R(control_decoder); /* SOend */
+      so_end = nr_rlc_pdu_decoder_get_bits(&control_decoder, 16); R(control_decoder);
+    } else {
+      so_start = 0;
+      so_end = 0xffff;
     }
     if (control_e3) {
-      nr_rlc_pdu_decoder_get_bits(&control_decoder, 8); R(control_decoder); /* NACK range */
+      range = nr_rlc_pdu_decoder_get_bits(&control_decoder, 8); R(control_decoder);
+    } else {
+      range = 1;
+    }
+    if (range < 1) {
+      LOG_E(RLC, "%s:%d:%s: error, bad 'range' in RLC NACK (sn %d)\n",
+            __FILE__, __LINE__, __FUNCTION__, nack_sn);
+      goto err;
+    }
+    /* so_start can be > so_end if more than one range; they don't refer
+     * to the same PDU then
+     */
+    if (range == 1 && so_end < so_start) {
+      LOG_E(RLC, "%s:%d:%s: error, bad so start/end (sn %d)\n",
+            __FILE__, __LINE__, __FUNCTION__, nack_sn);
+      goto err;
     }
   }
 
   /* 38.322 5.3.3.3 says to stop t_poll_retransmit if a ACK or NACK is
-   * received for the SN 'poll_sn'
+   * received for the SN 'poll_sn' - check ACK case (NACK done below)
    */
   if (sn_compare_tx(entity, entity->poll_sn, ack_sn) < 0)
     entity->t_poll_retransmit_start = 0;
@@ -697,28 +716,22 @@ control:
     if (e2) {
       so_start = nr_rlc_pdu_decoder_get_bits(&decoder, 16); R(decoder);
       so_end = nr_rlc_pdu_decoder_get_bits(&decoder, 16); R(decoder);
-      if (so_end < so_start) {
-        LOG_W(RLC, "%s:%d:%s: warning, bad so start/end, NACK the whole PDU (sn %d)\n",
-              __FILE__, __LINE__, __FUNCTION__, nack_sn);
-        so_start = 0;
-        so_end = -1;
-      }
-      /* special value 0xffff indicates 'all bytes to the end' */
-      if (so_end == 0xffff)
-        so_end = -1;
     } else {
       so_start = 0;
-      so_end = -1;
+      so_end = 0xffff;
     }
     if (e3) {
       range = nr_rlc_pdu_decoder_get_bits(&decoder, 8); R(decoder);
     } else {
       range = 1;
     }
+    /* special value 0xffff indicates 'all bytes to the end' */
+    if (so_end == 0xffff)
+      so_end = -1;
     process_received_nack(entity, nack_sn, so_start, so_end, range, sn_set);
 
     /* 38.322 5.3.3.3 says to stop t_poll_retransmit if a ACK or NACK is
-     * received for the SN 'poll_sn'
+     * received for the SN 'poll_sn' - check NACK case (ACK done above)
      */
     if (sn_compare_tx(entity, nack_sn, entity->poll_sn) <= 0 &&
         sn_compare_tx(entity, entity->poll_sn, (nack_sn + range) % entity->sn_modulus) < 0)

openair2/LAYER2/nr_rlc/tests/run_tests.sh

@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_count=14
+test_count=15
 
 for i in `seq $test_count`
 do

openair2/LAYER2/nr_rlc/tests/test15.h

@@ -0,0 +1,19 @@
+/*
+ * am test (SN field size 18):
+ * test "range" in NACK, generate a case where so_start > so_end
+ * (so so_start and so_end are not from the same PDU)
+ */
+
+TIME, 1,
+    GNB_AM, 100000, 100000, 45, 35, 0, -1, -1, 8, 18,
+    UE_AM, 100000, 100000, 45, 35, 0, -1, -1, 8, 18,
+    GNB_PDU_SIZE, 40,
+    UE_PDU_SIZE, 80,
+    GNB_SDU, 0, 50,
+    GNB_SDU, 1, 50,
+    GNB_SDU, 2, 50,
+TIME, 2,
+    UE_RECV_FAILS, 1,
+TIME, 4,
+    UE_RECV_FAILS, 0,
+TIME, -1