Re: [PATCH 2/2] accel/ivpu: Validate firmware log buffer metadata

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsafe printing of firmware-controlled string without length bounds could lead to out-of-bounds read.
--

commit fd0e38bea76a0e3e0f409139f3cf70f7ae5a79ea
Author: Magdalena Schulfer <magdalena.schulfer@intel.com>

accel/ivpu: Validate firmware log buffer metadata

The tracing log headers parsed by fw_log_print_buffer() reside in
DMA-shared BOs that the NPU firmware can write to. This commit mitigates
a TOCTOU vulnerability by snapshotting the header size and size fields
before validating them.

> diff --git a/drivers/accel/ivpu/ivpu_fw_log.c b/drivers/accel/ivpu/ivpu_fw_log.c
> index 716467aa31567..4f9055aa9d33f 100644
> --- a/drivers/accel/ivpu/ivpu_fw_log.c
> +++ b/drivers/accel/ivpu/ivpu_fw_log.c
[ ... ]
> @@ -39,26 +46,32 @@ static int fw_log_from_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, u32 *off
[ ... ]
> -	*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);

[Severity: High]
This isn't a bug introduced by this patch, but since this commit aims to
secure the untrusted log header, is it safe to print log->name using %s?

The name array (16 bytes) in struct vpu_tracing_buffer_header is controlled by
the firmware. If the firmware fails to null-terminate it, printing it with %s
in ivpu_dbg() will cause an out-of-bounds read. This could leak adjacent
memory into the kernel log or trigger a panic if the read crosses into
an unmapped page.

Could we bound the length by using %.16s instead?

A similar issue also exists in fw_log_print_buffer(), where log->name is
repeatedly passed to drm_printf() using %s without length limits.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827102339.281799-1-dawid.osuchowski@linux.intel.com?part=2