Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The validation of `log->size` and `log->header_size` is vulnerable to a Time-Of-Check to Time-Of-Use (TOCTOU) race condition because the fields are read multiple times directly from firmware-shared BO memory, allowing malicious firmware to bypass bounds checks. - [Medium] The firmware-controlled `log->name` character array is printed using an unbounded `%s` format specifier, which can lead to out-of-bounds reads if the device does not null-terminate the string. -- --- Patch [1]: [PATCH] accel/ivpu: Reject firmware log with size smaller than header --- commit 8efc93a32eb39ab04f9058367f2b3c6355d62590 Author: Jhonraushan <[email protected]> accel/ivpu: Reject firmware log with size smaller than header This commit adds a check to fw_log_from_bo() to reject firmware logs where the log size is smaller than the header size. This is intended to prevent an integer underflow when calculating the data size in fw_log_print_buffer(), which could defeat bounds checks. > diff --git a/drivers/accel/ivpu/ivpu_fw_log.c > b/drivers/accel/ivpu/ivpu_fw_log.c > index 275baf844b562..716467aa31567 100644 > --- a/drivers/accel/ivpu/ivpu_fw_log.c > +++ b/drivers/accel/ivpu/ivpu_fw_log.c > @@ -43,6 +43,10 @@ static int fw_log_from_bo(struct ivpu_device *vdev, struct > ivpu_bo *bo, u32 *off > ivpu_dbg(vdev, FW_BOOT, "Invalid header size 0x%x\n", > log->header_size); > return -EINVAL; > } > + if (log->size < log->header_size) { > + ivpu_dbg(vdev, FW_BOOT, "Invalid log size 0x%x\n", log->size); > + return -EINVAL; > + } [Severity: High] This is a pre-existing issue, but since the log pointer accesses BO memory shared directly with the firmware, could a malicious firmware alter log->size or log->header_size after these checks are performed? For example, in fw_log_from_bo() the offset is later updated: *offset += log->size; And in fw_log_print_buffer() data_size is calculated: u32 data_size = log->size - log->header_size; If the firmware changes the values between the validation here and these subsequent reads, could this bypass the bounds checks and lead to the very same underflow or infinite loop this patch aims to fix? Would it be safer to read these values once into local variables? [Severity: Medium] This is also a pre-existing issue, but I noticed that later in fw_log_from_bo(), the log->name array is printed using the %s format: 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); Since log->name is a 16-byte character array in the shared firmware BO memory, what happens if the firmware fills it without a null terminator? Could the %s format specifier read past the array bounds and leak memory or cause a page fault? This similarly occurs in fw_log_print_buffer() when using drm_printf() with %s on log->name. > 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); > return -EINVAL; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
