Re: [PATCH v4 4/5] x86/amd_node: Skip SMN setup on Hygon Fam18h models 0x04-0x08

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] amd_smn_init() stores pci_dev pointers in the amd_roots array without acquiring independent references, leading to an under-refcounted device and a use-after-free risk.
- [High] amd_smn_read() unconditionally evaluates uninitialized memory when __amd_smn_rw() fails early, causing Undefined Behavior.
- [High] Kernel panic due to a modulo by zero if the number of PCI root bridges is less than the number of AMD nodes.
--

commit 64cdde27c04c50425e1c00a79b17abd2a38904c9
Author: Lin Wang <wanglin@open-hieco.net>

x86/amd_node: Skip SMN setup on Hygon Fam18h models 0x04-0x08

This commit skips AMD SMN setup on specific Hygon Fam18h models (0x04-0x08).
It prevents incorrect node-to-root mappings by leaving the roots unclaimed,
returning -ENODEV for SMN calls while setup is inactive.

> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index 0be01725a2a45..a0cfd74ada036 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -88,6 +88,9 @@ static int __amd_smn_rw(u8 i_off, u8 d_off, u16 node, u32 address, u32 *value, b
>  	struct pci_dev *root;
>  	int err = -ENODEV;
>  
> +	if (!smn_exclusive)
> +		return err;
> +

[Severity: High]
This isn't a bug introduced by this patch, but this patch guarantees it will
trigger routinely on the excluded Hygon models.

Does this early return result in an uninitialized variable read?

When __amd_smn_rw() returns early here because smn_exclusive is false (which
this patch forces for specific Hygon models), the output parameter value is
never written to.

Callers typically pass a pointer to an uninitialized local variable, for
example in amd_smn_read():

arch/x86/kernel/amd_node.c:amd_smn_read() {
	int err = __amd_smn_rw(..., value, false);

	if (PCI_POSSIBLE_ERROR(*value)) {
		err = -ENODEV;
		*value = 0;
	}
...
}

Evaluating PCI_POSSIBLE_ERROR(*value) when *value is uninitialized causes
Undefined Behavior and can lead to information leaks or KMSAN kernel panics.

[ ... ]

> @@ -252,6 +252,18 @@ static int __init amd_smn_init(void)
>  	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
>  		return 0;
>  
> +	/*
> +	 * Hygon models outside this range, including legacy parts, keep the
> +	 * existing AMD SMN setup. Models 0x04-0x08 group SMN roots by socket
> +	 * instead of amd_num_nodes(), so leave AMD SMN inactive. The Hygon node
> +	 * layer, when built, owns their node-to-root mapping.
> +	 */
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON &&
> +	    boot_cpu_data.x86 == 0x18 &&
> +	    boot_cpu_data.x86_model >= 0x04 &&
> +	    boot_cpu_data.x86_model <= 0x08)
> +		return 0;
> +
>  	guard(mutex)(&smn_mutex);
>  
>  	if (amd_roots)

[Severity: High]
This is a pre-existing issue, but it remains a severe flaw for other
configurations (and is not fixed in subsequent patches in the series).

Can this cause a kernel panic due to a modulo by zero?

In amd_smn_init(), roots_per_node is calculated as num_roots / num_nodes.
If num_roots is less than num_nodes (e.g. due to virtualized hardware, a
broken BIOS, or unusual topologies), roots_per_node evaluates to 0.

The subsequent loop performs a modulo operation:

arch/x86/kernel/amd_node.c:amd_smn_init() {
...
		if (count++ % roots_per_node)
			continue;
...
}

This division by zero triggers a Divide Error (#DE) trap, leading to an
immediate kernel panic during boot on affected hardware environments.


[Severity: High]
This isn't a bug introduced by this patch, but it remains a severe flaw (and
is not fixed in subsequent patches in the series).

Is there a missing reference count increment when storing device pointers in
the amd_roots array?

In the amd_smn_init() loop, the device pointer is saved without acquiring an
independent reference:

arch/x86/kernel/amd_node.c:amd_smn_init() {
...
	while (node < num_nodes && (root = get_next_root(root))) {
		if (count++ % roots_per_node)
			continue;

		pci_dbg(root, "is root for AMD node %u\n", node);
		amd_roots[node++] = root;
	}
...
}

On the next loop iteration, get_next_root(root) is called, which delegates
to pci_get_class() and implicitly drops the reference count of the passed
device.

Since this executes unconditionally during a successful boot on AMD hardware,
the array holds pointers to under-refcounted devices, creating a
use-after-free risk for PCI host bridge devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901071700.3255382-1-wanglin@open-hieco.net?part=4