Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] accel/amdxdna: uninitialized read in aie4_psp_fw_variant() on SMN failure --
commit 8b85be20f84a0e02e0435160129ed37c6585968f Author: Mario Limonciello <[email protected]> accel/amdxdna: Load the aie4 firmware variant the PSP reports This commit reads PSP C2PMSG_63 over SMN to determine whether an AIE4 NPU requires default, alternative ("_l"), or preproduction ("_p") signed firmware, and adds MODULE_FIRMWARE() entries for the "_l" images. > diff --git a/drivers/accel/amdxdna/aie4_pci.c > b/drivers/accel/amdxdna/aie4_pci.c > index 02dcb2f813d45..fa8f99d1be361 100644 > --- a/drivers/accel/amdxdna/aie4_pci.c > +++ b/drivers/accel/amdxdna/aie4_pci.c [ ... ] > @@ -558,46 +560,128 @@ static void aie4_classic_hw_stop(struct > amdxdna_dev_hdl *ndev) > aie4_fw_stop(ndev); > } > > -static int aie4_request_firmware(struct amdxdna_dev_hdl *ndev, > - const struct firmware **npufw, > - const struct firmware **certfw) > +/* > + * PSP C2PMSG_63 (SMN) bit 6: 0 = default firmware signing, 1 = alternative > + * PSP C2PMSG_63 (SMN) bit 28: 0 = production part, 1 = preproduction part > + */ > +#define AIE4_PSP_C2PMSG_63_SMN_ADDR 0x38109fc > +#define AIE4_PSP_FW_SIGN_ALT BIT(6) > +#define AIE4_PSP_FW_PREPROD BIT(28) > + > +enum aie4_fw_variant { > + AIE4_FW_DEFAULT, > + AIE4_FW_ALT, /* "_l" images */ > + AIE4_FW_PREPROD, /* "_p" images */ > +}; > + > +/* > + * The PSP only trusts firmware signed with the scheme the platform firmware > + * uses and a preproduction part needs firmware of its own. Reading which > one is > + * in use is optional and only used as a hint for which images to try first: > + * anything but a clear answer is treated as default. > + */ > +static enum aie4_fw_variant aie4_psp_fw_variant(struct amdxdna_dev *xdna) > { > - struct amdxdna_dev *xdna = ndev->aie.xdna; > - struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev); > - char fw_name[128]; > + u32 val; > int ret; > > - ret = snprintf(fw_name, sizeof(fw_name), "amdnpu/%04x_%02x/%s", > - pdev->device, pdev->revision, ndev->priv->npufw_path); > - if (ret >= sizeof(fw_name)) { > - XDNA_ERR(xdna, "npu firmware path is truncated"); > - return -EINVAL; > + ret = amd_smn_read(0, AIE4_PSP_C2PMSG_63_SMN_ADDR, &val); > + if (ret) { > + XDNA_DBG(xdna, "PSP firmware variant not available, ret %d", > ret); > + return AIE4_FW_DEFAULT; > } [ ... ] > @@ -645,18 +729,40 @@ static int aie4_prepare_firmware(struct amdxdna_dev_hdl > *ndev, > return 0; > } > > +/* > + * The firmware can only be started once. It is the PSP that decides whether > it > + * accepts the images, and it does that when the firmware is started, so > there > + * is no trying a variant and falling back. Pick the variant that the PSP > + * reported (the default one if it can't be read) and use the others only if > + * its images are missing. > + */ > static int aie4_load_fw(struct amdxdna_dev_hdl *ndev, > void __iomem *tbl[PCI_NUM_RESOURCES]) > { > + struct amdxdna_dev *xdna = ndev->aie.xdna; > + enum aie4_fw_variant hint = aie4_psp_fw_variant(xdna); > const struct firmware *npufw, *certfw; > - int ret; > + enum aie4_fw_variant order[3]; > + int i, n = 0, ret = -ENOENT; > > if (!ndev->priv->npufw_path && !ndev->priv->certfw_path) > return 0; [Severity: Medium] Can calling aie4_psp_fw_variant() here read uninitialized stack memory when amd_smn_read() fails early? In aie4_psp_fw_variant(), val is declared uninitialized on the stack and passed to amd_smn_read(). In arch/x86/kernel/amd_node.c:amd_smn_read(), *value is unconditionally inspected even when __amd_smn_rw() returns an error before pci_read_config_dword() writes to *value: arch/x86/kernel/amd_node.c:amd_smn_read() { int err = __amd_smn_rw(SMN_INDEX_OFFSET, SMN_DATA_OFFSET, node, address, value, false); if (PCI_POSSIBLE_ERROR(*value)) { err = -ENODEV; *value = 0; } return err; } When __amd_smn_rw() returns early with -ENODEV (for example when !amd_roots in a guest VM, or when pci_request_config_region_exclusive() failed during amd_smn_init()), PCI_POSSIBLE_ERROR(*value) reads the uninitialized stack variable val. Additionally, because aie4_load_fw() calls aie4_psp_fw_variant(xdna) in its variable declarations before checking: if (!ndev->priv->npufw_path && !ndev->priv->certfw_path) return 0; probing a VF device (aie4_vf_init() -> aie4m_pcidev_init() -> aie4_load_fw(), where npu3_dev_vf_priv has both paths set to NULL) will always perform this SMN read and hit the uninitialized read in a guest VM before immediately returning 0. Should val be initialized in aie4_psp_fw_variant() (e.g., u32 val = 0;), and should the aie4_psp_fw_variant(xdna) call in aie4_load_fw() be moved after the npufw_path/certfw_path check? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
