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

["Wachowski, Karol" <karol.wachowski@linux.intel.com>]

On 01-Sep-26 14:57, Dawid Osuchowski wrote:
> From: Magdalena Schulfer <magdalena.schulfer@intel.com>
> 
> 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: stable@vger.kernel.org
> Fixes: d4e4257afa6e ("accel/ivpu: Add firmware tracing support")
> Signed-off-by: Magdalena Schulfer <magdalena.schulfer@intel.com>
> Signed-off-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com>
> ---
>   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;
>   	}
>   }

Reviewed-by: Karol Wachowski <karol.wachowski@linux.intel.com>