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 <[email protected]> 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/[email protected]?part=2
