Skip to content

Commit c2b833e

Browse files
committed
core: Fix large packet buffer overrun
The astlpc binding allows negotiation of Tx/Rx region sizes, but the packet accumulator assumed packet sizes were at most 4096 bytes. Avoid buffer overflow by allocating at least the length of the inbound packet if we have not yet initialised the packet buffer. Fixes: ================================================================= ==42296==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x621000002500 at pc 0x7ff8a22235ce bp 0x7ffd47469750 sp 0x7ffd47468ef8 WRITE of size 8192 at 0x621000002500 thread T0 #0 0x7ff8a22235cd in __interceptor_memcpy (/usr/lib/x86_64-linux-gnu/libasan.so.6+0x3a5cd) #1 0x7ff8a21ac78b in memcpy /usr/include/x86_64-linux-gnu/bits/string_fortified.h:34 #2 0x7ff8a21ac78b in mctp_msg_ctx_add_pkt /home/andrew/src/openbmc/libmctp/core.c:237 #3 0x7ff8a21af245 in mctp_bus_rx /home/andrew/src/openbmc/libmctp/core.c:495 #4 0x56458d3f9648 in mctp_astlpc_rx_start astlpc.c:813 #5 0x56458d3f9648 in mctp_astlpc_poll astlpc.c:931 #6 0x56458d3fc1f4 in astlpc_test_send_large_packet tests/test_astlpc.c:1111 #7 0x56458d3efc86 in main tests/test_astlpc.c:1185 #8 0x7ff8a165dcb1 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28cb1) #9 0x56458d3efe7d in _start (/home/andrew/src/openbmc/libmctp/tests/.libs/test_astlpc+0x17e7d) 0x621000002500 is located 0 bytes to the right of 4096-byte region [0x621000001500,0x621000002500) allocated by thread T0 here: #0 0x7ff8a22998d0 in __interceptor_realloc (/usr/lib/x86_64-linux-gnu/libasan.so.6+0xb08d0) #1 0x7ff8a21b0533 in __mctp_realloc /home/andrew/src/openbmc/libmctp/alloc.c:48 SUMMARY: AddressSanitizer: heap-buffer-overflow (/usr/lib/x86_64-linux-gnu/libasan.so.6+0x3a5cd) in __interceptor_memcpy Shadow bytes around the buggy address: 0x0c427fff8450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c427fff8460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c427fff8470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c427fff8480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c427fff8490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x0c427fff84a0:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fff84b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fff84c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fff84d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fff84e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fff84f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==42296==ABORTING Signed-off-by: Andrew Jeffery <[email protected]> Change-Id: I9d39090cb9246ec2f6c06942d4f2a91fe0df0202
1 parent 5a50891 commit c2b833e

File tree

2 files changed

+62
-1
lines changed

2 files changed

+62
-1
lines changed

core.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "libmctp-alloc.h"
1717
#include "libmctp-log.h"
1818
#include "libmctp-cmds.h"
19+
#include "range.h"
1920

2021
/* Internal data structures */
2122

@@ -214,7 +215,7 @@ static int mctp_msg_ctx_add_pkt(struct mctp_msg_ctx *ctx,
214215

215216
/* @todo: finer-grained allocation */
216217
if (!ctx->buf_alloc_size) {
217-
new_alloc_size = 4096;
218+
new_alloc_size = MAX(len, 4096UL);
218219
} else {
219220
new_alloc_size = ctx->buf_alloc_size * 2;
220221
}

tests/test_astlpc.c

+60
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include <stdio.h>
2727
#include <stdlib.h>
2828
#include <string.h>
29+
#include <sys/random.h>
2930

3031
#ifndef ARRAY_SIZE
3132
#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
@@ -1070,6 +1071,64 @@ static void astlpc_test_negotiate_mtu_low_high(void)
10701071
free(lpc_mem);
10711072
}
10721073

1074+
static void astlpc_test_send_large_packet(void)
1075+
{
1076+
struct astlpc_endpoint *bmc, *host;
1077+
struct astlpc_test ctx;
1078+
uint8_t kcs[2] = { 0 };
1079+
void *lpc_mem;
1080+
int rc;
1081+
1082+
host = &ctx.host;
1083+
bmc = &ctx.bmc;
1084+
1085+
/* Test harness initialisation */
1086+
lpc_mem = calloc(1, 1 * 1024 * 1024);
1087+
assert(lpc_mem);
1088+
1089+
/* BMC initialisation */
1090+
rc = endpoint_init(bmc, 8, MCTP_BINDING_ASTLPC_MODE_BMC, 8192, &kcs,
1091+
lpc_mem);
1092+
assert(!rc);
1093+
1094+
/* Host initialisation */
1095+
rc = endpoint_init(host, 9, MCTP_BINDING_ASTLPC_MODE_HOST, 8192, &kcs,
1096+
lpc_mem);
1097+
assert(!rc);
1098+
1099+
ctx.count = 0;
1100+
mctp_set_rx_all(bmc->mctp, rx_message, &ctx);
1101+
1102+
rc = mctp_astlpc_poll(bmc->astlpc);
1103+
assert(rc == 0);
1104+
1105+
rc = mctp_astlpc_poll(host->astlpc);
1106+
assert(rc == 0);
1107+
1108+
ctx.msg = malloc(2 * MCTP_BODY_SIZE(8192));
1109+
assert(ctx.msg);
1110+
1111+
memset(ctx.msg, 0x5a, 2 * MCTP_BODY_SIZE(8192));
1112+
1113+
rc = mctp_message_tx(host->mctp, 8, ctx.msg, 2 * MCTP_BODY_SIZE(8192));
1114+
assert(rc == 0);
1115+
rc = mctp_astlpc_poll(bmc->astlpc);
1116+
assert(rc == 0);
1117+
rc = mctp_astlpc_poll(host->astlpc);
1118+
assert(rc == 0);
1119+
rc = mctp_astlpc_poll(bmc->astlpc);
1120+
assert(rc == 0);
1121+
rc = mctp_astlpc_poll(host->astlpc);
1122+
assert(rc == 0);
1123+
1124+
assert(ctx.count == 1);
1125+
1126+
free(ctx.msg);
1127+
endpoint_destroy(host);
1128+
endpoint_destroy(bmc);
1129+
free(lpc_mem);
1130+
}
1131+
10731132
/* clang-format off */
10741133
#define TEST_CASE(test) { #test, test }
10751134
static const struct {
@@ -1109,6 +1168,7 @@ static const struct {
11091168
TEST_CASE(astlpc_test_buffers_bad_host_init),
11101169
TEST_CASE(astlpc_test_negotiate_increased_mtu),
11111170
TEST_CASE(astlpc_test_negotiate_mtu_low_high),
1171+
TEST_CASE(astlpc_test_send_large_packet),
11121172
};
11131173
/* clang-format on */
11141174

0 commit comments

Comments
 (0)