From: Magdalena Schulfer <[email protected]>
The tracing log headers parsed by fw_log_print_buffer() reside in
DMA-shared BOs that the NPU firmware can write to.
fw_log_from_bo() validated log->header_size and log->size, but
fw_log_print_buffer() re-read those same fields from shared memory
afterwards, allowing a TOCTOU where firmware changes them between the
check and the use, and making the host dereference out-of-bounds
addresses while printing logs.
Snapshot the validated values once with READ_ONCE() and pass them down
explicitly in a new struct ivpu_fw_log_desc instead of re-reading them
from the shared struct.
Cc: [email protected]
Fixes: d4e4257afa6e ("accel/ivpu: Add firmware tracing support")
Signed-off-by: Magdalena Schulfer <[email protected]>
Signed-off-by: Dawid Osuchowski <[email protected]>
---
drivers/accel/ivpu/ivpu_fw_log.c | 76 +++++++++++++++++++-------------
1 file changed, 45 insertions(+), 31 deletions(-)
diff --git a/drivers/accel/ivpu/ivpu_fw_log.c b/drivers/accel/ivpu/ivpu_fw_log.c
index 716467aa3156..4f9055aa9d33 100644
--- a/drivers/accel/ivpu/ivpu_fw_log.c
+++ b/drivers/accel/ivpu/ivpu_fw_log.c
@@ -26,10 +26,17 @@ MODULE_PARM_DESC(fw_log_level,
" error=" __stringify(IVPU_FW_LOG_ERROR)
" fatal=" __stringify(IVPU_FW_LOG_FATAL));
+struct ivpu_fw_log_desc {
+ struct vpu_tracing_buffer_header *log;
+ u32 header_size;
+ u32 size;
+};
+
static int fw_log_from_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, u32
*offset,
- struct vpu_tracing_buffer_header **out_log)
+ struct ivpu_fw_log_desc *desc)
{
struct vpu_tracing_buffer_header *log;
+ u32 header_size, size;
if ((*offset + sizeof(*log)) > ivpu_bo_size(bo))
return -EINVAL;
@@ -39,26 +46,32 @@ static int fw_log_from_bo(struct ivpu_device *vdev, struct
ivpu_bo *bo, u32 *off
if (log->vpu_canary_start != VPU_TRACING_BUFFER_CANARY)
return -EINVAL;
- if (log->header_size < sizeof(*log) || log->header_size > 1024) {
- ivpu_dbg(vdev, FW_BOOT, "Invalid header size 0x%x\n",
log->header_size);
+ header_size = READ_ONCE(log->header_size);
+ size = READ_ONCE(log->size);
+
+ if (header_size < sizeof(*log) || header_size > 1024) {
+ ivpu_dbg(vdev, FW_BOOT, "Invalid header size 0x%x\n",
header_size);
return -EINVAL;
}
- if (log->size < log->header_size) {
- ivpu_dbg(vdev, FW_BOOT, "Invalid log size 0x%x\n", log->size);
+ if ((char *)log + size > (char *)ivpu_bo_vaddr(bo) + ivpu_bo_size(bo)) {
+ ivpu_dbg(vdev, FW_BOOT, "Invalid log size 0x%x\n", size);
return -EINVAL;
}
- if ((char *)log + log->size > (char *)ivpu_bo_vaddr(bo) +
ivpu_bo_size(bo)) {
- ivpu_dbg(vdev, FW_BOOT, "Invalid log size 0x%x\n", log->size);
+ if (size < header_size) {
+ ivpu_dbg(vdev, FW_BOOT, "Invalid log size 0x%x < header size
0x%x\n",
+ size, header_size);
return -EINVAL;
}
- *out_log = log;
- *offset += log->size;
+ desc->log = log;
+ desc->header_size = header_size;
+ desc->size = size;
+ *offset += size;
ivpu_dbg(vdev, FW_BOOT,
"FW log name \"%s\", write offset 0x%x size 0x%x, wrap count %d,
hdr version %d size %d format %d, alignment %d",
- log->name, log->write_index, log->size, log->wrap_count,
log->header_version,
- log->header_size, log->format, log->alignment);
+ log->name, log->write_index, size, log->wrap_count,
log->header_version,
+ header_size, log->format, log->alignment);
return 0;
}
@@ -94,11 +107,12 @@ static void fw_log_print_lines(char *buffer, u32 size,
struct drm_printer *p)
drm_printf(p, "%s", line);
}
-static void fw_log_print_buffer(struct vpu_tracing_buffer_header *log, const char *prefix,
+static void fw_log_print_buffer(struct ivpu_fw_log_desc *desc, const char
*prefix,
bool only_new_msgs, struct drm_printer *p)
{
- char *log_data = (void *)log + log->header_size;
- u32 data_size = log->size - log->header_size;
+ struct vpu_tracing_buffer_header *log = desc->log;
+ char *log_data = (void *)log + desc->header_size;
+ u32 data_size = desc->size - desc->header_size;
u32 log_start = only_new_msgs ? READ_ONCE(log->read_index) : 0;
u32 log_end = READ_ONCE(log->write_index);
@@ -134,11 +148,11 @@ static void
fw_log_print_all_in_bo(struct ivpu_device *vdev, const char *name,
struct ivpu_bo *bo, bool only_new_msgs, struct
drm_printer *p)
{
- struct vpu_tracing_buffer_header *log;
+ struct ivpu_fw_log_desc desc;
u32 next = 0;
- while (fw_log_from_bo(vdev, bo, &next, &log) == 0)
- fw_log_print_buffer(log, name, only_new_msgs, p);
+ while (fw_log_from_bo(vdev, bo, &next, &desc) == 0)
+ fw_log_print_buffer(&desc, name, only_new_msgs, p);
}
void ivpu_fw_log_print(struct ivpu_device *vdev, bool only_new_msgs, struct drm_printer *p)
@@ -149,36 +163,36 @@ void ivpu_fw_log_print(struct ivpu_device *vdev, bool
only_new_msgs, struct drm_
void ivpu_fw_log_mark_read(struct ivpu_device *vdev)
{
- struct vpu_tracing_buffer_header *log;
+ struct ivpu_fw_log_desc desc;
u32 next;
next = 0;
- while (fw_log_from_bo(vdev, vdev->fw->mem_log_crit, &next, &log) == 0) {
- log->read_index = READ_ONCE(log->write_index);
- log->read_wrap_count = READ_ONCE(log->wrap_count);
+ while (fw_log_from_bo(vdev, vdev->fw->mem_log_crit, &next, &desc) == 0)
{
+ desc.log->read_index = READ_ONCE(desc.log->write_index);
+ desc.log->read_wrap_count = READ_ONCE(desc.log->wrap_count);
}
next = 0;
- while (fw_log_from_bo(vdev, vdev->fw->mem_log_verb, &next, &log) == 0) {
- log->read_index = READ_ONCE(log->write_index);
- log->read_wrap_count = READ_ONCE(log->wrap_count);
+ while (fw_log_from_bo(vdev, vdev->fw->mem_log_verb, &next, &desc) == 0)
{
+ desc.log->read_index = READ_ONCE(desc.log->write_index);
+ desc.log->read_wrap_count = READ_ONCE(desc.log->wrap_count);
}
}
void ivpu_fw_log_reset(struct ivpu_device *vdev)
{
- struct vpu_tracing_buffer_header *log;
+ struct ivpu_fw_log_desc desc;
u32 next;
next = 0;
- while (fw_log_from_bo(vdev, vdev->fw->mem_log_crit, &next, &log) == 0) {
- log->read_index = 0;
- log->read_wrap_count = 0;
+ while (fw_log_from_bo(vdev, vdev->fw->mem_log_crit, &next, &desc) == 0)
{
+ desc.log->read_index = 0;
+ desc.log->read_wrap_count = 0;
}
next = 0;
- while (fw_log_from_bo(vdev, vdev->fw->mem_log_verb, &next, &log) == 0) {
- log->read_index = 0;
- log->read_wrap_count = 0;
+ while (fw_log_from_bo(vdev, vdev->fw->mem_log_verb, &next, &desc) == 0)
{
+ desc.log->read_index = 0;
+ desc.log->read_wrap_count = 0;
}
}
