Re: [PATCH v2 2/5] drm/nouveau/gsp/r535: Add support for MEMSYS_GET_STATIC_CONFIG

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/nouveau/gsp: read of uninitialized memory in r535_gsp_get_static_memsys_info
- [Medium] drm/nouveau/gsp: r535_gsp_get_static_memsys_info is never called for R535 initialization
--

commit 2fa837b461b49fefa7883b5628782713aaf7bd67
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r535: Add support for MEMSYS_GET_STATIC_CONFIG
    
This is a GSP structure describing various characteristics of the memory
management system that GSP provides. Start by fetching it during driver
load, but don't do anything with the information we get from it just yet.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> index 09b17b85dc1ac..c57a3250d6b96 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
[ ... ]
> @@ -1808,6 +1808,23 @@ r535_gsp_fini(struct nvkm_gsp *gsp, enum nvkm_suspend_state suspend)
>  	return 0;
>  }
>  
> +int
> +r535_gsp_get_static_memsys_info(struct nvkm_gsp *gsp)
> +{
> +	NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS *ctrl;
> +
> +	ctrl = nvkm_gsp_rm_ctrl_get(&gsp->internal.device.subdevice,
> +				    NV2080_CTRL_CMD_INTERNAL_MEMSYS_GET_STATIC_CONFIG,
> +				    sizeof(*ctrl));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	gsp->memsys.use_raw_mode_comptagline_alloc = ctrl->bUseRawModeComptaglineAllocation;

[Severity: High]
Does this read from uninitialized memory?

The nvkm_gsp_rm_ctrl_get() function only allocates the RPC payload buffer,
but it doesn't execute the command. This reads from the buffer without pushing
it to the firmware (e.g. using nvkm_gsp_rm_ctrl_push() or nvkm_gsp_rm_ctrl_rd()).

Without executing the command, does it just read the uninitialized or zeroed
buffer memory?

> +
> +	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, ctrl);
> +	return 0;
> +}
> +
[ ... ]
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> index b45781cd0dfdc..5a51fe7e5ab98 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> @@ -137,6 +137,14 @@ r570_gsp_get_static_info(struct nvkm_gsp *gsp)
>  		}
>  	}
>  
> +	ret = r535_gsp_get_static_memsys_info(gsp);
> +	if (ret) {
> +		nvkm_error(&gsp->subdev, "Retrieving static memsys info failed\n");
> +		return ret;
> +	}
> +	nvkm_debug(&gsp->subdev, "memsys: Use raw mode for comptag allocations? %s\n",
> +		   str_yes_no(gsp->memsys.use_raw_mode_comptagline_alloc));
> +

[Severity: Medium]
Is r535_gsp_get_static_memsys_info() missing a call in the r535 initialization?

This adds the call to r570_gsp_get_static_info() here, but I don't see a
similar call added to r535_gsp_get_static_info() in r535/gsp.c, even though
the commit message mentions adding support for r535.

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915210647.695187-1-lyude@redhat.com?part=2