Re: [PATCH v10 01/10] drm/xe/uc_fw: Allow re-initializing firmware

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

New issues:
- [High] xe_uc_fw_reinit() lacks an IS_SRIOV_VF() check, causing VF firmware state corruption and potential NULL pointer dereference.

Pre-existing issues:
- [High] Integer overflow in parse_gsc_layout() leads to out-of-bounds read and potential kernel panic.
--

--- Patch [1]: [PATCH v10 01/10] drm/xe/uc_fw: Allow re-initializing firmware ---
commit 0789aa0c08f22f9ec32c3de1bbde4b80f0984ba2
Author: Raag Jadav <[email protected]>

drm/xe/uc_fw: Allow re-initializing firmware

This commit introduces xe_uc_fw_reinit() to support re-initializing
firmware without reloading the driver. It copies the firmware back
to its buffer object and marks it as loadable after events like
PCIe FLR.

> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
[ ... ]
> +/**
> + * xe_uc_fw_reinit() - Re-initialize uC firmware into its bo
> + * @uc_fw: uC firmware
> + *
> + * Returns: 0 on success, negative error code otherwise.
> + */
> +int xe_uc_fw_reinit(struct xe_uc_fw *uc_fw)
> +{
> +	struct xe_device *xe = uc_fw_to_xe(uc_fw);
> +	struct xe_uc_fw_version old_fw, new_fw;
> +	const struct firmware *fw = NULL;
> +	int err;
> +
> +	/* Make sure the status was cleared the last time we reset the uc */
> +	xe_assert(xe, !xe_uc_fw_is_loaded(uc_fw));
> +
> +	/* We shouldn't be here for the firmware which wasn't available */
> +	if (!xe_uc_fw_is_available(uc_fw))
> +		return -ENOEXEC;

[Severity: High]
Does this code safely handle SR-IOV Virtual Functions (VFs)?

On VFs, firmware loading is handled by the PF, so the VF initializes its
firmware state with the dummy path "Loaded by PF" and skips allocating
uc_fw->bo. Since the VF reaches the XE_UC_FIRMWARE_PRELOADED status, the
xe_uc_fw_is_available() check above will pass.

Because there is no IS_SRIOV_VF() check here, this proceeds to request
the dummy firmware path. If that request fails, the VF's status is
incorrectly overwritten to XE_UC_FIRMWARE_INIT_FAIL.

If a file named "Loaded by PF" actually exists, the code proceeds to call
uc_fw_reinit():

drivers/gpu/drm/xe/xe_uc_fw.c:uc_fw_reinit() {
    ...
    xe_map_memcpy_to(xe, &uc_fw->bo->vmap, 0, data, uc_fw->size);
    ...
}

Since uc_fw->bo is NULL on VFs, will this lead to a NULL pointer
dereference?

> +
> +	old_fw = uc_fw->versions.found[XE_UC_FW_VER_RELEASE];
> +
> +	err = firmware_request_nowarn(&fw, uc_fw->path, xe->drm.dev);
> +	if (err)
> +		goto init_fail;
> +
> +	err = parse_headers(uc_fw, fw);

[Severity: High]
This isn't a bug introduced by this patch, but could the firmware parsing
called here be vulnerable to an integer overflow?

Inside parse_headers(), when handling GSC firmware, it calls
parse_gsc_layout() which performs the following bounds check:

drivers/gpu/drm/xe/xe_uc_fw.c:parse_gsc_layout() {
    ...
    min_size = layout->boot1.offset + layout->boot1.size;
    if (size < min_size) {
        ...
        return -ENODATA;
    }
    ...
    bpdt_header = data + layout->boot1.offset;
    if (bpdt_header->signature != GSC_BPDT_HEADER_SIGNATURE) {
    ...
}

Since both layout->boot1.offset and layout->boot1.size are 32-bit
unsigned integers, can a crafted firmware cause their sum to wrap around?

If they wrap before being assigned to the 64-bit min_size, the bounds
check would be bypassed, potentially resulting in an out-of-bounds read
when dereferencing bpdt_header->signature.

> +	if (err)
> +		goto restore_old;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1