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 jcline@redhat.com

---

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 for the CPU; as from for_each_online_node()
• • @avail_size: Available space in bytes at the end of the @crat_table.
• • @crat_table: The CRAT table to append the IO link 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_iolink_info_for_cpu(int numa_node_id, int *avail_size,

• 		uint32_t *num_entries,
  

• 		struct crat_subtype_iolink *sub_type_hdr)
  

• 		struct crat_header *crat_table)
  

{ int nid; struct cpuinfo_x86 *c = &cpu_data(0); uint8_t link_type;

• struct crat_subtype_iolink *sub_type_hdr;

  if (c->x86_vendor == X86_VENDOR_AMD) link_type = CRAT_IOLINK_TYPE_HYPERTRANSPORT; else link_type = CRAT_IOLINK_TYPE_QPI_1_1;

• *num_entries = 0;
• /* Create IO links from this node to other CPU nodes */ for_each_online_node(nid) { if (nid == numa_node_id) /* node itself */

@@ -935,6 +954,8 @@ static int kfd_fill_iolink_info_for_cpu(int numa_node_id, int *avail_size, 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_iolink));

  /* Fill in subtype header data */

@@ -947,8 +968,8 @@ static int kfd_fill_iolink_info_for_cpu(int numa_node_id, int *avail_size, sub_type_hdr->proximity_domain_to = nid; sub_type_hdr->io_interface_type = link_type;

• (*num_entries)++;
  

• sub_type_hdr++;
  

• crat_table->length += sub_type_hdr->length;
  

• crat_table->total_entries++;
  

  }

  return 0;

@@ -966,12 +987,8 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size) struct crat_header *crat_table = (struct crat_header *)pcrat_image; struct acpi_table_header *acpi_table; acpi_status status;

• struct crat_subtype_generic *sub_type_hdr; int avail_size = *size; int numa_node_id;

-#ifdef CONFIG_X86_64

• uint32_t entries = 0;

-#endif int ret = 0;

if (!pcrat_image) @@ -1003,48 +1020,25 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size) crat_table->total_entries = 0; crat_table->num_domains = 0;

• sub_type_hdr = (struct crat_subtype_generic *)(crat_table+1);

• for_each_online_node(numa_node_id) { if (kfd_numa_node_to_apic_id(numa_node_id) == -1) continue;

  /* Fill in Subtype: Compute Unit */

• ret = kfd_fill_cu_for_cpu(numa_node_id, &avail_size,
  

• 	crat_table->num_domains,
  

• 	(struct crat_subtype_computeunit *)sub_type_hdr);
  

• ret = kfd_fill_cu_for_cpu(numa_node_id, &avail_size, crat_table);
  

  if (ret < 0) return ret;

• crat_table->length += sub_type_hdr->length;
  

• crat_table->total_entries++;
  

• sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
  

• 	sub_type_hdr->length);
  
  

  /* Fill in Subtype: Memory */

• ret = kfd_fill_mem_info_for_cpu(numa_node_id, &avail_size,
  

• 	crat_table->num_domains,
  

• 	(struct crat_subtype_memory *)sub_type_hdr);
  

• ret = kfd_fill_mem_info_for_cpu(numa_node_id, &avail_size, crat_table);
  

  if (ret < 0) return ret;

• crat_table->length += sub_type_hdr->length;
  

• crat_table->total_entries++;
  

• sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
  

• 	sub_type_hdr->length);
  
  

  /* Fill in Subtype: IO Link */

#ifdef CONFIG_X86_64

• ret = kfd_fill_iolink_info_for_cpu(numa_node_id, &avail_size,
  

• 		&entries,
  

• 		(struct crat_subtype_iolink *)sub_type_hdr);
  

• ret = kfd_fill_iolink_info_for_cpu(numa_node_id, &avail_size, crat_table);
  

  if (ret < 0) return ret;

• crat_table->length += (sub_type_hdr->length * entries);
  

• crat_table->total_entries += entries;
  

• sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
  

• 		sub_type_hdr->length * entries);
  

#else pr_info("IO link not available for non x86 platforms\n"); #endif

Am 2021-01-11 um 4:05 p.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.

Check if there were any entries before de-referencing sub_type_hdr which may be pointing to out-of-bounds memory.

Fixes: b7b6c38529c9 ("drm/amdkfd: Calculate CPU VCRAT size dynamically (v2)") Suggested-by: Felix Kuehling Felix.Kuehling@amd.com Signed-off-by: Jeremy Cline jcline@redhat.com

Thanks. I'll apply this patch.

Reviewed-by: Felix Kuehling Felix.Kuehling@amd.com

---

drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c index 8cac497c2c45..a5640a6138cf 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c @@ -1040,11 +1040,14 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size) (struct crat_subtype_iolink *)sub_type_hdr); if (ret < 0) return ret;

• crat_table->length += (sub_type_hdr->length * entries);
  

• crat_table->total_entries += entries;
  
  

• sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
  

• 		sub_type_hdr->length * entries);
  

• if (entries) {
  

• 	crat_table->length += (sub_type_hdr->length * entries);
  

• 	crat_table->total_entries += entries;
  

• 	sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
  

• 			sub_type_hdr->length * entries);
  

• }
  

#else pr_info("IO link not available for non x86 platforms\n"); #endif