Re: [PATCH v3] drm/amd/amdgpu: Prevent null pointer dereference in GPU bandwidth calculation

["Christian König" <christian.koenig@amd.com>]

Am 20.01.25 um 14:31 schrieb Srinivasan Shanmugam:
> If the parent is NULL, adev->pdev is used to retrieve the PCIe speed and
> width, ensuring that  the function can still determine these
> capabilities from the device itself.
>
> Fixes the below:
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:6193 amdgpu_device_gpu_bandwidth()
> 	error: we previously assumed 'parent' could be null (see line 6180)
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>      6170 static void amdgpu_device_gpu_bandwidth(struct amdgpu_device *adev,
>      6171                                         enum pci_bus_speed *speed,
>      6172                                         enum pcie_link_width *width)
>      6173 {
>      6174         struct pci_dev *parent = adev->pdev;
>      6175
>      6176         if (!speed || !width)
>      6177                 return;
>      6178
>      6179         parent = pci_upstream_bridge(parent);
>      6180         if (parent && parent->vendor == PCI_VENDOR_ID_ATI) {
>                       ^^^^^^
> If parent is NULL
>
>      6181                 /* use the upstream/downstream switches internal to dGPU */
>      6182                 *speed = pcie_get_speed_cap(parent);
>      6183                 *width = pcie_get_width_cap(parent);
>      6184                 while ((parent = pci_upstream_bridge(parent))) {
>      6185                         if (parent->vendor == PCI_VENDOR_ID_ATI) {
>      6186                                 /* use the upstream/downstream switches internal to dGPU */
>      6187                                 *speed = pcie_get_speed_cap(parent);
>      6188                                 *width = pcie_get_width_cap(parent);
>      6189                         }
>      6190                 }
>      6191         } else {
>      6192                 /* use the device itself */
> --> 6193                 *speed = pcie_get_speed_cap(parent);
>                                                       ^^^^^^ Then we are toasted here.
>
>      6194                 *width = pcie_get_width_cap(parent);
>      6195         }
>      6196 }
>
> Fixes: 9e424a5d9087 ("drm/amdgpu: cache gpu pcie link width")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> Suggested-by: Lijo Lazar <lijo.lazar@amd.com>
> ---
> v3: change the else s/parent/adev->pdev (Lijo)
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 46af07faf8c8..8ed7f2f8546d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -6174,7 +6174,7 @@ static void amdgpu_device_gpu_bandwidth(struct amdgpu_device *adev,
>   		return;
>   
>   	parent = pci_upstream_bridge(parent);
> -	if (parent && parent->vendor == PCI_VENDOR_ID_ATI) {
> +	if (parent->vendor == PCI_VENDOR_ID_ATI) {

That will now crash when parent is NULL :)

Christian.

>   		/* use the upstream/downstream switches internal to dGPU */
>   		*speed = pcie_get_speed_cap(parent);
>   		*width = pcie_get_width_cap(parent);
> @@ -6187,8 +6187,8 @@ static void amdgpu_device_gpu_bandwidth(struct amdgpu_device *adev,
>   		}
>   	} else {
>   		/* use the device itself */
> -		*speed = pcie_get_speed_cap(parent);
> -		*width = pcie_get_width_cap(parent);
> +		*speed = pcie_get_speed_cap(adev->pdev);
> +		*width = pcie_get_width_cap(adev->pdev);
>   	}
>   }
>