msgbuf: Harden pldm_msgbuf_{insert,extract}_array()
Review of some proposed APIs suggested that correct use of the
pldm_msgbuf_{insert,extract}_array() helpers was more difficult that it
should be. In the three-parameter form, it was too tempting to provide
the length to extract as parsed out of a PLDM message. The intended
use was that the length parameter represented the length of the
user-provided data buffer.
Instead, move to a four-parameter form, provide reasonable documentation
for how these APIs should be used, fix all the call-sites, and deprecate
some existing unsafe APIs.
Change-Id: If58e5574600e80b354f383554283c4eda5d7234c
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
diff --git a/src/compiler.h b/src/compiler.h
index 3657471..af392b1 100644
--- a/src/compiler.h
+++ b/src/compiler.h
@@ -13,11 +13,14 @@
"`always_inline` attribute is required");
static_assert(__has_attribute(unused),
"`unused` attribute is required");
+ static_assert(__has_attribute(warn_unused_result),
+ "`warn_unused_result` attribute is required");
int compliance;
} pldm_required_attributes __attribute__((unused));
-#define LIBPLDM_CC_ALWAYS_INLINE __attribute__((always_inline)) static inline
-#define LIBPLDM_CC_UNUSED __attribute__((unused))
+#define LIBPLDM_CC_ALWAYS_INLINE __attribute__((always_inline)) static inline
+#define LIBPLDM_CC_UNUSED __attribute__((unused))
+#define LIBPLDM_CC_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
// NOLINTBEGIN(bugprone-macro-parentheses)
/**
diff --git a/src/dsp/base.h b/src/dsp/base.h
index 0e2c1a0..1124b00 100644
--- a/src/dsp/base.h
+++ b/src/dsp/base.h
@@ -4,6 +4,7 @@
/* Internal functions */
+#include "compiler.h"
#include <libpldm/base.h>
int pack_pldm_header_errno(const struct pldm_header_info *hdr,
@@ -12,4 +13,11 @@
int unpack_pldm_header_errno(const struct pldm_msg_hdr *msg,
struct pldm_header_info *hdr);
+LIBPLDM_CC_ALWAYS_INLINE
+int pldm_msg_has_error(const struct pldm_msg *msg, size_t payload_length)
+{
+ static_assert(PLDM_SUCCESS == 0, "Rework required");
+ return payload_length < 1 ? 0 : msg->payload[0];
+}
+
#endif
diff --git a/src/dsp/firmware_update.c b/src/dsp/firmware_update.c
index 80310b2..61bb953 100644
--- a/src/dsp/firmware_update.c
+++ b/src/dsp/firmware_update.c
@@ -1152,8 +1152,14 @@
if (rc < 0) {
return rc;
}
- pldm_msgbuf_extract_array(buf, entry->active_comp_release_date,
- PLDM_FWUP_COMPONENT_RELEASE_DATA_LEN);
+ rc = pldm_msgbuf_extract_array(buf,
+ PLDM_FWUP_COMPONENT_RELEASE_DATA_LEN,
+ entry->active_comp_release_date,
+ sizeof(entry->active_comp_release_date));
+ if (rc < 0) {
+ return rc;
+ }
+
// Fill the last byte with NULL character
entry->active_comp_release_date[PLDM_FWUP_COMPONENT_RELEASE_DATA_LEN] =
'\0';
@@ -1164,8 +1170,15 @@
if (rc < 0) {
return rc;
}
- pldm_msgbuf_extract_array(buf, entry->pending_comp_release_date,
- PLDM_FWUP_COMPONENT_RELEASE_DATA_LEN);
+
+ rc = pldm_msgbuf_extract_array(
+ buf, PLDM_FWUP_COMPONENT_RELEASE_DATA_LEN,
+ entry->pending_comp_release_date,
+ sizeof(entry->pending_comp_release_date));
+ if (rc < 0) {
+ return rc;
+ }
+
// Fill the last byte with NULL character
entry->pending_comp_release_date[PLDM_FWUP_COMPONENT_RELEASE_DATA_LEN] =
'\0';
@@ -1218,10 +1231,20 @@
return rc;
}
- pldm_msgbuf_extract_array(buf, active, entry->active_comp_ver_str_len);
+ rc = pldm_msgbuf_extract_array(buf, entry->active_comp_ver_str_len,
+ active, entry->active_comp_ver_str_len);
+ if (rc < 0) {
+ return rc;
+ }
+
active[entry->active_comp_ver_str_len] = '\0';
- pldm_msgbuf_extract_array(buf, pending,
- entry->pending_comp_ver_str_len);
+ rc = pldm_msgbuf_extract_array(buf, entry->pending_comp_ver_str_len,
+ pending,
+ entry->pending_comp_ver_str_len);
+ if (rc < 0) {
+ return rc;
+ }
+
pending[entry->pending_comp_ver_str_len] = '\0';
entry->active_comp_ver_str = active;
diff --git a/src/dsp/platform.c b/src/dsp/platform.c
index 6ef1061..e355792 100644
--- a/src/dsp/platform.c
+++ b/src/dsp/platform.c
@@ -421,7 +421,7 @@
return PLDM_SUCCESS;
}
-LIBPLDM_ABI_STABLE
+LIBPLDM_ABI_DEPRECATED
int decode_get_pdr_repository_info_resp(
const struct pldm_msg *msg, size_t payload_length,
uint8_t *completion_code, uint8_t *repository_state,
@@ -457,8 +457,20 @@
return PLDM_ERROR_INVALID_DATA;
}
- pldm_msgbuf_extract_array(buf, update_time, PLDM_TIMESTAMP104_SIZE);
- pldm_msgbuf_extract_array(buf, oem_update_time, PLDM_TIMESTAMP104_SIZE);
+ /* NOTE: Memory safety */
+ rc = pldm_msgbuf_extract_array(buf, PLDM_TIMESTAMP104_SIZE, update_time,
+ PLDM_TIMESTAMP104_SIZE);
+ if (rc) {
+ return rc;
+ }
+
+ /* NOTE: Memory safety */
+ rc = pldm_msgbuf_extract_array(buf, PLDM_TIMESTAMP104_SIZE,
+ oem_update_time, PLDM_TIMESTAMP104_SIZE);
+ if (rc) {
+ return rc;
+ }
+
pldm_msgbuf_extract_p(buf, record_count);
pldm_msgbuf_extract_p(buf, repository_size);
pldm_msgbuf_extract_p(buf, largest_record_size);
@@ -467,6 +479,61 @@
return pldm_msgbuf_destroy(buf);
}
+LIBPLDM_ABI_TESTING
+int decode_get_pdr_repository_info_resp_safe(
+ const struct pldm_msg *msg, size_t payload_length,
+ struct pldm_pdr_repository_info_resp *resp)
+{
+ struct pldm_msgbuf _buf;
+ struct pldm_msgbuf *buf = &_buf;
+ int rc;
+
+ if (msg == NULL || resp == NULL) {
+ return -EINVAL;
+ }
+
+ rc = pldm_msg_has_error(msg, payload_length);
+ if (rc) {
+ resp->completion_code = rc;
+ return 0;
+ }
+
+ rc = pldm_msgbuf_init_errno(buf,
+ PLDM_GET_PDR_REPOSITORY_INFO_RESP_BYTES,
+ msg->payload, payload_length);
+ if (rc) {
+ return rc;
+ }
+
+ rc = pldm_msgbuf_extract(buf, resp->completion_code);
+ if (rc) {
+ return rc;
+ }
+
+ pldm_msgbuf_extract(buf, resp->repository_state);
+
+ rc = pldm_msgbuf_extract_array(buf, sizeof(resp->update_time),
+ resp->update_time,
+ sizeof(resp->update_time));
+ if (rc) {
+ return rc;
+ }
+
+ rc = pldm_msgbuf_extract_array(buf, sizeof(resp->oem_update_time),
+ resp->oem_update_time,
+ sizeof(resp->oem_update_time));
+ if (rc) {
+ return rc;
+ }
+
+ pldm_msgbuf_extract(buf, resp->record_count);
+ pldm_msgbuf_extract(buf, resp->repository_size);
+ pldm_msgbuf_extract(buf, resp->largest_record_size);
+ pldm_msgbuf_extract(buf, resp->data_transfer_handle_timeout);
+
+ return pldm_msgbuf_destroy_consumed(buf);
+}
+
LIBPLDM_ABI_STABLE
int encode_get_pdr_req(uint8_t instance_id, uint32_t record_hndl,
uint32_t data_transfer_hndl, uint8_t transfer_op_flag,
@@ -503,7 +570,7 @@
return PLDM_SUCCESS;
}
-LIBPLDM_ABI_STABLE
+LIBPLDM_ABI_DEPRECATED
int decode_get_pdr_resp(const struct pldm_msg *msg, size_t payload_length,
uint8_t *completion_code, uint32_t *next_record_hndl,
uint32_t *next_data_transfer_hndl,
@@ -544,7 +611,12 @@
if (record_data_length < *resp_cnt) {
return PLDM_ERROR_INVALID_LENGTH;
}
- pldm_msgbuf_extract_array(buf, record_data, *resp_cnt);
+ /* NOTE: Memory safety */
+ rc = pldm_msgbuf_extract_array(buf, *resp_cnt, record_data,
+ *resp_cnt);
+ if (rc) {
+ return rc;
+ }
}
if (*transfer_flag == PLDM_END) {
@@ -554,6 +626,59 @@
return pldm_msgbuf_destroy(buf);
}
+LIBPLDM_ABI_TESTING
+int decode_get_pdr_resp_safe(const struct pldm_msg *msg, size_t payload_length,
+ struct pldm_get_pdr_resp *resp, size_t resp_len,
+ uint8_t *transfer_crc)
+{
+ struct pldm_msgbuf _buf;
+ struct pldm_msgbuf *buf = &_buf;
+ int rc;
+
+ if (msg == NULL || resp == NULL || transfer_crc == NULL) {
+ return -EINVAL;
+ }
+
+ rc = pldm_msg_has_error(msg, payload_length);
+ if (rc) {
+ resp->completion_code = rc;
+ return 0;
+ }
+
+ rc = pldm_msgbuf_init_errno(buf, PLDM_GET_PDR_MIN_RESP_BYTES,
+ msg->payload, payload_length);
+ if (rc) {
+ return rc;
+ }
+
+ pldm_msgbuf_extract(buf, resp->completion_code);
+ pldm_msgbuf_extract(buf, resp->next_record_handle);
+ pldm_msgbuf_extract(buf, resp->next_data_transfer_handle);
+
+ rc = pldm_msgbuf_extract(buf, resp->transfer_flag);
+ if (rc) {
+ return rc;
+ }
+
+ rc = pldm_msgbuf_extract(buf, resp->response_count);
+ if (rc) {
+ return rc;
+ }
+
+ rc = pldm_msgbuf_extract_array(
+ buf, resp->response_count, resp->record_data,
+ resp_len - (sizeof(*resp) - sizeof(resp->record_data)));
+ if (rc) {
+ return rc;
+ }
+
+ if (resp->transfer_flag == PLDM_END) {
+ pldm_msgbuf_extract_p(buf, transfer_crc);
+ }
+
+ return pldm_msgbuf_destroy_consumed(buf);
+}
+
LIBPLDM_ABI_STABLE
int decode_set_numeric_effecter_value_req(const struct pldm_msg *msg,
size_t payload_length,
@@ -1057,7 +1182,11 @@
pldm_msgbuf_insert(buf, event_data_size);
if ((event_data_size > 0) && event_data) {
- pldm_msgbuf_insert_array(buf, event_data, event_data_size);
+ rc = pldm_msgbuf_insert_array(buf, event_data_size, event_data,
+ event_data_size);
+ if (rc) {
+ return rc;
+ }
}
if (transfer_flag == PLDM_END || transfer_flag == PLDM_START_AND_END) {
@@ -2768,7 +2897,10 @@
for (i = 0; i < pdr->name_string_count; i++) {
pldm_msgbuf_span_string_ascii(src, NULL, NULL);
- pldm_msgbuf_copy_string_utf16(dst, src);
+ rc = pldm_msgbuf_copy_string_utf16(dst, src);
+ if (rc) {
+ return rc;
+ }
}
rc = pldm_msgbuf_destroy_consumed(src);
@@ -2783,7 +2915,10 @@
}
for (i = 0; i < pdr->name_string_count; i++) {
- pldm_msgbuf_copy_string_ascii(dst, src);
+ rc = pldm_msgbuf_copy_string_ascii(dst, src);
+ if (rc) {
+ return rc;
+ }
pldm_msgbuf_span_string_utf16(src, NULL, NULL);
}
@@ -2902,8 +3037,12 @@
return -EOVERFLOW;
}
- pldm_msgbuf_extract_array_uint8(buf, cper_event->event_data,
- cper_event->event_data_length);
+ rc = pldm_msgbuf_extract_array_uint8(
+ buf, cper_event->event_data_length, cper_event->event_data,
+ cper_event_length - sizeof(*cper_event));
+ if (rc) {
+ return rc;
+ }
return pldm_msgbuf_destroy_consumed(buf);
}
diff --git a/src/msgbuf.h b/src/msgbuf.h
index b4fd677..c3498bb 100644
--- a/src/msgbuf.h
+++ b/src/msgbuf.h
@@ -722,14 +722,18 @@
int32_t *: pldm__msgbuf_extract_int32, \
real32_t *: pldm__msgbuf_extract_real32)(ctx, dst)
+/**
+ * @ref pldm_msgbuf_extract_array
+ */
+LIBPLDM_CC_WARN_UNUSED_RESULT
LIBPLDM_CC_ALWAYS_INLINE int
// NOLINTNEXTLINE(bugprone-reserved-identifier,cert-dcl37-c,cert-dcl51-cpp)
-pldm__msgbuf_extract_array_void(struct pldm_msgbuf *ctx, void *dst,
- size_t count)
+pldm__msgbuf_extract_array_void(struct pldm_msgbuf *ctx, size_t count,
+ void *dst, size_t dst_count)
{
assert(ctx);
- if (!ctx->cursor || !dst) {
+ if (!ctx->cursor || !dst || count > dst_count) {
return pldm_msgbuf_status(ctx, EINVAL);
}
@@ -758,23 +762,46 @@
return 0;
}
+/**
+ * @ref pldm_msgbuf_extract_array
+ */
+LIBPLDM_CC_WARN_UNUSED_RESULT
LIBPLDM_CC_ALWAYS_INLINE int
-pldm_msgbuf_extract_array_char(struct pldm_msgbuf *ctx, char *dst, size_t count)
+pldm_msgbuf_extract_array_char(struct pldm_msgbuf *ctx, size_t count, char *dst,
+ size_t dst_count)
{
- return pldm__msgbuf_extract_array_void(ctx, dst, count);
+ return pldm__msgbuf_extract_array_void(ctx, count, dst, dst_count);
}
+/**
+ * @ref pldm_msgbuf_extract_array
+ */
+LIBPLDM_CC_WARN_UNUSED_RESULT
LIBPLDM_CC_ALWAYS_INLINE int
-pldm_msgbuf_extract_array_uint8(struct pldm_msgbuf *ctx, uint8_t *dst,
- size_t count)
+pldm_msgbuf_extract_array_uint8(struct pldm_msgbuf *ctx, size_t count,
+ uint8_t *dst, size_t dst_count)
{
- return pldm__msgbuf_extract_array_void(ctx, dst, count);
+ return pldm__msgbuf_extract_array_void(ctx, count, dst, dst_count);
}
-#define pldm_msgbuf_extract_array(ctx, dst, count) \
+/**
+ * Extract an array of data from the msgbuf instance
+ *
+ * @param ctx - The msgbuf instance from which to extract an array of data
+ * @param count - The number of array elements to extract
+ * @param dst - The array object into which elements from @p ctx should be
+ extracted
+ * @param dst_count - The maximum number of elements to place into @p dst
+ *
+ * Note that both @p count and @p dst_count can only be counted by `sizeof` for
+ * arrays where `sizeof(*dst) == 1` holds. Specifically, they count the number
+ * of array elements and _not_ the object size of the array.
+ */
+#define pldm_msgbuf_extract_array(ctx, count, dst, dst_count) \
_Generic((*(dst)), \
uint8_t: pldm_msgbuf_extract_array_uint8, \
- char: pldm_msgbuf_extract_array_char)(ctx, dst, count)
+ char: pldm_msgbuf_extract_array_char)(ctx, count, dst, \
+ dst_count)
LIBPLDM_CC_ALWAYS_INLINE int pldm_msgbuf_insert_uint32(struct pldm_msgbuf *ctx,
const uint32_t src)
@@ -967,14 +994,18 @@
uint32_t: pldm_msgbuf_insert_uint32, \
int32_t: pldm_msgbuf_insert_int32)(dst, src)
+/**
+ * @ref pldm_msgbuf_insert_array
+ */
+LIBPLDM_CC_WARN_UNUSED_RESULT
LIBPLDM_CC_ALWAYS_INLINE int
// NOLINTNEXTLINE(bugprone-reserved-identifier,cert-dcl37-c,cert-dcl51-cpp)
-pldm__msgbuf_insert_array_void(struct pldm_msgbuf *ctx, const void *src,
- size_t count)
+pldm__msgbuf_insert_array_void(struct pldm_msgbuf *ctx, size_t count,
+ const void *src, size_t src_count)
{
assert(ctx);
- if (!ctx->cursor || !src) {
+ if (!ctx->cursor || !src || count > src_count) {
return pldm_msgbuf_status(ctx, EINVAL);
}
@@ -1003,24 +1034,47 @@
return 0;
}
+/**
+ * @ref pldm_msgbuf_insert_array
+ */
+LIBPLDM_CC_WARN_UNUSED_RESULT
LIBPLDM_CC_ALWAYS_INLINE int
-pldm_msgbuf_insert_array_char(struct pldm_msgbuf *ctx, const char *src,
- size_t count)
+pldm_msgbuf_insert_array_char(struct pldm_msgbuf *ctx, size_t count,
+ const char *src, size_t src_count)
{
- return pldm__msgbuf_insert_array_void(ctx, src, count);
+ return pldm__msgbuf_insert_array_void(ctx, count, src, src_count);
}
+/**
+ * @ref pldm_msgbuf_insert_array
+ */
+LIBPLDM_CC_WARN_UNUSED_RESULT
LIBPLDM_CC_ALWAYS_INLINE int
-pldm_msgbuf_insert_array_uint8(struct pldm_msgbuf *ctx, const uint8_t *src,
- size_t count)
+pldm_msgbuf_insert_array_uint8(struct pldm_msgbuf *ctx, size_t count,
+ const uint8_t *src, size_t src_count)
{
- return pldm__msgbuf_insert_array_void(ctx, src, count);
+ return pldm__msgbuf_insert_array_void(ctx, count, src, src_count);
}
-#define pldm_msgbuf_insert_array(dst, src, count) \
+/**
+ * Insert an array of data into the msgbuf instance
+ *
+ * @param ctx - The msgbuf instance into which the array of data should be
+ * inserted
+ * @param count - The number of array elements to insert
+ * @param src - The array object from which elements should be inserted into
+ @p ctx
+ * @param src_count - The maximum number of elements to insert from @p src
+ *
+ * Note that both @p count and @p src_count can only be counted by `sizeof` for
+ * arrays where `sizeof(*dst) == 1` holds. Specifically, they count the number
+ * of array elements and _not_ the object size of the array.
+ */
+#define pldm_msgbuf_insert_array(dst, count, src, src_count) \
_Generic((*(src)), \
uint8_t: pldm_msgbuf_insert_array_uint8, \
- char: pldm_msgbuf_insert_array_char)(dst, src, count)
+ char: pldm_msgbuf_insert_array_char)(dst, count, src, \
+ src_count)
LIBPLDM_CC_ALWAYS_INLINE int pldm_msgbuf_span_required(struct pldm_msgbuf *ctx,
size_t required,
@@ -1271,6 +1325,7 @@
return 0;
}
+LIBPLDM_CC_WARN_UNUSED_RESULT
LIBPLDM_CC_ALWAYS_INLINE int
pldm_msgbuf_copy_string_ascii(struct pldm_msgbuf *dst, struct pldm_msgbuf *src)
{
@@ -1283,9 +1338,10 @@
return rc;
}
- return pldm__msgbuf_insert_array_void(dst, ascii, len);
+ return pldm__msgbuf_insert_array_void(dst, len, ascii, len);
}
+LIBPLDM_CC_WARN_UNUSED_RESULT
LIBPLDM_CC_ALWAYS_INLINE int
pldm_msgbuf_copy_string_utf16(struct pldm_msgbuf *dst, struct pldm_msgbuf *src)
{
@@ -1298,7 +1354,7 @@
return rc;
}
- return pldm__msgbuf_insert_array_void(dst, utf16, len);
+ return pldm__msgbuf_insert_array_void(dst, len, utf16, len);
}
#ifdef __cplusplus
diff --git a/src/oem/meta/file_io.c b/src/oem/meta/file_io.c
index 073c446..9df6263 100644
--- a/src/oem/meta/file_io.c
+++ b/src/oem/meta/file_io.c
@@ -13,22 +13,29 @@
{
struct pldm_msgbuf _buf;
struct pldm_msgbuf *buf = &_buf;
+ int rc;
if (msg == NULL || file_handle == NULL || length == NULL ||
data == NULL) {
return PLDM_ERROR_INVALID_DATA;
}
- int rc = pldm_msgbuf_init_cc(
- buf, PLDM_OEM_META_DECODE_WRITE_FILE_IO_MIN_SIZE, msg->payload,
- payload_length);
+ rc = pldm_msgbuf_init_cc(buf,
+ PLDM_OEM_META_DECODE_WRITE_FILE_IO_MIN_SIZE,
+ msg->payload, payload_length);
if (rc) {
return rc;
}
pldm_msgbuf_extract_p(buf, file_handle);
pldm_msgbuf_extract_p(buf, length);
- pldm_msgbuf_extract_array_uint8(buf, data, *length);
+
+ /* NOTE: Memory safety failure */
+ rc = pldm_msgbuf_extract_array_uint8(buf, (size_t)(*length), data,
+ UINT32_MAX);
+ if (rc) {
+ return rc;
+ }
return pldm_msgbuf_destroy_consumed(buf);
}