Re: [PATCH] drm/nouveau/acr: fix missing nvkm_done() in error path of nvkm_acr_oneinit()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Incomplete fix of memory mapping reference leak. The patch adds one `nvkm_done(acr->wpr)` before returning `-EINVAL`, but `nvkm_kmap(acr->wpr)` was called twice on this code path.
- [Medium] The commit message demonstrates a misunderstanding of the control flow, stating that the function returns `-EINVAL` only when a mismatch is detected.
- [Medium] The return values of `acr->func->wpr_build()` and `acr->func->wpr_patch()` are ignored, allowing initialization to proceed with corrupted firmware data.
- [Medium] The WPR comparison loop can read out of bounds of the `acr->wpr` memory object and the firmware data blob.
--

commit 018071a64ea846382bbfa2aefceef09269c67376
Author: Wentao Liang <vulab@iscas.ac.cn>

drm/nouveau/acr: fix missing nvkm_done() in error path of nvkm_acr_oneinit()

This commit attempts to fix a memory mapping reference leak in
nvkm_acr_oneinit() by adding a missing nvkm_done() call in the error
path during WPR comparison.

>     However, when a mismatch is detected during the loop within the
>     conditional block, the function returns -EINVAL at line 318 without
>     calling nvkm_done(). This results in a leak of the kmap reference(s)
>     acquired earlier.

[Severity: Medium]
This isn't a bug introduced by this patch, but the commit message states the
function returns -EINVAL when a mismatch is detected.

Looking at nvkm_acr_oneinit(), the return statement is unconditional for the
entire block and happens regardless of whether a mismatch is detected:

	if (acr->wpr_fw && acr->wpr_comp) {
		nvkm_kmap(acr->wpr);
		for (i = 0; i < acr->wpr_fw->size; i += 4) {
			...
			if (fw != us) {
				nvkm_warn(...);
			}
		}
		nvkm_done(acr->wpr);
		return -EINVAL;
	}

Is this unconditional return intended?

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c
> index 4c7745cd6ae52..7fd967a2554f9 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c
> @@ -315,6 +315,7 @@ nvkm_acr_oneinit(struct nvkm_subdev *subdev)
>  					  i, us, fw);
>  			}
>  		}

[Severity: Medium]
This is a pre-existing issue, but could this loop read out-of-bounds of the
acr->wpr memory object and the firmware data blob?

If the user provides a firmware file larger than the dynamically calculated
wpr_size, there doesn't appear to be a check to ensure
acr->wpr_fw->size <= wpr_size before reading nvkm_ro32(acr->wpr, i).

Also, if acr->wpr_fw->size is not a multiple of 4, will the loop increment
cause the final iteration to read out-of-bounds?

[Severity: Medium]
This is a pre-existing issue, but does ignoring the return values from
wpr_build() and wpr_patch() earlier in this function allow initialization
to proceed with corrupted firmware data?

	if (!acr->wpr_fw || acr->wpr_comp)
		acr->func->wpr_build(acr, rtos);
	acr->func->wpr_patch(acr, (s64)acr->wpr_start - acr->wpr_prev);

If either function fails, could the system continue initializing with an
incomplete or corrupted WPR image?

> +		nvkm_done(acr->wpr);
>  		return -EINVAL;

[Severity: Medium]
This is a pre-existing issue, but does this error path still leak a mapping
reference?

In nvkm_acr_oneinit(), nvkm_kmap(acr->wpr) is called unconditionally before
the wpr_build() call:

	nvkm_kmap(acr->wpr);
	if (acr->wpr_fw && !acr->wpr_comp)
		nvkm_wobj(acr->wpr, 0, acr->wpr_fw->data, acr->wpr_fw->size);

It is then called a second time within this wpr_comp block.

Since the reference count is incremented twice, does it need to be
decremented twice by nvkm_done() before returning -EINVAL here?

>  	}
>  	nvkm_done(acr->wpr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606155606.77593-1-vulab@iscas.ac.cn?part=1