Re: [PATCH 5/6] nouveau/gsp: add some more fields to r570 system info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The BAR index calculation for NVKM_BAR3_IO lacks bounds checking, leading to PCI ROM resource aliasing if all preceding BARs are 64-bit.
--

--- Patch [5]: [PATCH 5/6] nouveau/gsp: add some more fields to r570 system info ---
commit 18c8db5e682be4972a386c192c00409e01054607
Author: Dave Airlie <airlied@redhat.com>

nouveau/gsp: add some more fields to r570 system info

This commit adds the pci io bar, link cap and os page size to the r570
system info.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/pci.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/pci.c
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/pci.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/pci.c
[ ... ]
> @@ -1576,6 +1576,10 @@ nvkm_device_pci_resource_idx(struct nvkm_device_pci *pdev, enum nvkm_bar_id bar)
>  	if (bar == NVKM_BAR2_INST)
>  		return idx;
>  
> +	idx += (pci_resource_flags(pdev->pdev, idx) & IORESOURCE_MEM_64) ? 2 : 1;
> +	if (bar == NVKM_BAR3_IO)
> +		return idx;

[Severity: Medium]
Does this calculation lack a bounds check against PCI_STD_NUM_BARS?

If a GPU is configured with three 64-bit BARs (BAR0, BAR1, and BAR2), the
calculation idx += 2 will occur three times. This results in idx becoming 6.

Since index 6 corresponds to PCI_ROM_RESOURCE, could this cause the IO BAR
lookup to accidentally alias with the PCI Expansion ROM resource?

> +
>  	WARN_ON(1);
>  	return -1;
>  }
[ ... ]
> 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
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
[ ... ]
> @@ -165,6 +165,7 @@ r570_gsp_set_system_info(struct nvkm_gsp *gsp)
>  	info->gpuPhysAddr = device->func->resource_addr(device, NVKM_BAR0_PRI);
>  	info->gpuPhysFbAddr = device->func->resource_addr(device, NVKM_BAR1_FB);
>  	info->gpuPhysInstAddr = device->func->resource_addr(device, NVKM_BAR2_INST);
> +	info->gpuPhysIoAddr = device->func->resource_addr(device, NVKM_BAR3_IO);

[Severity: Medium]
If the above index calculation results in 6 due to having three 64-bit BARs,
this assignment will populate info->gpuPhysIoAddr with the physical address
of the PCI Expansion ROM instead of a non-existent IO BAR.

Could exposing this incorrect hardware layout cause undefined firmware
behavior or hardware faults if the GSP firmware attempts to access the
ROM address assuming it is an IO space?

>  	info->nvDomainBusDeviceFunc = pci_dev_id(pdev);
>  	info->maxUserVa = TASK_SIZE;
>  	info->pciConfigMirrorBase = device->pci->func->cfg.addr;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729041653.3985549-1-airlied@gmail.com?part=5