Skip to content

Commit c7fec71

Browse files
sidchacarlescufi
sidcha
authored and
carlescufi
committed
mgmt/osdp: Add length checks for commands and replies
For all commands and replies, the buffer length needed to build or the length of data needed to decode needs to be checked and asserted. Right now we do this by ad-hoc if-s. Add macros that do this at a common location. Signed-off-by: Siddharth Chandrasekaran <[email protected]>
1 parent f4e3f2b commit c7fec71

File tree

2 files changed

+61
-83
lines changed

2 files changed

+61
-83
lines changed

‎subsys/mgmt/osdp/src/osdp_cp.c

Lines changed: 41 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,12 @@ int osdp_extract_address(int *address)
8080
return (pd_offset == CONFIG_OSDP_NUM_CONNECTED_PD) ? 0 : -1;
8181
}
8282

83+
static inline void assert_buf_len(int need, int have)
84+
{
85+
__ASSERT(need < have, "OOM at build command: need:%d have:%d",
86+
need, have);
87+
}
88+
8389
static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len)
8490
{
8591
struct osdp_cmd *cmd = NULL;
@@ -97,45 +103,50 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len)
97103

98104
switch (pd->cmd_id) {
99105
case CMD_POLL:
100-
__fallthrough;
106+
assert_buf_len(CMD_POLL_LEN, max_len);
107+
buf[len++] = pd->cmd_id;
108+
ret = 0;
109+
break;
101110
case CMD_LSTAT:
102-
__fallthrough;
111+
assert_buf_len(CMD_LSTAT_LEN, max_len);
112+
buf[len++] = pd->cmd_id;
113+
ret = 0;
114+
break;
103115
case CMD_ISTAT:
104-
__fallthrough;
116+
assert_buf_len(CMD_ISTAT_LEN, max_len);
117+
buf[len++] = pd->cmd_id;
118+
ret = 0;
119+
break;
105120
case CMD_OSTAT:
106-
__fallthrough;
121+
assert_buf_len(CMD_OSTAT_LEN, max_len);
122+
buf[len++] = pd->cmd_id;
123+
ret = 0;
124+
break;
107125
case CMD_RSTAT:
126+
assert_buf_len(CMD_RSTAT_LEN, max_len);
108127
buf[len++] = pd->cmd_id;
109128
ret = 0;
110129
break;
111130
case CMD_ID:
112-
if (max_len < CMD_ID_LEN) {
113-
break;
114-
}
131+
assert_buf_len(CMD_ID_LEN, max_len);
115132
buf[len++] = pd->cmd_id;
116133
buf[len++] = 0x00;
117134
ret = 0;
118135
break;
119136
case CMD_CAP:
120-
if (max_len < CMD_CAP_LEN) {
121-
break;
122-
}
137+
assert_buf_len(CMD_CAP_LEN, max_len);
123138
buf[len++] = pd->cmd_id;
124139
buf[len++] = 0x00;
125140
ret = 0;
126141
break;
127142
case CMD_DIAG:
128-
if (max_len < CMD_DIAG_LEN) {
129-
break;
130-
}
143+
assert_buf_len(CMD_DIAG_LEN, max_len);
131144
buf[len++] = pd->cmd_id;
132145
buf[len++] = 0x00;
133146
ret = 0;
134147
break;
135148
case CMD_OUT:
136-
if (max_len < CMD_OUT_LEN) {
137-
break;
138-
}
149+
assert_buf_len(CMD_OUT_LEN, max_len);
139150
cmd = (struct osdp_cmd *)pd->cmd_data;
140151
buf[len++] = pd->cmd_id;
141152
buf[len++] = cmd->output.output_no;
@@ -145,9 +156,7 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len)
145156
ret = 0;
146157
break;
147158
case CMD_LED:
148-
if (max_len < CMD_LED_LEN) {
149-
break;
150-
}
159+
assert_buf_len(CMD_LED_LEN, max_len);
151160
cmd = (struct osdp_cmd *)pd->cmd_data;
152161
buf[len++] = pd->cmd_id;
153162
buf[len++] = cmd->led.reader;
@@ -169,9 +178,7 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len)
169178
ret = 0;
170179
break;
171180
case CMD_BUZ:
172-
if (max_len < CMD_BUZ_LEN) {
173-
break;
174-
}
181+
assert_buf_len(CMD_BUZ_LEN, max_len);
175182
cmd = (struct osdp_cmd *)pd->cmd_data;
176183
buf[len++] = pd->cmd_id;
177184
buf[len++] = cmd->buzzer.reader;
@@ -183,9 +190,7 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len)
183190
break;
184191
case CMD_TEXT:
185192
cmd = (struct osdp_cmd *)pd->cmd_data;
186-
if (max_len < (CMD_TEXT_LEN + cmd->text.length)) {
187-
break;
188-
}
193+
assert_buf_len(CMD_TEXT_LEN + cmd->text.length, max_len);
189194
buf[len++] = pd->cmd_id;
190195
buf[len++] = cmd->text.reader;
191196
buf[len++] = cmd->text.control_code;
@@ -199,9 +204,7 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len)
199204
ret = 0;
200205
break;
201206
case CMD_COMSET:
202-
if (max_len < CMD_COMSET_LEN) {
203-
break;
204-
}
207+
assert_buf_len(CMD_COMSET_LEN, max_len);
205208
cmd = (struct osdp_cmd *)pd->cmd_data;
206209
buf[len++] = pd->cmd_id;
207210
buf[len++] = cmd->comset.address;
@@ -217,9 +220,7 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len)
217220
LOG_ERR("Cannot perform KEYSET without SC!");
218221
return OSDP_CP_ERR_GENERIC;
219222
}
220-
if (max_len < CMD_KEYSET_LEN) {
221-
break;
222-
}
223+
assert_buf_len(CMD_KEYSET_LEN, max_len);
223224
buf[len++] = pd->cmd_id;
224225
buf[len++] = 1; /* key type (1: SCBK) */
225226
buf[len++] = 16; /* key length in bytes */
@@ -228,7 +229,8 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len)
228229
ret = 0;
229230
break;
230231
case CMD_CHLNG:
231-
if (smb == NULL || max_len < CMD_CHLNG_LEN) {
232+
assert_buf_len(CMD_CHLNG_LEN, max_len);
233+
if (smb == NULL) {
232234
break;
233235
}
234236
osdp_fill_random(pd->sc.cp_random, 8);
@@ -242,7 +244,8 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len)
242244
ret = 0;
243245
break;
244246
case CMD_SCRYPT:
245-
if (smb == NULL || max_len < CMD_SCRYPT_LEN) {
247+
assert_buf_len(CMD_SCRYPT_LEN, max_len);
248+
if (smb == NULL) {
246249
break;
247250
}
248251
osdp_compute_cp_cryptogram(pd);
@@ -489,6 +492,11 @@ static int cp_decode_response(struct osdp_pd *pd, uint8_t *buf, int len)
489492
return OSDP_CP_ERR_UNKNOWN;
490493
}
491494

