From 9d1c24d46172357eda675e201b05d9f0f77cb400 Mon Sep 17 00:00:00 2001 From: Cedric Roux Date: Fri, 26 Mar 2021 15:36:43 +0100 Subject: [PATCH] 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 | 45 +++++++++++++-------- openair2/LAYER2/nr_rlc/tests/run_tests.sh | 2 +- openair2/LAYER2/nr_rlc/tests/test15.h | 19 +++++++++ openair2/LAYER2/nr_rlc/tests/test15.txt.gz | Bin 0 -> 554 bytes 4 files changed, 49 insertions(+), 17 deletions(-) create mode 100644 openair2/LAYER2/nr_rlc/tests/test15.h create mode 100644 openair2/LAYER2/nr_rlc/tests/test15.txt.gz diff --git a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c index 1d4efc1e26..000c764e8f 100644 --- a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c +++ b/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 */ - nr_rlc_pdu_decoder_get_bits(&control_decoder, 16); R(control_decoder); /* SOend */ + so_start = nr_rlc_pdu_decoder_get_bits(&control_decoder, 16); R(control_decoder); + 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) diff --git a/openair2/LAYER2/nr_rlc/tests/run_tests.sh b/openair2/LAYER2/nr_rlc/tests/run_tests.sh index bf1b343d7e..a98f6b5d0f 100755 --- a/openair2/LAYER2/nr_rlc/tests/run_tests.sh +++ b/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 diff --git a/openair2/LAYER2/nr_rlc/tests/test15.h b/openair2/LAYER2/nr_rlc/tests/test15.h new file mode 100644 index 0000000000..8ce9f97896 --- /dev/null +++ b/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 diff --git a/openair2/LAYER2/nr_rlc/tests/test15.txt.gz b/openair2/LAYER2/nr_rlc/tests/test15.txt.gz new file mode 100644 index 0000000000000000000000000000000000000000..e615a59a5faa88571ac77198d4f53f979163309c GIT binary patch literal 554 zcmV+_0@eK=iwFo!vs_>T19W9`bTKt9ba-?C&6Z1R6hRb5_x_5r)Q!}A)N7VvaOWn; zN+k57S^`02CKJ(L@62RIGMdZ~!6eW$z35w-7qKF}m zL?j^@DM&>+q(=s1L>e-o0TonHLmiE1LNi*>igsv^4(NzBbix8ApAQMhzJMuam}3!3 zSjGxgu@3980UNP~O?bcsSKM&NBcAY#7rf#f-s1y4;tiix>l1~jL?b#e5)&~K3$YSA zVow~1BXI>Ykpf9bO0t6jx-XEN6iJDcNrhBN9jPY`q>RXs#1;W)cB{sm-e;IyX$uG zZCq~YeM7&h48O0Z3(_8+-peOt&DY)GTZixQ`LpBp^290JrZ^fS99zh_%)H}Tikew+0`pU3Cz^V;VD zK9M^{ZTqmAAh!))-&d1J_f748O^XeuJrtRj|Dlb3JOj@)={9@o5AE%X{s-8hA32Z1 sU%gm62EKO-7RzC1i^a6Oy85x(f;PUruRtDKpa%