Skip to content

Commit dc15744

Browse files
0xc01700xc0170
authored
Merge pull request #9601 from hasnainvirk/mem_leak_link_adr
LoRaWAN: Proper size checks for link ADR cmds & correct include path in Unittests framework
2 parents a7b9490 + a14acfa commit dc15744

File tree

12 files changed

+85
-17
lines changed

12 files changed

+85
-17
lines changed

‎UNITTESTS/CMakeLists.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ target_include_directories(gmock_main SYSTEM BEFORE INTERFACE
5959
# TESTING
6060
####################
6161

62-
enable_testing()
62+
include(CTest)
6363

6464
set_property(DIRECTORY APPEND PROPERTY ADDITIONAL_MAKE_CLEAN_FILES
6565
"${CMAKE_BINARY_DIR}/Testing"

‎UNITTESTS/features/lorawan/loraphyau915/Test_LoRaPHYAU915.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,10 @@ TEST_F(Test_LoRaPHYAU915, link_ADR_request)
181181
uint8_t nb_rep_out = 0;
182182
uint8_t nb_bytes_parsed = 0;
183183

184+
uint8_t payload [] = {SRV_MAC_LINK_ADR_REQ, 1, 2, 3, 4};
185+
params.payload = payload;
186+
params.payload_size = 5;
187+
184188
LoRaPHY_stub::uint8_value = 1;
185189
LoRaPHY_stub::ch_mask_value = 6;
186190
LoRaPHY_stub::adr_parse_count = 2;

‎UNITTESTS/features/lorawan/loraphycn470/Test_LoRaPHYCN470.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,10 @@ TEST_F(Test_LoRaPHYCN470, link_ADR_request)
206206
uint8_t nb_rep_out = 0;
207207
uint8_t nb_bytes_parsed = 0;
208208

209+
uint8_t payload [] = {SRV_MAC_LINK_ADR_REQ, 1, 2, 3, 4};
210+
params.payload = payload;
211+
params.payload_size = 5;
212+
209213
LoRaPHY_stub::uint8_value = 1;
210214
LoRaPHY_stub::ch_mask_value = 6;
211215
LoRaPHY_stub::adr_parse_count = 2;

‎UNITTESTS/features/lorawan/loraphyus915/Test_LoRaPHYUS915.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,15 +177,22 @@ TEST_F(Test_LoRaPHYUS915, tx_config)
177177

178178
TEST_F(Test_LoRaPHYUS915, link_ADR_request)
179179
{
180+
uint8_t payload [] = {SRV_MAC_LINK_ADR_REQ, 1, 2, 3, 4};
180181
adr_req_params_t params;
181182
memset(&params, 0, sizeof(params));
182183
int8_t dr_out = 0;
183184
int8_t tx_power_out = 0;
184185
uint8_t nb_rep_out = 0;
185186
uint8_t nb_bytes_parsed = 0;
186187

187-
EXPECT_TRUE(0 == object->link_ADR_request(&params, &dr_out, &tx_power_out, &nb_rep_out, &nb_bytes_parsed));
188+
params.payload = payload;
189+
params.payload_size = 4;
188190

191+
uint8_t status = object->link_ADR_request(&params, &dr_out, &tx_power_out, &nb_rep_out, &nb_bytes_parsed);
192+
193+
EXPECT_TRUE(0 == nb_bytes_parsed);
194+
195+
params.payload_size = 5;
189196
LoRaPHY_stub::uint8_value = 1;
190197
LoRaPHY_stub::ch_mask_value = 6;
191198
LoRaPHY_stub::adr_parse_count = 2;

‎UNITTESTS/features/lorawan/lorawanstack/Test_LoRaWANStack.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -494,6 +494,10 @@ TEST_F(Test_LoRaWANStack, handle_rx)
494494
ind.buffer = ind_buf;
495495
ind.buffer_size = 150;
496496
ind.type = MCPS_UNCONFIRMED;
497+
ind.port = 15;
498+
ind.is_data_recvd = true;
499+
ind.fpending_status = false;
500+
LoRaMac_stub::dev_class_value = CLASS_A;
497501
radio._ev->rx_done(NULL, 0, 0, 0);
498502

499503
//data == NULL || LENGTH == 0 (2 cases)

‎UNITTESTS/stubs/LoRaPHY_stub.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,9 +151,12 @@ lorawan_time_t LoRaPHY::update_band_timeoff(bool joined, bool duty_cycle,
151151
}
152152

153153
uint8_t LoRaPHY::parse_link_ADR_req(const uint8_t *payload,
154+
uint8_t payload_size,
154155
link_adr_params_t *params)
155156
{
156157
params->ch_mask_ctrl = LoRaPHY_stub::ch_mask_value;
158+
params->channel_mask = 0;
159+
params->datarate = 0;
157160

158161
if (LoRaPHY_stub::adr_parse_count) {
159162
return --LoRaPHY_stub::adr_parse_count;

‎features/lorawan/lorastack/mac/LoRaMacCommand.cpp

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(const uint8_t *payload, ui
149149
int8_t link_adr_dr = DR_0;
150150
int8_t link_adr_txpower = TX_POWER_0;
151151
uint8_t link_adr_nbtrans = 0;
152-
uint8_t link_adr_nb_bytes_pasred = 0;
152+
uint8_t link_adr_nb_bytes_parsed = 0;
153153

154154
// Fill parameter structure
155155
link_adr_req.payload = &payload[mac_index - 1];
@@ -165,7 +165,14 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(const uint8_t *payload, ui
165165
&link_adr_dr,
166166
&link_adr_txpower,
167167
&link_adr_nbtrans,
168-
&link_adr_nb_bytes_pasred);
168+
&link_adr_nb_bytes_parsed);
169+
170+
// If nothing was consumed, we have a malformed packet at our hand
171+
// we bin everything and return. link_adr_nb_bytes_parsed being 0 is
172+
// a magic identifier letting us know that there are payload inconsistencies
173+
if (link_adr_nb_bytes_parsed == 0) {
174+
return LORAWAN_STATUS_UNSUPPORTED;
175+
}
169176

170177
if ((status & 0x07) == 0x07) {
171178
mac_sys_params.channel_data_rate = link_adr_dr;
@@ -174,11 +181,11 @@ lorawan_status_t LoRaMacCommand::process_mac_commands(const uint8_t *payload, ui
174181
}
175182

176183
// Add the answers to the buffer
177-
for (uint8_t i = 0; i < (link_adr_nb_bytes_pasred / 5); i++) {
184+
for (uint8_t i = 0; i < (link_adr_nb_bytes_parsed / 5); i++) {
178185
ret_value = add_link_adr_ans(status);
179186
}
180187
// Update MAC index
181-
mac_index += link_adr_nb_bytes_pasred - 1;
188+
mac_index += link_adr_nb_bytes_parsed - 1;
182189
}
183190
break;
184191
case SRV_MAC_DUTY_CYCLE_REQ:

‎features/lorawan/lorastack/phy/LoRaPHY.cpp

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -309,11 +309,12 @@ lorawan_time_t LoRaPHY::update_band_timeoff(bool joined, bool duty_cycle,
309309
}
310310

311311
uint8_t LoRaPHY::parse_link_ADR_req(const uint8_t *payload,
312+
uint8_t payload_size,
312313
link_adr_params_t *params)
313314
{
314315
uint8_t ret_index = 0;
315316

316-
if (payload[0] == SRV_MAC_LINK_ADR_REQ) {
317+
if (payload_size >= 5) {
317318

318319
// Parse datarate and tx power
319320
params->datarate = payload[1];
@@ -973,13 +974,17 @@ uint8_t LoRaPHY::link_ADR_request(adr_req_params_t *link_adr_req,
973974

974975
verify_adr_params_t verify_params;
975976

976-
while (bytes_processed < link_adr_req->payload_size) {
977+
while (bytes_processed < link_adr_req->payload_size &&
978+
link_adr_req->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) {
977979
// Get ADR request parameters
978980
next_index = parse_link_ADR_req(&(link_adr_req->payload[bytes_processed]),
981+
link_adr_req->payload_size - bytes_processed,
979982
&adr_settings);
980983

981984
if (next_index == 0) {
982-
break; // break loop, since no more request has been found
985+
bytes_processed = 0;
986+
// break loop, malformed packet
987+
break;
983988
}
984989

985990
// Update bytes processed
@@ -1024,6 +1029,11 @@ uint8_t LoRaPHY::link_ADR_request(adr_req_params_t *link_adr_req,
10241029
}
10251030
}
10261031

1032+
if (bytes_processed == 0) {
1033+
*nb_bytes_processed = 0;
1034+
return status;
1035+
}
1036+
10271037
if (is_datarate_supported(adr_settings.datarate)) {
10281038
verify_params.status = status;
10291039

‎features/lorawan/lorastack/phy/LoRaPHY.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -608,7 +608,8 @@ class LoRaPHY : private mbed::NonCopyable<LoRaPHY> {
608608
/**
609609
* Parses the parameter of an LinkAdrRequest.
610610
*/
611-
uint8_t parse_link_ADR_req(const uint8_t *payload, link_adr_params_t *adr_params);
611+
uint8_t parse_link_ADR_req(const uint8_t *payload, uint8_t payload_size,
612+
link_adr_params_t *adr_params);
612613

613614
/**
614615
* Verifies and updates the datarate, the TX power and the number of repetitions

‎features/lorawan/lorastack/phy/LoRaPHYAU915.cpp

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -435,12 +435,16 @@ uint8_t LoRaPHYAU915::link_ADR_request(adr_req_params_t *params,
435435
// Initialize local copy of channels mask
436436
copy_channel_mask(temp_channel_masks, channel_mask, AU915_CHANNEL_MASK_SIZE);
437437

438-
while (bytes_processed < params->payload_size) {
438+
while (bytes_processed < params->payload_size &&
439+
params->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) {
439440
next_index = parse_link_ADR_req(&(params->payload[bytes_processed]),
441+
params->payload_size,
440442
&adr_settings);
441443

442444
if (next_index == 0) {
443-
break; // break loop, since no more request has been found
445+
bytes_processed = 0;
446+
// break loop, malformed packet
447+
break;
444448
}
445449

446450
// Update bytes processed
@@ -471,6 +475,11 @@ uint8_t LoRaPHYAU915::link_ADR_request(adr_req_params_t *params,
471475
}
472476
}
473477

478+
if (bytes_processed == 0) {
479+
*nb_bytes_parsed = 0;
480+
return status;
481+
}
482+
474483
// FCC 15.247 paragraph F mandates to hop on at least 2 125 kHz channels
475484
if ((adr_settings.datarate < DR_6)
476485
&& (num_active_channels(temp_channel_masks, 0, 4) < 2)) {

‎features/lorawan/lorastack/phy/LoRaPHYCN470.cpp

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -460,13 +460,18 @@ uint8_t LoRaPHYCN470::link_ADR_request(adr_req_params_t *params,
460460
// Initialize local copy of channels mask
461461
copy_channel_mask(temp_channel_masks, channel_mask, CN470_CHANNEL_MASK_SIZE);
462462

463-
while (bytes_processed < params->payload_size) {
463+
while (bytes_processed < params->payload_size &&
464+
params->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) {
464465

465466
// Get ADR request parameters
466-
next_index = parse_link_ADR_req(&(params->payload[bytes_processed]), &adr_settings);
467+
next_index = parse_link_ADR_req(&(params->payload[bytes_processed]),
468+
params->payload_size,
469+
&adr_settings);
467470

468471
if (next_index == 0) {
469-
break; // break loop, since no more request has been found
472+
bytes_processed = 0;
473+
// break loop, malformed packet
474+
break;
470475
}
471476

472477
// Update bytes processed
@@ -501,6 +506,11 @@ uint8_t LoRaPHYCN470::link_ADR_request(adr_req_params_t *params,
501506
}
502507
}
503508

509+
if (bytes_processed == 0) {
510+
*nb_bytes_parsed = 0;
511+
return status;
512+
}
513+
504514
verify_params.status = status;
505515
verify_params.adr_enabled = params->adr_enabled;
506516
verify_params.datarate = adr_settings.datarate;

‎features/lorawan/lorastack/phy/LoRaPHYUS915.cpp

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -460,12 +460,16 @@ uint8_t LoRaPHYUS915::link_ADR_request(adr_req_params_t *params,
460460
// Initialize local copy of channels mask
461461
copy_channel_mask(temp_channel_masks, channel_mask, US915_CHANNEL_MASK_SIZE);
462462

463-
while (bytes_processed < params->payload_size) {
463+
while (bytes_processed < params->payload_size &&
464+
params->payload[bytes_processed] == SRV_MAC_LINK_ADR_REQ) {
464465
next_idx = parse_link_ADR_req(&(params->payload[bytes_processed]),
466+
params->payload_size - bytes_processed,
465467
&adr_settings);
466468

467469
if (next_idx == 0) {
468-
break; // break loop, since no more request has been found
470+
bytes_processed = 0;
471+
// break loop, malformed packet
472+
break;
469473
}
470474

471475
// Update bytes processed
@@ -501,6 +505,11 @@ uint8_t LoRaPHYUS915::link_ADR_request(adr_req_params_t *params,
501505
}
502506
}
503507

508+
if (bytes_processed == 0) {
509+
*nb_bytes_parsed = 0;
510+
return status;
511+
}
512+
504513
// FCC 15.247 paragraph F mandates to hop on at least 2 125 kHz channels
505514
if ((adr_settings.datarate < DR_4) &&
506515
(num_active_channels(temp_channel_masks, 0, 4) < 2)) {

0 commit comments

Comments
 (0)