Re: [PATCH] accel/amdxdna: Load the aie4 firmware variant the PSP reports

[sashiko-bot@kernel.org]

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 <mario.limonciello@amd.com>

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/20261104-superm1-lms-npu-v1-1-dbffcc5abc23@amd.com?part=1