Re: [PATCH] drm/amdkfd: Fix out-of-bounds read in kdf_create_vcrat_image_cpu()
On Fri, Jan 08, 2021 at 06:46:17PM -0500, Felix Kuehling wrote: > Am 2021-01-08 um 11:31 a.m. schrieb Jeremy Cline: > > KASAN reported a slab-out-of-bounds read of size 1 in > > kdf_create_vcrat_image_cpu(). > > > > This occurs when, for example, when on an x86_64 with a single NUMA node > > because kfd_fill_iolink_info_for_cpu() is a no-op, but afterwards the > > sub_type_hdr->length, which is out-of-bounds, is read and multiplied by > > entries. Fortunately, entries is 0 in this case so the overall > > crat_table->length is still correct. > > That's a pretty big change to fix that. Wouldn't it be enough to add a > simple check after calling kfd_fill_iolink_info_for_cpu: > > if (entries) { > crat_table->length += (sub_type_hdr->length * entries); > crat_table->total_entries += entries; > } > > Or change the output parameters of the kfd_fill_..._for_cpu functions > from num_entries to size_filled, so the caller doesn't need to read > sub_type_hdr->length any more. > For sure. I felt like this was a bit tidier afterwards, but that's an opinion and not one I hold strongly. I'll look at preparing a smaller fix next week. Thanks, Jeremy > > > > This refactors the helper functions to accept the crat_table directly > > and calculate the table entry pointer based on the current table length. > > This allows us to avoid an out-of-bounds read and hopefully makes the > > pointer arithmetic clearer. It should have no functional change beyond > > removing the out-of-bounds read. > > > > Fixes: b7b6c38529c9 ("drm/amdkfd: Calculate CPU VCRAT size dynamically > > (v2)") > > Signed-off-by: Jeremy Cline > > --- > > drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 86 +-- > > 1 file changed, 40 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > index 8cac497c2c45..e50db2c0f4ee 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > @@ -829,21 +829,24 @@ int kfd_create_crat_image_acpi(void **crat_image, > > size_t *size) > > /* kfd_fill_cu_for_cpu - Fill in Compute info for the given CPU NUMA node > > * > > * @numa_node_id: CPU NUMA node id > > - * @avail_size: Available size in the memory > > - * @sub_type_hdr: Memory into which compute info will be filled in > > + * @avail_size: Available space in bytes at the end of the @crat_table. > > + * @crat_table: The CRAT table to append the Compute info to; > > + * on success the table length and total_entries count is updated. > > * > > * Return 0 if successful else return -ve value > > */ > > static int kfd_fill_cu_for_cpu(int numa_node_id, int *avail_size, > > - int proximity_domain, > > - struct crat_subtype_computeunit *sub_type_hdr) > > + struct crat_header *crat_table) > > { > > const struct cpumask *cpumask; > > + struct crat_subtype_computeunit *sub_type_hdr; > > > > *avail_size -= sizeof(struct crat_subtype_computeunit); > > if (*avail_size < 0) > > return -ENOMEM; > > > > + sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table + > > + crat_table->length); > > memset(sub_type_hdr, 0, sizeof(struct crat_subtype_computeunit)); > > > > /* Fill in subtype header data */ > > @@ -855,36 +858,42 @@ static int kfd_fill_cu_for_cpu(int numa_node_id, int > > *avail_size, > > > > /* Fill in CU data */ > > sub_type_hdr->flags |= CRAT_CU_FLAGS_CPU_PRESENT; > > - sub_type_hdr->proximity_domain = proximity_domain; > > + sub_type_hdr->proximity_domain = crat_table->num_domains; > > sub_type_hdr->processor_id_low = kfd_numa_node_to_apic_id(numa_node_id); > > if (sub_type_hdr->processor_id_low == -1) > > return -EINVAL; > > > > sub_type_hdr->num_cpu_cores = cpumask_weight(cpumask); > > > > + crat_table->length += sub_type_hdr->length; > > + crat_table->total_entries++; > > + > > return 0; > > } > > > > /* kfd_fill_mem_info_for_cpu - Fill in Memory info for the given CPU NUMA > > node > > * > > * @numa_node_id: CPU NUMA node id > > - * @avail_size: Available size in the memory > > - * @sub_type_hdr: Memory into which compute info will be filled in > > + * @avail_size: Available space in bytes at the end of the @crat_table. > > + * @crat_table: The CRAT table to append the Memory info to; > > + * on success the table length and total_entries count is updated. > > * > > * Return 0 if successful else return -ve value > > */ > > static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size, > > - int proximity_domain, > > - struct crat_subtype_memory *sub_type_hdr) > > + struct crat_header *crat_table) > > { > > uint64_t mem_in_bytes = 0; > > pg_data_t *pgdat; > > int zone_type; > > + struct crat_subtype_memory
Re: [PATCH] drm/amdkfd: Fix out-of-bounds read in kdf_create_vcrat_image_cpu()
Am 2021-01-08 um 11:31 a.m. schrieb Jeremy Cline: > KASAN reported a slab-out-of-bounds read of size 1 in > kdf_create_vcrat_image_cpu(). > > This occurs when, for example, when on an x86_64 with a single NUMA node > because kfd_fill_iolink_info_for_cpu() is a no-op, but afterwards the > sub_type_hdr->length, which is out-of-bounds, is read and multiplied by > entries. Fortunately, entries is 0 in this case so the overall > crat_table->length is still correct. That's a pretty big change to fix that. Wouldn't it be enough to add a simple check after calling kfd_fill_iolink_info_for_cpu: if (entries) { crat_table->length += (sub_type_hdr->length * entries); crat_table->total_entries += entries; } Or change the output parameters of the kfd_fill_..._for_cpu functions from num_entries to size_filled, so the caller doesn't need to read sub_type_hdr->length any more. Regards, Felix > > This refactors the helper functions to accept the crat_table directly > and calculate the table entry pointer based on the current table length. > This allows us to avoid an out-of-bounds read and hopefully makes the > pointer arithmetic clearer. It should have no functional change beyond > removing the out-of-bounds read. > > Fixes: b7b6c38529c9 ("drm/amdkfd: Calculate CPU VCRAT size dynamically (v2)") > Signed-off-by: Jeremy Cline > --- > drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 86 +-- > 1 file changed, 40 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > index 8cac497c2c45..e50db2c0f4ee 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > @@ -829,21 +829,24 @@ int kfd_create_crat_image_acpi(void **crat_image, > size_t *size) > /* kfd_fill_cu_for_cpu - Fill in Compute info for the given CPU NUMA node > * > * @numa_node_id: CPU NUMA node id > - * @avail_size: Available size in the memory > - * @sub_type_hdr: Memory into which compute info will be filled in > + * @avail_size: Available space in bytes at the end of the @crat_table. > + * @crat_table: The CRAT table to append the Compute info to; > + * on success the table length and total_entries count is updated. > * > * Return 0 if successful else return -ve value > */ > static int kfd_fill_cu_for_cpu(int numa_node_id, int *avail_size, > - int proximity_domain, > - struct crat_subtype_computeunit *sub_type_hdr) > + struct crat_header *crat_table) > { > const struct cpumask *cpumask; > + struct crat_subtype_computeunit *sub_type_hdr; > > *avail_size -= sizeof(struct crat_subtype_computeunit); > if (*avail_size < 0) > return -ENOMEM; > > + sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table + > + crat_table->length); > memset(sub_type_hdr, 0, sizeof(struct crat_subtype_computeunit)); > > /* Fill in subtype header data */ > @@ -855,36 +858,42 @@ static int kfd_fill_cu_for_cpu(int numa_node_id, int > *avail_size, > > /* Fill in CU data */ > sub_type_hdr->flags |= CRAT_CU_FLAGS_CPU_PRESENT; > - sub_type_hdr->proximity_domain = proximity_domain; > + sub_type_hdr->proximity_domain = crat_table->num_domains; > sub_type_hdr->processor_id_low = kfd_numa_node_to_apic_id(numa_node_id); > if (sub_type_hdr->processor_id_low == -1) > return -EINVAL; > > sub_type_hdr->num_cpu_cores = cpumask_weight(cpumask); > > + crat_table->length += sub_type_hdr->length; > + crat_table->total_entries++; > + > return 0; > } > > /* kfd_fill_mem_info_for_cpu - Fill in Memory info for the given CPU NUMA > node > * > * @numa_node_id: CPU NUMA node id > - * @avail_size: Available size in the memory > - * @sub_type_hdr: Memory into which compute info will be filled in > + * @avail_size: Available space in bytes at the end of the @crat_table. > + * @crat_table: The CRAT table to append the Memory info to; > + * on success the table length and total_entries count is updated. > * > * Return 0 if successful else return -ve value > */ > static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size, > - int proximity_domain, > - struct crat_subtype_memory *sub_type_hdr) > + struct crat_header *crat_table) > { > uint64_t mem_in_bytes = 0; > pg_data_t *pgdat; > int zone_type; > + struct crat_subtype_memory *sub_type_hdr; > > *avail_size -= sizeof(struct crat_subtype_memory); > if (*avail_size < 0) > return -ENOMEM; > > + sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table + > + crat_table->length); > memset(sub_type_hdr, 0, sizeof(struct crat_subtype_memory)); > > /* Fill in subtype
[PATCH] drm/amdkfd: Fix out-of-bounds read in kdf_create_vcrat_image_cpu()
KASAN reported a slab-out-of-bounds read of size 1 in kdf_create_vcrat_image_cpu(). This occurs when, for example, when on an x86_64 with a single NUMA node because kfd_fill_iolink_info_for_cpu() is a no-op, but afterwards the sub_type_hdr->length, which is out-of-bounds, is read and multiplied by entries. Fortunately, entries is 0 in this case so the overall crat_table->length is still correct. This refactors the helper functions to accept the crat_table directly and calculate the table entry pointer based on the current table length. This allows us to avoid an out-of-bounds read and hopefully makes the pointer arithmetic clearer. It should have no functional change beyond removing the out-of-bounds read. Fixes: b7b6c38529c9 ("drm/amdkfd: Calculate CPU VCRAT size dynamically (v2)") Signed-off-by: Jeremy Cline --- drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 86 +-- 1 file changed, 40 insertions(+), 46 deletions(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c index 8cac497c2c45..e50db2c0f4ee 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c @@ -829,21 +829,24 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t *size) /* kfd_fill_cu_for_cpu - Fill in Compute info for the given CPU NUMA node * * @numa_node_id: CPU NUMA node id - * @avail_size: Available size in the memory - * @sub_type_hdr: Memory into which compute info will be filled in + * @avail_size: Available space in bytes at the end of the @crat_table. + * @crat_table: The CRAT table to append the Compute info to; + * on success the table length and total_entries count is updated. * * Return 0 if successful else return -ve value */ static int kfd_fill_cu_for_cpu(int numa_node_id, int *avail_size, - int proximity_domain, - struct crat_subtype_computeunit *sub_type_hdr) + struct crat_header *crat_table) { const struct cpumask *cpumask; + struct crat_subtype_computeunit *sub_type_hdr; *avail_size -= sizeof(struct crat_subtype_computeunit); if (*avail_size < 0) return -ENOMEM; + sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table + + crat_table->length); memset(sub_type_hdr, 0, sizeof(struct crat_subtype_computeunit)); /* Fill in subtype header data */ @@ -855,36 +858,42 @@ static int kfd_fill_cu_for_cpu(int numa_node_id, int *avail_size, /* Fill in CU data */ sub_type_hdr->flags |= CRAT_CU_FLAGS_CPU_PRESENT; - sub_type_hdr->proximity_domain = proximity_domain; + sub_type_hdr->proximity_domain = crat_table->num_domains; sub_type_hdr->processor_id_low = kfd_numa_node_to_apic_id(numa_node_id); if (sub_type_hdr->processor_id_low == -1) return -EINVAL; sub_type_hdr->num_cpu_cores = cpumask_weight(cpumask); + crat_table->length += sub_type_hdr->length; + crat_table->total_entries++; + return 0; } /* kfd_fill_mem_info_for_cpu - Fill in Memory info for the given CPU NUMA node * * @numa_node_id: CPU NUMA node id - * @avail_size: Available size in the memory - * @sub_type_hdr: Memory into which compute info will be filled in + * @avail_size: Available space in bytes at the end of the @crat_table. + * @crat_table: The CRAT table to append the Memory info to; + * on success the table length and total_entries count is updated. * * Return 0 if successful else return -ve value */ static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size, - int proximity_domain, - struct crat_subtype_memory *sub_type_hdr) + struct crat_header *crat_table) { uint64_t mem_in_bytes = 0; pg_data_t *pgdat; int zone_type; + struct crat_subtype_memory *sub_type_hdr; *avail_size -= sizeof(struct crat_subtype_memory); if (*avail_size < 0) return -ENOMEM; + sub_type_hdr = (typeof(sub_type_hdr))((char *)crat_table + + crat_table->length); memset(sub_type_hdr, 0, sizeof(struct crat_subtype_memory)); /* Fill in subtype header data */ @@ -905,27 +914,37 @@ static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size, sub_type_hdr->length_low = lower_32_bits(mem_in_bytes); sub_type_hdr->length_high = upper_32_bits(mem_in_bytes); - sub_type_hdr->proximity_domain = proximity_domain; + sub_type_hdr->proximity_domain = crat_table->num_domains; + + crat_table->length += sub_type_hdr->length; + crat_table->total_entries++; return 0; } #ifdef CONFIG_X86_64 +/* kfd_fill_iolink_info_for_cpu() - Add IO link info to a Virtual CRAT + * + * @numa_node_id: The NUMA node ID