bios_table: pldm_bios_table_iter_next(): Invalid entry halts iteration
The attribute iterator machinary prevented misbehaviour through use of
assert(). The attribute list is maintained as a linear sequence of
variably sized data structures that are packed against each other in the
address space. The iterator is implemented by assigning a callback that
can determine the length of each entry as appropriate for the entry's
type, and then moving the iterator's cursor between elements.
The length derivation for some elements was protected by assert(). To
avoid the asserts we rework the length callback prototype to return
a signed size value and indicate an error state with a negative size.
pldm_bios_table_iter_next() is reworked to detect the error case on
deriving the element size (negative size) and behave as if the iterator
has terminated.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Change-Id: I80db3fe179201b169acc68c68633d8dd3f3a6334
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 86905bf..9f0b16e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -27,6 +27,7 @@
2. requester: Expose pldm_close() in header
3. bios_table: pldm_bios_table_string_entry_encode_check(): Handle overflow
4. bios_table: pldm_bios_table_iter_create(): Return NULL on failed alloc
+5. bios_table: pldm_bios_table_iter_next(): Invalid entry halts iteration
### Deprecated
diff --git a/src/bios_table.c b/src/bios_table.c
index 910a499..ec96b82 100644
--- a/src/bios_table.c
+++ b/src/bios_table.c
@@ -4,6 +4,7 @@
#include "utils.h"
#include <assert.h>
#include <endian.h>
+#include <limits.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdlib.h>
@@ -111,11 +112,15 @@
return PLDM_SUCCESS;
}
-static size_t string_table_entry_length(const void *table_entry)
+static ssize_t string_table_entry_length(const void *table_entry)
{
const struct pldm_bios_string_table_entry *entry = table_entry;
- return sizeof(*entry) - sizeof(entry->name) +
- pldm_bios_table_string_entry_decode_string_length(entry);
+ size_t len = sizeof(*entry) - sizeof(entry->name) +
+ pldm_bios_table_string_entry_decode_string_length(entry);
+ if (len > SSIZE_MAX) {
+ return -1;
+ }
+ return (ssize_t)len;
}
static int get_bios_attr_handle(uint16_t *val)
@@ -291,12 +296,17 @@
/** @brief Get length of an enum attribute entry
*/
-static size_t attr_table_entry_length_enum(const void *arg)
+static ssize_t attr_table_entry_length_enum(const void *arg)
{
const struct pldm_bios_attr_table_entry *entry = arg;
uint8_t pv_num = entry->metadata[0];
uint8_t def_num = pldm_bios_table_attr_entry_enum_decode_def_num(entry);
- return pldm_bios_table_attr_entry_enum_encode_length(pv_num, def_num);
+ size_t len =
+ pldm_bios_table_attr_entry_enum_encode_length(pv_num, def_num);
+ if (len > SSIZE_MAX) {
+ return -1;
+ }
+ return (ssize_t)len;
}
struct attr_table_string_entry_fields {
@@ -456,12 +466,17 @@
/** @brief Get length of a string attribute entry
*/
-static size_t attr_table_entry_length_string(const void *entry)
+static ssize_t attr_table_entry_length_string(const void *entry)
{
uint16_t def_str_len =
pldm_bios_table_attr_entry_string_decode_def_string_length(
entry);
- return pldm_bios_table_attr_entry_string_encode_length(def_str_len);
+ size_t len =
+ pldm_bios_table_attr_entry_string_encode_length(def_str_len);
+ if (len > SSIZE_MAX) {
+ return -1;
+ }
+ return (ssize_t)len;
}
struct attr_table_integer_entry_fields {
@@ -572,15 +587,19 @@
*def = le64toh(fields->default_value);
}
-static size_t attr_table_entry_length_integer(const void *entry)
+static ssize_t attr_table_entry_length_integer(const void *entry)
{
(void)entry;
- return pldm_bios_table_attr_entry_integer_encode_length();
+ size_t len = pldm_bios_table_attr_entry_integer_encode_length();
+ if (len > SSIZE_MAX) {
+ return -1;
+ }
+ return (ssize_t)len;
}
struct table_entry_length {
uint8_t attr_type;
- size_t (*entry_length_handler)(const void *);
+ ssize_t (*entry_length_handler)(const void *);
};
#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
@@ -614,7 +633,7 @@
.entry_length_handler = attr_table_entry_length_integer },
};
-static size_t attr_table_entry_length(const void *table_entry)
+static ssize_t attr_table_entry_length(const void *table_entry)
{
const struct pldm_bios_attr_table_entry *entry = table_entry;
const struct table_entry_length *attr_table_entry =
@@ -622,7 +641,13 @@
attr_table_entries,
ARRAY_SIZE(attr_table_entries));
assert(attr_table_entry != NULL);
+ if (!attr_table_entry) {
+ return -1;
+ }
assert(attr_table_entry->entry_length_handler != NULL);
+ if (!attr_table_entry->entry_length_handler) {
+ return -1;
+ }
return attr_table_entry->entry_length_handler(entry);
}
@@ -703,11 +728,16 @@
return PLDM_SUCCESS;
}
-static size_t attr_value_table_entry_length_enum(const void *entry)
+static ssize_t attr_value_table_entry_length_enum(const void *entry)
{
uint8_t number =
pldm_bios_table_attr_value_entry_enum_decode_number(entry);
- return pldm_bios_table_attr_value_entry_encode_enum_length(number);
+ size_t len =
+ pldm_bios_table_attr_value_entry_encode_enum_length(number);
+ if (len > SSIZE_MAX) {
+ return -1;
+ }
+ return (ssize_t)len;
}
LIBPLDM_ABI_STABLE
@@ -774,12 +804,16 @@
return PLDM_SUCCESS;
}
-static size_t attr_value_table_entry_length_string(const void *entry)
+static ssize_t attr_value_table_entry_length_string(const void *entry)
{
uint16_t str_length =
pldm_bios_table_attr_value_entry_string_decode_length(entry);
- return pldm_bios_table_attr_value_entry_encode_string_length(
+ size_t len = pldm_bios_table_attr_value_entry_encode_string_length(
str_length);
+ if (len > SSIZE_MAX) {
+ return -1;
+ }
+ return (ssize_t)len;
}
LIBPLDM_ABI_STABLE
@@ -832,10 +866,14 @@
return cv;
}
-static size_t attr_value_table_entry_length_integer(const void *entry)
+static ssize_t attr_value_table_entry_length_integer(const void *entry)
{
(void)entry;
- return pldm_bios_table_attr_value_entry_encode_integer_length();
+ size_t len = pldm_bios_table_attr_value_entry_encode_integer_length();
+ if (len > SSIZE_MAX) {
+ return -1;
+ }
+ return (ssize_t)len;
}
static const struct table_entry_length attr_value_table_entries[] = {
@@ -853,7 +891,7 @@
.entry_length_handler = attr_value_table_entry_length_integer },
};
-static size_t attr_value_table_entry_length(const void *table_entry)
+static ssize_t attr_value_table_entry_length(const void *table_entry)
{
const struct pldm_bios_attr_val_table_entry *entry = table_entry;
const struct table_entry_length *entry_length =
@@ -861,7 +899,13 @@
entry->attr_type, attr_value_table_entries,
ARRAY_SIZE(attr_value_table_entries));
assert(entry_length != NULL);
+ if (!entry_length) {
+ return -1;
+ }
assert(entry_length->entry_length_handler != NULL);
+ if (!entry_length->entry_length_handler) {
+ return -1;
+ }
return entry_length->entry_length_handler(entry);
}
@@ -951,7 +995,7 @@
const uint8_t *table_data;
size_t table_len;
size_t current_pos;
- size_t (*entry_length_handler)(const void *table_entry);
+ ssize_t (*entry_length_handler)(const void *table_entry);
};
LIBPLDM_ABI_STABLE
@@ -993,10 +1037,15 @@
LIBPLDM_ABI_STABLE
bool pldm_bios_table_iter_is_end(const struct pldm_bios_table_iter *iter)
{
+ ssize_t len;
+
if (iter->table_len - iter->current_pos <= pad_and_check_max) {
return true;
}
- return false;
+
+ len = iter->entry_length_handler(iter->table_data + iter->current_pos);
+
+ return len < 0;
}
LIBPLDM_ABI_STABLE
@@ -1006,7 +1055,12 @@
return;
}
const void *entry = iter->table_data + iter->current_pos;
- iter->current_pos += iter->entry_length_handler(entry);
+ ssize_t rc = iter->entry_length_handler(entry);
+ /* Prevent bad behaviour by acting as if we've hit the end of the iterator */
+ if (rc < 0) {
+ return;
+ }
+ iter->current_pos += rc;
}
LIBPLDM_ABI_STABLE
diff --git a/tests/bios_table_iter.c b/tests/bios_table_iter.c
new file mode 100644
index 0000000..f8da402
--- /dev/null
+++ b/tests/bios_table_iter.c
@@ -0,0 +1,54 @@
+/* Force elision of assert() */
+#ifndef NDEBUG
+#define NDEBUG
+#endif
+
+#include <assert.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+/* NOLINTNEXTLINE(bugprone-suspicious-include) */
+#include "bios_table.c"
+
+/* Satisfy the symbol needs of bios_table.c */
+uint32_t crc32(const void* data __attribute__((unused)),
+ size_t size __attribute__((unused)))
+{
+ return 0;
+}
+
+/* This is the non-death version of TEST(Iterator, DeathTest) */
+int main(void)
+{
+ struct pldm_bios_attr_table_entry entries[2] = {0};
+ struct pldm_bios_table_iter* iter;
+ int result;
+
+ static_assert(2 * sizeof(entries[0]) == sizeof(entries), "");
+
+ entries[0].attr_type = PLDM_BIOS_PASSWORD;
+ entries[1].attr_type = PLDM_BIOS_STRING_READ_ONLY;
+
+ iter = pldm_bios_table_iter_create(entries, sizeof(entries),
+ PLDM_BIOS_ATTR_TABLE);
+
+ /*
+ * We expect the test configuration to claim the iterator has reached the
+ * end beause the there's no entry length descriptor for the
+ * PLDM_BIOS_PASSWORD entry type. By the attr_able_entry_length()
+ * implementation this would normally trigger an assert() to uphold that the
+ * necessary pointers are not NULL. However, we've defined NDEBUG above and
+ * so the assert() is elided. That should force us down the path of the
+ * early-exit, which should in-turn yield a `true` result from
+ * pldm_bios_table_iter_is_end() to prevent further attempts to access
+ * invalid objects.
+ */
+ result = pldm_bios_table_iter_is_end(iter) ? EXIT_SUCCESS : EXIT_FAILURE;
+
+ pldm_bios_table_iter_free(iter);
+
+ exit(result);
+
+ return 0;
+}
diff --git a/tests/meson.build b/tests/meson.build
index 78d65ee..834f19c 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -46,6 +46,11 @@
workdir: meson.current_source_dir())
endforeach
+test('bios_table_iter', executable('bios_table_iter',
+ 'bios_table_iter.c',
+ implicit_include_directories: false,
+ include_directories: src_includes))
+
test('msgbuf_generic', executable('msgbuf_generic',
'msgbuf_generic.c',
implicit_include_directories: false,