Re: [PATCH] drm/amdkfd: validate SRAT subtables before parsing

Re: [PATCH] drm/amdkfd: validate SRAT subtables before parsing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/amdkfd: unaligned access and endianness bugs in SRAT parsing
--

commit 172d11efb00c0dedbe45ba294e5438f125fd1ad9
Author: Jiale Yao <yaojiale02@163.com>

drm/amdkfd: validate SRAT subtables before parsing

The KFD SRAT walker reads the first subtable length without checking that a
header is present and trusts subsequent lengths when accessing CPU and generic
affinity records. This patch validates the table header, subtable ranges, and
supported typed records to prevent out-of-bounds reads.

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> index 6e0df685503d0..32d7573dffc94 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
[ ... ]
> @@ -2056,27 +2056,33 @@ static void kfd_find_numa_node_in_srat(struct kfd_node *kdev)
[ ... ]
>  		switch (sub_header->type) {
>  		case ACPI_SRAT_TYPE_CPU_AFFINITY:
>  			cpu = (struct acpi_srat_cpu_affinity *)sub_header;
> +			if (subtable_len < sizeof(*cpu))
> +				goto invalid_srat;
> +
>  			pxm = *((u32 *)cpu->proximity_domain_hi) << 8 |
>  					cpu->proximity_domain_lo;

[Severity: High]
cpu->proximity_domain_hi is at offset 9 in the packed acpi_srat_cpu_affinity
struct. Can casting it to a u32 pointer lead to an unaligned memory access,
potentially triggering exceptions on architectures that do not support native
unaligned loads?

Also, on Big-Endian architectures, reading proximity_domain_hi as a u32 loads
bytes in reverse order. Does shifting by 8 discard the most significant byte
instead of clock_domain[0], resulting in a corrupted pxm value?

>  			if (pxm > max_pxm)
> @@ -2084,6 +2090,9 @@ static void kfd_find_numa_node_in_srat(struct kfd_node *kdev)
>  			break;
>  		case ACPI_SRAT_TYPE_GENERIC_AFFINITY:
>  			gpu = (struct acpi_srat_generic_affinity *)sub_header;
> +			if (subtable_len < sizeof(*gpu))
> +				goto invalid_srat;
> +
>  			bdf = *((u16 *)(&gpu->device_handle[0])) << 16 |
>  					*((u16 *)(&gpu->device_handle[2]));

[Severity: High]
Reading gpu->device_handle using u16 pointer casts correctly extracts the
segment and BDF on Little-Endian, but can this swap the bytes of both the
segment and the BDF on Big-Endian systems? Does this break GPU NUMA node
discovery on Big-Endian platforms?

>  			if (bdf == pci_id) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016141504.2794541-1-yaojiale02@163.com?part=1