Re: [PATCH] drm/panthor: validate firmware interface structure sizes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in firmware interface initialization allows kernel memory corruption.
- [High] Integer overflow and incorrect structure size in firmware bounds checks on 32-bit architectures.
- [High] NULL pointer dereference when firmware provides a 0-sized shared section.
--

--- Patch [1]: [PATCH] drm/panthor: validate firmware interface structure sizes ---
commit a126d5ded05975c89ebf69bcb6a208082b1f032d
Author: Osama Abdelkader <osama.abdelkader@gmail.com>

drm/panthor: validate firmware interface structure sizes

iface_fw_to_cpu_addr() only checks that the firmware-provided MCU virtual
address points inside the shared section. The returned pointer is later
used as a full firmware interface structure, so accepting an address near
the end of the shared section can still lead to out-of-bounds accesses.

Pass the expected object size to iface_fw_to_cpu_addr() and reject ranges
that do not fit entirely in the shared section.

> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 90f59d782a809..1697305c7b0e9 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c

[ ... ]

> @@ -890,8 +896,10 @@ static int panthor_init_cs_iface(struct panthor_device *ptdev,
>  
>  	spin_lock_init(&cs_iface->lock);

[Severity: High]
This is a pre-existing issue, but could the bounds check earlier in
panthor_init_cs_iface() be vulnerable to a 32-bit integer overflow?

panthor_fw.c:panthor_init_cs_iface() {
    ...
    if (iface_offset + sizeof(*cs_iface) >= shared_section_sz)
        return -EINVAL;
    ...
}

If a malicious firmware provides crafted strides, could iface_offset wrap
around near U32_MAX on 32-bit architectures, allowing it to bypass this
check while still extending beyond shared_section_sz?

Also, does this check incorrectly use the size of the host wrapper struct
sizeof(*cs_iface) instead of the actual firmware struct size, allowing
an invalid iface_offset to pass the check if the host struct is smaller?

>  	cs_iface->control = ptdev->fw->shared_section->mem->kmap + iface_offset;
> -	cs_iface->input = iface_fw_to_cpu_addr(ptdev, cs_iface->control->input_va);
> -	cs_iface->output = iface_fw_to_cpu_addr(ptdev, cs_iface->control->output_va);
> +	cs_iface->input = iface_fw_to_cpu_addr(ptdev, cs_iface->control->input_va,
> +					       sizeof(*cs_iface->input));
> +	cs_iface->output = iface_fw_to_cpu_addr(ptdev, cs_iface->control->output_va,
> +						sizeof(*cs_iface->output));

[ ... ]

> @@ -941,8 +949,10 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
>  
>  	spin_lock_init(&csg_iface->lock);
>  	csg_iface->control = ptdev->fw->shared_section->mem->kmap + iface_offset;
> -	csg_iface->input = iface_fw_to_cpu_addr(ptdev, csg_iface->control->input_va);
> -	csg_iface->output = iface_fw_to_cpu_addr(ptdev, csg_iface->control->output_va);
> +	csg_iface->input = iface_fw_to_cpu_addr(ptdev, csg_iface->control->input_va,
> +						sizeof(*csg_iface->input));
> +	csg_iface->output = iface_fw_to_cpu_addr(ptdev, csg_iface->control->output_va,
> +						 sizeof(*csg_iface->output));
>  
>  	if (csg_iface->control->stream_num < MIN_CS_PER_CSG ||
>  	    csg_iface->control->stream_num > MAX_CS_PER_CSG)

[Severity: Critical]
This is a pre-existing issue, but is there a Time-of-Check to Time-of-Use
(TOCTOU) vulnerability here and in panthor_fw_init_ifaces()?

Since panthor_fw_start() starts the GPU MCU before panthor_fw_init_ifaces()
is called, could compromised GPU firmware mutate stream_num (or group_num)
directly in shared memory after this bounds check?

If the compiler reloads the limit from memory on each iteration of the
subsequent loop:

    for (i = 0; i < csg_iface->control->stream_num; i++) {
        int ret = panthor_init_cs_iface(ptdev, csg_idx, i);
        ...
    }

could the loop overrun and index out of bounds, overwriting adjacent kernel
structures?

[ ... ]

> @@ -999,8 +1009,10 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev)
>  		return -EINVAL;
>  	}
>  
> -	glb_iface->input = iface_fw_to_cpu_addr(ptdev, glb_iface->control->input_va);
> -	glb_iface->output = iface_fw_to_cpu_addr(ptdev, glb_iface->control->output_va);

[Severity: High]
This isn't a bug introduced by this patch, but earlier in this function
could we dereference a NULL pointer if the firmware provides a 0-sized
shared section?

In panthor_fw_load_section_entry(), if the firmware provides a
CSF_MCU_SHARED_REGION_START section header where hdr.va.end == hdr.va.start,
the memory allocation is skipped:

panthor_fw.c:panthor_fw_load_section_entry() {
    ...
    section_size = hdr.va.end - hdr.va.start;
    if (section_size) {
        ...
    }
    ptdev->fw->shared_section = section;
    ...
}

This leaves section->mem as NULL.  When panthor_fw_init_ifaces() runs,
it checks:

    if (!ptdev->fw->shared_section->mem->kmap)

Will this unconditionally dereference mem when it is NULL, causing a
kernel panic during device probe?

> +	glb_iface->input = iface_fw_to_cpu_addr(ptdev, glb_iface->control->input_va,
> +						sizeof(*glb_iface->input));
> +	glb_iface->output = iface_fw_to_cpu_addr(ptdev, glb_iface->control->output_va,
> +						 sizeof(*glb_iface->output));
>  	if (!glb_iface->input || !glb_iface->output) {
>  		drm_err(&ptdev->base, "Invalid global control interface input/output VA");
>  		return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720114918.15973-1-osama.abdelkader@gmail.com?part=1