495+
if (ret != OSDP_CP_ERR_NONE) {
496+
LOG_ERR("Failed to decode response: REPLY(%02x) for CMD(%02x)",
497+
pd->cmd_id, pd->reply_id);
498+
}
499+
492500
if (pd->cmd_id != CMD_POLL) {
493501
LOG_DBG("CMD(%02x) REPLY(%02x)", pd->cmd_id, pd->reply_id);
494502
}

‎subsys/mgmt/osdp/src/osdp_pd.c

Lines changed: 20 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,6 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len)
299299
#ifdef CONFIG_OSDP_SC_ENABLED
300300
case CMD_KEYSET:
301301
if (len != CMD_KEYSET_DATA_LEN) {
302-
LOG_ERR("CMD_KEYSET length mismatch! %d/18", len);
303302
break;
304303
}
305304
/**
@@ -342,7 +341,6 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len)
342341
break;
343342
}
344343
if (len != CMD_CHLNG_DATA_LEN) {
345-
LOG_ERR("CMD_CHLNG length mismatch! %d/8", len);
346344
break;
347345
}
348346
osdp_sc_init(pd);
@@ -356,7 +354,6 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len)
356354
}
357355
case CMD_SCRYPT:
358356
if (len != CMD_SCRYPT_DATA_LEN) {
359-
LOG_ERR("CMD_SCRYPT length mismatch! %d/16", len);
360357
break;
361358
}
362359
for (i = 0; i < 16; i++) {
@@ -373,11 +370,12 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len)
373370
return OSDP_PD_ERR_REPLY;
374371
}
375372

376-
if (ret != 0) {
377-
LOG_ERR("Invalid command structure. CMD: %02x, Len: %d",
378-
pd->cmd_id, len);
373+
if (ret == OSDP_PD_ERR_GENERIC) {
374+
LOG_ERR("Failed to decode command: CMD(%02x) Len:%d ret:%d",
375+
pd->cmd_id, len, ret);
379376
pd->reply_id = REPLY_NAK;
380377
pd->cmd_data[0] = OSDP_PD_NAK_CMD_LEN;
378+
ret = OSDP_PD_ERR_REPLY;
381379
}
382380

383381
if (pd->cmd_id != CMD_POLL) {
@@ -387,6 +385,12 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len)
387385
return ret;
388386
}
389387

388+
static inline void assert_buf_len(int need, int have)
389+
{
390+
__ASSERT(need < have, "OOM at build command: need:%d have:%d",
391+
need, have);
392+
}
393+
390394
/**
391395
* Returns:
392396
* +ve: length of command
@@ -404,25 +408,15 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len)
404408
#endif
405409
buf += data_off;
406410
max_len -= data_off;
407-
if (max_len <= 0) {
408-
LOG_ERR("Out of buffer space!");
409-
return -1;
410-
}
411411

412412
switch (pd->reply_id) {
413413
case REPLY_ACK:
414-
if (max_len < REPLY_ACK_LEN) {
415-
LOG_ERR("Out of buffer space!");
416-
break;
417-
}
414+
assert_buf_len(REPLY_ACK_LEN, max_len);
418415
buf[len++] = pd->reply_id;
419416
ret = OSDP_PD_ERR_NONE;
420417
break;
421418
case REPLY_PDID:
422-
if (max_len < REPLY_PDID_LEN) {
423-
LOG_ERR("Out of buffer space!");
424-
break;
425-
}
419+
assert_buf_len(REPLY_PDID_LEN, max_len);
426420
buf[len++] = pd->reply_id;
427421

428422
buf[len++] = BYTE_0(pd->id.vendor_code);
@@ -443,10 +437,7 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len)
443437
ret = OSDP_PD_ERR_NONE;
444438
break;
445439
case REPLY_PDCAP:
446-
if (max_len < REPLY_PDCAP_LEN) {
447-
LOG_ERR("Out of buffer space!");
448-
break;
449-
}
440+
assert_buf_len(REPLY_PDCAP_LEN, max_len);
450441
buf[len++] = pd->reply_id;
451442
for (i = 0; i < OSDP_PD_CAP_SENTINEL; i++) {
452443
if (pd->cap[i].function_code != i) {
@@ -464,29 +455,20 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len)
464455
ret = OSDP_PD_ERR_NONE;
465456
break;
466457
case REPLY_LSTATR:
467-
if (max_len < REPLY_LSTATR_LEN) {
468-
LOG_ERR("Out of buffer space!");
469-
break;
470-
}
458+
assert_buf_len(REPLY_LSTATR_LEN, max_len);
471459
buf[len++] = pd->reply_id;
472460
buf[len++] = ISSET_FLAG(pd, PD_FLAG_TAMPER);
473461
buf[len++] = ISSET_FLAG(pd, PD_FLAG_POWER);
474462
ret = OSDP_PD_ERR_NONE;
475463
break;
476464
case REPLY_RSTATR:
477-
if (max_len < REPLY_RSTATR_LEN) {
478-
LOG_ERR("Out of buffer space!");
479-
break;
480-
}
465+
assert_buf_len(REPLY_RSTATR_LEN, max_len);
481466
buf[len++] = pd->reply_id;
482467
buf[len++] = ISSET_FLAG(pd, PD_FLAG_R_TAMPER);
483468
ret = OSDP_PD_ERR_NONE;
484469
break;
485470
case REPLY_COM:
486-
if (max_len < REPLY_COM_LEN) {
487-
LOG_ERR("Out of buffer space!");
488-
break;
489-
}
471+
assert_buf_len(REPLY_COM_LEN, max_len);
490472
/**
491473
* If COMSET succeeds, the PD must reply with the old params and
492474
* then switch to the new params from then then on. We have the
@@ -517,10 +499,7 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len)
517499
ret = OSDP_PD_ERR_NONE;
518500
break;
519501
case REPLY_NAK:
520-
if (max_len < REPLY_NAK_LEN) {
521-
LOG_ERR("Fatal: insufficient space for sending NAK");
522-
return -1;
523-
}
502+
assert_buf_len(REPLY_NAK_LEN, max_len);
524503
buf[len++] = pd->reply_id;
525504
buf[len++] = pd->cmd_data[0];
526505
ret = OSDP_PD_ERR_NONE;
@@ -530,10 +509,7 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len)
530509
if (smb == NULL) {
531510
break;
532511
}
533-
if (max_len < REPLY_CCRYPT_LEN) {
534-
LOG_ERR("Out of buffer space!");
535-
return -1;
536-
}
512+
assert_buf_len(REPLY_CCRYPT_LEN, max_len);
537513
osdp_fill_random(pd->sc.pd_random, 8);
538514
osdp_compute_session_keys(pd);
539515
osdp_compute_pd_cryptogram(pd);
@@ -556,10 +532,7 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len)
556532
if (smb == NULL) {
557533
break;
558534
}
559-
if (max_len < REPLY_RMAC_I_LEN) {
560-
LOG_ERR("Out of buffer space!");
561-
return -1;
562-
}
535+
assert_buf_len(REPLY_RMAC_I_LEN, max_len);
563536
osdp_compute_rmac_i(pd);
564537
buf[len++] = pd->reply_id;
565538
for (i = 0; i < 16; i++) {
@@ -595,10 +568,7 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len)
595568
/* catch all errors and report it as a RECORD error to CP */
596569
LOG_ERR("Failed to build REPLY(%02x); Sending NAK instead!",
597570
pd->reply_id);
598-
if (max_len < REPLY_NAK_LEN) {
599-
LOG_ERR("Fatal: insufficient space for sending NAK");
600-
return -1;
601-
}
571+
assert_buf_len(REPLY_NAK_LEN, max_len);
602572
buf[0] = REPLY_NAK;
603573
buf[1] = OSDP_PD_NAK_RECORD;
604574
len = 2;

0 commit comments

Comments
 (0)