Re: [PATCH AUTOSEL 6.18-6.12] drm/amdgpu: Guard reads in pcie state readout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/amdgpu: uninitialized pcie_regs fields in aqua_vanjaram_read_pcie_state()
--

commit c0c47257f13eed1e55050aec0ca9d8906dd7cf73
Author: Lijo Lazar <lijo.lazar@amd.com>

drm/amdgpu: Guard reads in pcie state readout

This commit introduces a helper to verify and retrieve the GPU's internal
upstream switch port and guards the upstream port PCIe configuration reads in
aqua_vanjaram_read_pcie_state() to avoid a NULL pointer dereference in
passthrough environments.

> diff --git a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
> index f9e2edf5260bc..4529627d13c5d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
> +++ b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c

[ ... ]

> @@ -650,25 +673,27 @@ static ssize_t aqua_vanjaram_read_pcie_state(struct amdgpu_device *adev,
>  		}
>  	}
>  
> -	ds_pdev = pci_upstream_bridge(adev->pdev);
> -	us_pdev = pci_upstream_bridge(ds_pdev);
> +	us_pdev = aqua_vanjaram_get_us_pdev(adev);
> +	if (us_pdev) {
> +		pcie_capability_read_word(us_pdev, PCI_EXP_DEVSTA,
> +					  &pcie_regs->device_status);
> +		pcie_capability_read_word(us_pdev, PCI_EXP_LNKSTA,
> +					  &pcie_regs->link_status);
> +
> +		aer_cap = pci_find_ext_capability(us_pdev, PCI_EXT_CAP_ID_ERR);
> +		if (aer_cap) {
> +			pci_read_config_dword(us_pdev,
> +					      aer_cap + PCI_ERR_COR_STATUS,
> +					      &pcie_regs->pcie_corr_err_status);
> +			pci_read_config_dword(us_pdev,
> +					      aer_cap + PCI_ERR_UNCOR_STATUS,
> +					      &pcie_regs->pcie_uncorr_err_status);
> +		}
>  
> -	pcie_capability_read_word(us_pdev, PCI_EXP_DEVSTA,
> -				  &pcie_regs->device_status);
> -	pcie_capability_read_word(us_pdev, PCI_EXP_LNKSTA,
> -				  &pcie_regs->link_status);
> -
> -	aer_cap = pci_find_ext_capability(us_pdev, PCI_EXT_CAP_ID_ERR);
> -	if (aer_cap) {
> -		pci_read_config_dword(us_pdev, aer_cap + PCI_ERR_COR_STATUS,
> -				      &pcie_regs->pcie_corr_err_status);
> -		pci_read_config_dword(us_pdev, aer_cap + PCI_ERR_UNCOR_STATUS,
> -				      &pcie_regs->pcie_uncorr_err_status);
> +		pci_read_config_dword(us_pdev, PCI_PRIMARY_BUS,
> +				      &pcie_regs->sub_bus_number_latency);
>  	}

[Severity: High]
When aqua_vanjaram_get_us_pdev(adev) returns NULL (or when aer_cap is 0), do
these skipped fields in pcie_regs leak uninitialized kernel heap memory to
userspace?

When userspace reads the world-readable (0444) reg_state sysfs binary
attribute at offset AMDGPU_SYS_REG_STATE_PCIE, the buffer is allocated with
kmalloc() without __GFP_ZERO and passed down without being zeroed:

kernfs_file_read_iter()
  buf = kmalloc(len, GFP_KERNEL);
  sysfs_kf_bin_read()
    amdgpu_sysfs_reg_state_get()
      amdgpu_asic_get_reg_state()
        aqua_vanjaram_get_reg_state()
          aqua_vanjaram_read_pcie_state()

Previously, pcie_regs->device_status, pcie_regs->link_status, and
pcie_regs->sub_bus_number_latency were unconditionally written. With this
change, when us_pdev is NULL, those fields along with
pcie_regs->pcie_corr_err_status and pcie_regs->pcie_uncorr_err_status are
left completely uninitialized in buf while pcie_regs->inst_header.state is
set to AMDGPU_INST_S_OK, and kernfs_file_read_iter() copies szbuf bytes back
to userspace via copy_to_iter().

Should these fields (or pcie_regs) be zero-initialized before populating the
state?

>  
> -	pci_read_config_dword(us_pdev, PCI_PRIMARY_BUS,
> -			      &pcie_regs->sub_bus_number_latency);
> -
>  	pcie_reg_state->common_header.structure_size = szbuf;
>  	pcie_reg_state->common_header.format_revision = 1;
>  	pcie_reg_state->common_header.content_revision = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-466-sashal@kernel.org?part=1