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

[Christian König]

Am 20.01.25 um 13:32 schrieb Srinivasan Shanmugam:

This commit adds an early return if no upstream bridge is found, setting
the speed and width to PCI_SPEED_UNKNOWN and PCIE_LNK_WIDTH_UNKNOWN,
respectively. This ensures that the function handles the absence of an
upstream bridge gracefully.

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.koe...@amd.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Reported-by: Dan Carpenter <dan.carpen...@linaro.org>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmu...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 46af07faf8c8..749791a486bd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -6174,6 +6174,14 @@ static void amdgpu_device_gpu_bandwidth(struct 
amdgpu_device *adev,
                return;

parent = pci_upstream_bridge(parent);

+
+       if (!parent) {
+               /*Return early if no upstream bridge is found */
+               *speed = PCI_SPEED_UNKNOWN;
+               *width = PCIE_LNK_WIDTH_UNKNOWN;
+               return;
+       }
+
        if (parent && parent->vendor == PCI_VENDOR_ID_ATI) {

You can then also remove this check for parent here.

Regards,
Christian.

                /* use the upstream/downstream switches internal to dGPU */
                *speed = pcie_get_speed_cap(parent);