Skip to content

Commit dca1974

Browse files
NickeZNickeZ
authored
Merge pull request #1493 from NickeZ/nickez/review-fixes
review: Various review fixes
2 parents 4786e19 + e4c2907 commit dca1974

File tree

7 files changed

+56
-21
lines changed

7 files changed

+56
-21
lines changed

‎src/bootloader/bootloader.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ void bootloader_render_default_screen(void)
359359

360360
#if PLATFORM_BITBOX02PLUS
361361
extern bool bootloader_pairing_request;
362-
extern uint8_t bootloader_pairing_code_bytes[16];
362+
extern uint8_t bootloader_pairing_code_bytes[4];
363363

364364
void bootloader_render_ble_confirm_screen(bool confirmed)
365365
{

‎src/bootloader/startup.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ uint32_t __stack_chk_guard = 0;
6161
#if PLATFORM_BITBOX02PLUS == 1
6262
extern volatile bool measurement_done_touch;
6363
int bootloader_pairing_request = false;
64-
uint8_t bootloader_pairing_code_bytes[16] = {0};
64+
uint8_t bootloader_pairing_code_bytes[4] = {0};
6565
// Must be power of 2, must fit bond_db
6666
#define UART_OUT_BUF_LEN 2048
6767
struct ringbuffer uart_write_queue;

‎src/da14531/da14531.c

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ void da14531_reset(struct ringbuffer* uart_out)
2323
{
2424
util_log("da14531_reset");
2525
uint8_t payload = CTRL_CMD_BLE_CHIP_RESET;
26-
uint8_t buf[10] = {0};
26+
uint8_t buf[12 + sizeof(payload) * 2] = {0};
2727
uint16_t len = da14531_protocol_format(
2828
&buf[0], sizeof(buf), DA14531_PROTOCOL_PACKET_TYPE_CTRL_DATA, &payload, 1);
2929
ASSERT(len <= sizeof(buf));
@@ -37,7 +37,7 @@ void da14531_power_down(struct ringbuffer* uart_out)
3737
{
3838
util_log("da14531_power_down");
3939
uint8_t payload[2] = {CTRL_CMD_BLE_POWER_DOWN, 0};
40-
uint8_t buf[10] = {0};
40+
uint8_t buf[12 + sizeof(payload) * 2] = {0};
4141
uint16_t len = da14531_protocol_format(
4242
&buf[0], sizeof(buf), DA14531_PROTOCOL_PACKET_TYPE_CTRL_DATA, &payload[0], sizeof(payload));
4343
ASSERT(len <= sizeof(buf));
@@ -57,7 +57,7 @@ void da14531_set_product(
5757
for (int i = 0; i < product_len; i++) {
5858
payload[1 + i] = product[i];
5959
}
60-
uint8_t tmp[128];
60+
uint8_t tmp[12 + sizeof(payload) * 2];
6161
uint16_t tmp_len = da14531_protocol_format(
6262
&tmp[0], sizeof(tmp), DA14531_PROTOCOL_PACKET_TYPE_CTRL_DATA, &payload[0], 1 + product_len);
6363
ASSERT(tmp_len <= sizeof(tmp));
@@ -72,7 +72,7 @@ void da14531_set_name(const char* name, size_t name_len, struct ringbuffer* uart
7272
uint8_t payload[64] = {0};
7373
payload[0] = CTRL_CMD_DEVICE_NAME;
7474
memcpy(&payload[1], name, name_len);
75-
uint8_t tmp[128];
75+
uint8_t tmp[12 + sizeof(payload) * 2];
7676
uint16_t tmp_len = da14531_protocol_format(
7777
&tmp[0], sizeof(tmp), DA14531_PROTOCOL_PACKET_TYPE_CTRL_DATA, &payload[0], 1 + name_len);
7878
ASSERT(tmp_len <= sizeof(tmp));
@@ -85,7 +85,7 @@ void da14531_set_name(const char* name, size_t name_len, struct ringbuffer* uart
8585
void da14531_get_connection_state(struct ringbuffer* uart_out)
8686
{
8787
uint8_t payload = CTRL_CMD_BLE_STATUS;
88-
uint8_t tmp[16];
88+
uint8_t tmp[12 + sizeof(payload) * 2];
8989
uint16_t tmp_len = da14531_protocol_format(
9090
&tmp[0], sizeof(tmp), DA14531_PROTOCOL_PACKET_TYPE_CTRL_DATA, &payload, 1);
9191
ASSERT(tmp_len <= sizeof(tmp));

‎src/da14531/da14531_handler.c

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ static void _ble_pairing_callback(bool ok, void* param)
5757
memcpy(&payload[1], &data->key[0], sizeof(data->key));
5858
payload[17] = ok ? 1 : 0; /* 1 yes, 0 no */
5959

60-
uint8_t tmp[32];
60+
uint8_t tmp[12 + sizeof(payload) * 2];
6161
uint16_t len = da14531_protocol_format(
6262
&tmp[0], sizeof(tmp), DA14531_PROTOCOL_PACKET_TYPE_CTRL_DATA, payload, sizeof(payload));
6363
ASSERT(len <= sizeof(tmp));
@@ -72,12 +72,11 @@ static void _ble_pairing_callback(bool ok, void* param)
7272
#else
7373
#include <bootloader/bootloader.h>
7474
extern bool bootloader_pairing_request;
75-
extern uint8_t bootloader_pairing_code_bytes[16];
75+
extern uint8_t bootloader_pairing_code_bytes[4];
7676
#endif
7777

7878
static void _ctrl_handler(struct da14531_ctrl_frame* frame, struct ringbuffer* queue)
7979
{
80-
uint8_t tmp[1152]; // 1024 + some margin (128)
8180
switch (frame->cmd) {
8281
case CTRL_CMD_DEVICE_NAME: {
8382
// util_log("da14531: get device name");
@@ -90,6 +89,7 @@ static void _ctrl_handler(struct da14531_ctrl_frame* frame, struct ringbuffer* q
9089
#else
9190
memory_get_device_name((char*)&response[1]);
9291
#endif
92+
uint8_t tmp[12 + sizeof(response) * 2];
9393
uint16_t len = da14531_protocol_format(
9494
&tmp[0],
9595
sizeof(tmp),
@@ -109,6 +109,7 @@ static void _ctrl_handler(struct da14531_ctrl_frame* frame, struct ringbuffer* q
109109
int16_t len = memory_get_ble_bond_db(&response[1]);
110110
// util_log("da14531: bond db len %d", len);
111111
uint16_t tmp_len;
112+
uint8_t tmp[12 + sizeof(response) * 2];
112113
if (len != -1) {
113114
tmp_len = da14531_protocol_format(
114115
&tmp[0],
@@ -128,13 +129,18 @@ static void _ctrl_handler(struct da14531_ctrl_frame* frame, struct ringbuffer* q
128129
} break;
129130
case CTRL_CMD_BOND_DB_SET:
130131
// util_log("da14531: set bond db");
131-
// util_log("da14531: bond db len %d", frame->payload_length - 1);
132+
if (frame->payload_length < 2) {
133+
util_log("da14531: set bond db, invalid length");
134+
ASSERT(false);
135+
break;
136+
}
132137
memory_set_ble_bond_db(&frame->cmd_data[0], frame->payload_length - 1);
133138
break;
134139
case CTRL_CMD_PAIRING_CODE: {
135140
if (frame->payload_length < 5) {
136-
// TODO handle error.
137-
Abort("Invalid payload length for BLE pairing code");
141+
util_log("da14531: pairing code, invalid length");
142+
ASSERT(false);
143+
break;
138144
}
139145
#if !defined(BOOTLOADER)
140146
memcpy(
@@ -163,6 +169,11 @@ static void _ctrl_handler(struct da14531_ctrl_frame* frame, struct ringbuffer* q
163169
#endif
164170
} break;
165171
case CTRL_CMD_BLE_STATUS:
172+
if (frame->payload_length < 2) {
173+
util_log("da14531: ble status, invalid length");
174+
ASSERT(false);
175+
break;
176+
}
166177
if (frame->cmd_data[0] <= DA14531_CONNECTED_CONNECTED_SECURED) {
167178
da14531_connected_state = frame->cmd_data[0];
168179
}
@@ -200,6 +211,7 @@ static void _ctrl_handler(struct da14531_ctrl_frame* frame, struct ringbuffer* q
200211
uint8_t response[1 + MEMORY_BLE_IRK_LEN] = {0}; //+1 for cmd
201212
response[0] = CTRL_CMD_IRK;
202213
memory_get_ble_irk(&response[1]);
214+
uint8_t tmp[12 + sizeof(response) * 2];
203215
uint16_t len = da14531_protocol_format(
204216
&tmp[0],
205217
sizeof(tmp),
@@ -223,6 +235,7 @@ static void _ctrl_handler(struct da14531_ctrl_frame* frame, struct ringbuffer* q
223235
uint8_t response[1 + MEMORY_BLE_ADDR_LEN] = {0};
224236
response[0] = CTRL_CMD_IDENTITY_ADDRESS;
225237
memory_get_ble_identity_address(&response[1]);
238+
uint8_t tmp[12 + sizeof(response) * 2];
226239
uint16_t len = da14531_protocol_format(
227240
&tmp[0],
228241
sizeof(tmp),
@@ -235,8 +248,9 @@ static void _ctrl_handler(struct da14531_ctrl_frame* frame, struct ringbuffer* q
235248
ringbuffer_put(queue, tmp[i]);
236249
}
237250
} break;
251+
#if !defined(NDEBUG)
238252
case CTRL_CMD_PAIRING_SUCCESSFUL:
239-
// util_log("da14531: pairing successful");
253+
util_log("da14531: pairing successful");
240254
break;
241255
case CTRL_CMD_DEBUG:
242256
util_log(
@@ -245,6 +259,7 @@ static void _ctrl_handler(struct da14531_ctrl_frame* frame, struct ringbuffer* q
245259
&frame->cmd_data[0],
246260
frame->payload_length - 1);
247261
break;
262+
#endif
248263
default:
249264
break;
250265
}

‎src/da14531/da14531_protocol.c

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,6 @@ static void _firmware_loader_poll(
182182
break;
183183
case FIRMWARE_LOADER_STATE_SENT_FIRMWARE:
184184
if (*buf_in_len == 1) {
185-
if (ble_fw == NULL || ble_fw_size == 0) {
186-
Abort("ble_fw is NULL");
187-
}
188185
if (buf_in[0] == ble_fw_checksum) {
189186
util_log("da14531: checksum success (%x)", buf_in[0]);
190187
ASSERT(ringbuffer_num(out_queue) + 1 <= out_queue->size);
@@ -257,13 +254,20 @@ static struct da14531_protocol_frame* _serial_link_in_poll(
257254
self->escape_state = ESCAPE_STATE_ACCEPT;
258255
if (self->frame_len > 0) {
259256
// save the bytes that wasn't consumed
260-
memcpy(&self->buf_in[0], &self->buf_in[i + 1], self->buf_in_len);
257+
memmove(&self->buf_in[0], &self->buf_in[i + 1], self->buf_in_len);
261258
self->state = SERIAL_LINK_STATE_CHECK;
262259
break;
263260
}
264261
continue;
265262
}
266263

264+
// If we ran out of space while parsing an incoming frame
265+
if (self->frame_len >= sizeof(self->frame)) {
266+
ASSERT(false);
267+
self->frame_len = 0;
268+
self->escape_state = ESCAPE_STATE_WAIT;
269+
}
270+
267271
switch (self->escape_state) {
268272
case ESCAPE_STATE_WAIT:
269273
break;
@@ -388,7 +392,7 @@ struct da14531_protocol_frame* da14531_protocol_poll(
388392
struct ringbuffer* out_queue)
389393
{
390394
if (hww_data && *hww_data) {
391-
uint8_t tmp[128];
395+
uint8_t tmp[12 + 64 * 2];
392396
int len = da14531_protocol_format(
393397
&tmp[0], sizeof(tmp), DA14531_PROTOCOL_PACKET_TYPE_BLE_DATA, *hww_data, 64);
394398
ASSERT(len < (int)sizeof(tmp));

‎src/da14531/da14531_protocol.h

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,21 @@ struct da14531_protocol_frame* da14531_protocol_poll(
4747
const uint8_t** hww_data,
4848
struct ringbuffer* out_queue);
4949

50-
/// Formats a packet into buf for sending over serial
51-
/// Returns number of bytes formatted
50+
// Formats a packet into buf for sending over serial. Worst case the buf_len needs to fit:
51+
// SOF - 1 byte
52+
// type - 1 byte
53+
// len - 2 bytes
54+
// payload - payload_len bytes
55+
// CRC - 2 bytes
56+
// EOF - 1 byte
57+
//
58+
// Type, len, payload and crc will have some bytes escaped so worst case takes twice the space.
59+
//
60+
// 2 + (1+2+payload_len+2)*2 = 2 + (5+payload_len)*2 = 12 + 2*payload_len
61+
//
62+
// For example, 64 bytes require 140 byte buffer worst case.
63+
//
64+
// Returns number of formattedb bytes
5265
uint16_t da14531_protocol_format(
5366
uint8_t* buf,
5467
uint16_t buf_len,

‎src/memory/memory_shared.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,9 @@ int16_t memory_get_ble_bond_db(uint8_t* data)
180180
bool memory_set_ble_bond_db(uint8_t* data, int16_t data_len)
181181
{
182182
ASSERT(data_len <= MEMORY_BLE_BOND_DB_LEN);
183+
if (data_len > MEMORY_BLE_BOND_DB_LEN) {
184+
return false;
185+
}
183186
chunk_shared_t chunk = {0};
184187
CLEANUP_CHUNK(chunk);
185188
memory_read_shared_bootdata(&chunk);

0 commit comments

Comments
 (0)