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 <[email protected]> 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/[email protected]?part=1
