Skip to content

Commit 7728587

Browse files
de-nordicnashif
authored andcommitted
mgmt/mcumgr/lib: Fix no check on message
The commit fixes smp_process_request_packet not checking header stated message length against actual message length. This could lead to an erratic behavior of an application when it tries to parse CBOR data beyond actual size of message or potential Zephyr crash. When application has asserts enabled the error leads to inevitable Zephyr crash, caused by net_buf checks. Signed-off-by: Dominik Ermel <[email protected]>
1 parent 56ebbb4 commit 7728587

File tree

1 file changed

+9
-15
lines changed
  • subsys/mgmt/mcumgr/lib/smp/src

1 file changed

+9
-15
lines changed

subsys/mgmt/mcumgr/lib/smp/src/smp.c

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,6 @@
1414
#include "mgmt/mgmt.h"
1515
#include "smp/smp.h"
1616

17-
static int
18-
smp_align4(int x)
19-
{
20-
int rem;
21-
22-
rem = x % 4;
23-
if (rem == 0) {
24-
return x;
25-
} else {
26-
return x - rem + 4;
27-
}
28-
}
29-
3017
/**
3118
* Converts a request opcode to its corresponding response opcode.
3219
*/
@@ -289,7 +276,8 @@ smp_on_err(struct smp_streamer *streamer, const struct mgmt_hdr *req_hdr,
289276
* @param streamer The streamer to use for reading, writing, and transmitting.
290277
* @param req A buffer containing the request packet.
291278
*
292-
* @return 0 on success, MGMT_ERR_ECORRUPT if buffer starts with non SMP data header,
279+
* @return 0 on success, MGMT_ERR_ECORRUPT if buffer starts with non SMP data header
280+
* or there is not enough bytes to process header,
293281
* or other MGMT_ERR_[...] code on failure.
294282
*/
295283
int
@@ -318,6 +306,12 @@ smp_process_request_packet(struct smp_streamer *streamer, void *req)
318306
valid_hdr = true;
319307
}
320308
mgmt_ntoh_hdr(&req_hdr);
309+
/* Does buffer contain whole message? */
310+
if (streamer->mgmt_stmr.reader->message_size < (req_hdr.nh_len + MGMT_HDR_SIZE)) {
311+
rc = MGMT_ERR_ECORRUPT;
312+
break;
313+
}
314+
321315
mgmt_streamer_trim_front(&streamer->mgmt_stmr, req, MGMT_HDR_SIZE);
322316

323317
rsp = mgmt_streamer_alloc_rsp(&streamer->mgmt_stmr, req);
@@ -345,7 +339,7 @@ smp_process_request_packet(struct smp_streamer *streamer, void *req)
345339
}
346340

347341
/* Trim processed request to free up space for subsequent responses. */
348-
mgmt_streamer_trim_front(&streamer->mgmt_stmr, req, smp_align4(req_hdr.nh_len));
342+
mgmt_streamer_trim_front(&streamer->mgmt_stmr, req, req_hdr.nh_len);
349343

350344
cmd_done_arg.err = MGMT_ERR_EOK;
351345
mgmt_evt(MGMT_EVT_OP_CMD_DONE, req_hdr.nh_group, req_hdr.nh_id,

0 commit comments

Comments
 (0)