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 <[email protected]> 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/[email protected]?part=1
