From 1983421b7d68f0e74c89f17b741b04c8a015efda Mon Sep 17 00:00:00 2001 From: Stefan Kerkmann Date: Tue, 1 Nov 2022 16:54:56 +0100 Subject: [PATCH] XAP: prevent OOB reads in config blob handler (#18926) This fixes two bugs: 1. An invalid offset could be specified which wasn't checked to be in the bounds of the config blob. 2. The data_len check was incorrect as it would allow reading one byte past the config blob lenght. Before the changes the following operation wouldn't fail: Assuming we have blob of 64 bytes size and attempt a read with an offset of 32 and data_len of 32, we actually try to read 32 bytes starting from the 33. byte in the config blob. This reads exactly one byte past array. Therefore we have to subtract one byte the get the correct length. --- quantum/xap/xap.c | 8 ++++++-- quantum/xap/xap_handlers.c | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/quantum/xap/xap.c b/quantum/xap/xap.c index 6222a091dc0..26f896b75ef 100644 --- a/quantum/xap/xap.c +++ b/quantum/xap/xap.c @@ -21,8 +21,12 @@ #include "lighting_map.h" #include "config_blob_gz.h" bool get_config_blob_chunk(uint16_t offset, uint8_t *data, uint8_t data_len) { - if (offset + data_len > CONFIG_BLOB_GZ_LEN) { - data_len = CONFIG_BLOB_GZ_LEN - offset; + if (offset >= CONFIG_BLOB_GZ_LEN) { + return false; + } + + if (offset + data_len >= CONFIG_BLOB_GZ_LEN) { + data_len = (CONFIG_BLOB_GZ_LEN - 1) - offset; } memcpy_P(data, &config_blob_gz[offset], data_len); diff --git a/quantum/xap/xap_handlers.c b/quantum/xap/xap_handlers.c index a16dbe34c5d..9277be00b75 100644 --- a/quantum/xap/xap_handlers.c +++ b/quantum/xap/xap_handlers.c @@ -71,7 +71,9 @@ bool xap_respond_get_config_blob_chunk(xap_token_t token, const void *data, size xap_route_qmk_config_blob_chunk_t ret = {0}; bool get_config_blob_chunk(uint16_t offset, uint8_t * data, uint8_t data_len); - get_config_blob_chunk(offset, (uint8_t *)&ret, sizeof(ret)); + if (!get_config_blob_chunk(offset, (uint8_t *)&ret, sizeof(ret))) { + return false; + } return xap_respond_data(token, &ret, sizeof(ret)); }