dsp: firmware_update: Bounds check decode_downstream_device_parameter_table_entry_versions()
```
../src/dsp/firmware_update.c: In function ‘decode_downstream_device_parameter_table_entry_versions’:
../src/dsp/firmware_update.c:1248:48: error: use of attacker-controlled value ‘*entry.active_comp_ver_str_len’ as offset without upper-bounds checking [CWE-823] [-Werror=analyzer-tainted-offset]
1248 | active[entry->active_comp_ver_str_len] = '\0';
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
```
gitlint-ignore: T1, B1, UC1
Fixes: b6ef35b48065 ("fw_update: Add encode req & decode resp for get_downstream_fw_params")
Change-Id: I15571804f391dc97de6d80c90325ded006aee500
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3d669ef..f1e7ed2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -57,6 +57,12 @@
3. platform: Support PLDM_CPER_EVENT in encode_platform_event_message_req()
+4. dsp: firmware_update: Bounds check
+ decode_downstream_device_parameter_table_entry_versions()
+
+ The additional bounds-checking required the addition of further length
+ parameters.
+
### Deprecated
1. oem: meta: Deprecate `decode_oem_meta_file_io_req()`
@@ -125,6 +131,8 @@
9. dsp: bios_table: Bounds check
pldm_bios_table_attr_value_entry_encode_string()
10. dsp: bios_table: Bounds check pldm_bios_table_attr_value_entry_encode_enum()
+11. dsp: firmware_update: Bounds check
+ decode_downstream_device_parameter_table_entry_versions()
## [0.9.1] - 2024-09-07
diff --git a/include/libpldm/firmware_update.h b/include/libpldm/firmware_update.h
index 272a483..4666110 100644
--- a/include/libpldm/firmware_update.h
+++ b/include/libpldm/firmware_update.h
@@ -1092,10 +1092,10 @@
*
* @param[in] versions - pointer to version strings raw data
* @param[in,out] entry - pointer to the decoded downstream device parameter table entry
- * @param[out] active - pointer to active component version string, the object pointed-to
- * must be at least as large as `entry->active_comp_ver_str_len + 1`
- * @param[out] pending - pointer to pending component version string, the object pointed-to
- * must be at least as large as `entry->pending_comp_ver_str_len + 1`
+ * @param[out] active - pointer to active component version string container
+ * @param[in] active_len - The size of the object pointed to by @p active
+ * @param[out] pending - pointer to pending component version string container
+ * @param[in] pending_len - The size of the object pointed to by @p pending
*
* @note Caller is responsible for memory alloc and dealloc of all the params,
* and the param `entry` should be the instance which has successfully decoded
@@ -1105,7 +1105,7 @@
int decode_downstream_device_parameter_table_entry_versions(
const struct variable_field *versions,
struct pldm_downstream_device_parameter_entry *entry, char *active,
- char *pending);
+ size_t active_len, char *pending, size_t pending_len);
/** @brief Create PLDM request message for RequestUpdate
*
diff --git a/src/dsp/firmware_update.c b/src/dsp/firmware_update.c
index c492a05..2afcaab 100644
--- a/src/dsp/firmware_update.c
+++ b/src/dsp/firmware_update.c
@@ -1225,7 +1225,7 @@
int decode_downstream_device_parameter_table_entry_versions(
const struct variable_field *versions,
struct pldm_downstream_device_parameter_entry *entry, char *active,
- char *pending)
+ size_t active_len, char *pending, size_t pending_len)
{
struct pldm_msgbuf _buf;
struct pldm_msgbuf *buf = &_buf;
@@ -1236,6 +1236,14 @@
return -EINVAL;
}
+ if (!active_len || active_len - 1 < entry->active_comp_ver_str_len) {
+ return -EOVERFLOW;
+ }
+
+ if (!pending_len || pending_len - 1 < entry->pending_comp_ver_str_len) {
+ return -EOVERFLOW;
+ }
+
/* This API should be called after decode_downstream_device_parameter_table_entry
* has successfully decoded the entry, assume the entry data is valid here.
*/
@@ -1248,15 +1256,14 @@
}
rc = pldm_msgbuf_extract_array(buf, entry->active_comp_ver_str_len,
- active, entry->active_comp_ver_str_len);
+ active, active_len);
if (rc < 0) {
return rc;
}
active[entry->active_comp_ver_str_len] = '\0';
rc = pldm_msgbuf_extract_array(buf, entry->pending_comp_ver_str_len,
- pending,
- entry->pending_comp_ver_str_len);
+ pending, pending_len);
if (rc < 0) {
return rc;
}
diff --git a/tests/dsp/firmware_update.cpp b/tests/dsp/firmware_update.cpp
index 8c2be11..1495fd8 100644
--- a/tests/dsp/firmware_update.cpp
+++ b/tests/dsp/firmware_update.cpp
@@ -1987,7 +1987,9 @@
// Further decode the version strings
rc = decode_downstream_device_parameter_table_entry_versions(
&versions, &entry_version.entry, entry_version.active_comp_ver_str,
- entry_version.pending_comp_ver_str);
+ sizeof(entry_version.active_comp_ver_str),
+ entry_version.pending_comp_ver_str,
+ sizeof(entry_version.pending_comp_ver_str));
struct pldm_downstream_device_parameter_entry entry = entry_version.entry;
EXPECT_EQ(rc, 0);
@@ -2049,7 +2051,9 @@
int rc = decode_downstream_device_parameter_table_entry_versions(
&versions, &entryVersion.entry, entryVersion.active_comp_ver_str,
- entryVersion.pending_comp_ver_str);
+ sizeof(entryVersion.active_comp_ver_str),
+ entryVersion.pending_comp_ver_str,
+ sizeof(entryVersion.pending_comp_ver_str));
EXPECT_EQ(rc, 0);
EXPECT_EQ(0, memcmp(entryVersion.active_comp_ver_str, versions.ptr,
@@ -2086,7 +2090,9 @@
int rc = decode_downstream_device_parameter_table_entry_versions(
&versions, nullptr, entryVersion.active_comp_ver_str,
- entryVersion.pending_comp_ver_str);
+ sizeof(entryVersion.active_comp_ver_str),
+ entryVersion.pending_comp_ver_str,
+ sizeof(entryVersion.pending_comp_ver_str));
EXPECT_EQ(rc, -EINVAL);
}
#endif
@@ -2113,7 +2119,9 @@
EXPECT_EQ(decode_downstream_device_parameter_table_entry_versions(
&versions, &entryVersion.entry,
entryVersion.active_comp_ver_str,
- entryVersion.pending_comp_ver_str),
+ sizeof(entryVersion.active_comp_ver_str),
+ entryVersion.pending_comp_ver_str,
+ sizeof(entryVersion.pending_comp_ver_str)),
-EOVERFLOW);
}
#endif