Re: [PATCH 6/7] drm/tegra: vic: Add Tegra264 support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Out-of-bounds read when parsing the RISC-V firmware descriptor.

Pre-existing issues:
- [High] Out-of-bounds read and potential crash when parsing internal offsets from the loaded firmware image.
- [High] Use-After-Free in error paths of `vic_load_firmware()` due to dangling pointer.
--

commit e93ea0b6d1fe675fec33102f35c8bdffae0a5687
Author: Mikko Perttunen <mperttunen@nvidia.com>

drm/tegra: vic: Add Tegra264 support

Add support for VIC on Tegra264. The Tegra264 VIC uses a RISC-V based
Falcon microcontroller instead of the traditional Falcon previously,
and has the TRANSCFG register in a different place.

> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
> --- a/drivers/gpu/drm/tegra/vic.c
> +++ b/drivers/gpu/drm/tegra/vic.c
[ ... ]
> @@ -85,31 +92,50 @@ static int vic_boot(struct vic *vic)
>  			CG_WAKEUP_DLY_CNT(4),
>  		   NV_PVIC_MISC_PRI_VIC_CG);
[ ... ]
> +	} else {
> +		u32 fce_ucode_size, fce_bin_data_offset;
> +		void *hdr;
> +
> +		hdr = vic->falcon.firmware.virt;
> +		fce_bin_data_offset = *(u32 *)(hdr + VIC_UCODE_FCE_DATA_OFFSET);
> +
> +		/* Old VIC firmware needs kernel help with setting up FCE microcode. */
> +		if (fce_bin_data_offset != 0x0 && fce_bin_data_offset != 0xa5a5a5a5) {
> +			hdr = vic->falcon.firmware.virt +
> +				*(u32 *)(hdr + VIC_UCODE_FCE_HEADER_OFFSET);
> +			fce_ucode_size = *(u32 *)(hdr + FCE_UCODE_SIZE_OFFSET);

[Severity: High]
This is a pre-existing issue, but does this code perform an out-of-bounds
read when parsing internal offsets from the loaded firmware image?

It appears that nested offsets are read directly from the firmware image
without verifying that the firmware size is large enough, or that the
offsets point within the valid firmware boundaries. If a malformed or
truncated firmware file is provided, could this arbitrary offset lead to an
out-of-bounds read and a kernel page fault?

> +
> +			falcon_execute_method(&vic->falcon, VIC_SET_FCE_UCODE_SIZE,
> +					      fce_ucode_size);
> +			falcon_execute_method(
> +				&vic->falcon, VIC_SET_FCE_UCODE_OFFSET,
> +				(vic->falcon.firmware.iova + fce_bin_data_offset) >> 8);
> +		}
[ ... ]
> @@ -277,6 +303,8 @@ static int vic_load_firmware(struct vic *vic)
>  
>  	if (!vic->config->supports_sid) {
>  		vic->can_use_context = false;
> +	} else if (vic->config->has_riscv) {
> +		vic->can_use_context = true;
>  	} else if (fce_bin_data_offset != 0x0 && fce_bin_data_offset != 0xa5a5a5a5) {
>  		/*
>  		 * Firmware will access FCE through STREAMID0, so context

[Severity: High]
This is a pre-existing issue, but can the error path in vic_load_firmware()
lead to a use-after-free?

If an error occurs during firmware loading, such as a failure in
falcon_load_firmware(), the cleanup path frees the allocated DMA memory
but seems to leave the firmware pointers dangling:

cleanup:
        if (!client->group)
                dma_free_coherent(vic->dev, size, virt, iova);
        else
                tegra_drm_free(tegra, size, virt, iova);

        mutex_unlock(&lock);
        return err;

If the device attempts to load the firmware again later, it might see
the non-null pointer, return success, and proceed to use the freed DMA
memory. Could this result in IOMMU faults and potential system crashes?

[ ... ]
> @@ -495,6 +537,7 @@ static int vic_probe(struct platform_device *pdev)
>  
>  	vic->falcon.dev = dev;
>  	vic->falcon.regs = vic->regs;
> +	vic->falcon.riscv = vic->config->has_riscv;
>  
>  	err = falcon_init(&vic->falcon);

[Severity: High]
Does enabling RISC-V support here expose an out-of-bounds read when
parsing the RISC-V firmware descriptor?

In falcon_parse_firmware_desc(), the descriptor data is cast to
struct falcon_fw_riscv_desc and fields at large offsets are read without
a preceding length check on the firmware size:

struct falcon_fw_riscv_desc *desc =
        (struct falcon_fw_riscv_desc *)falcon->firmware.desc_firmware->data;

falcon->firmware.code.offset = desc->code_offset;

If a truncated descriptor file is provided by userspace, could this
cause a buffer over-read and lead to a kernel page fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-t264-host1x-v1-0-8d934987de67@nvidia.com?part=6