Re: [PATCH] accel/ivpu: Reject firmware log with size smaller than header

[sashiko-bot@kernel.org]

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 <raushan.jhon@gmail.com>

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/20260715074206.867712-1-raushan.jhon@gmail.com?part=1