Re: [PATCH v3 08/16] accel/thames: Add support for submitting DMA operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] accel/thames: destination bounds bypass in thames_transfer_translate()
- [High] accel/thames: premature table publication in thames_transfers_build()
- [High] accel/thames: use-after-free of transfer_table_pool_map after unbind
--

commit 46b1c4034d8528dbd324d7c61cb1cf6deecdf877
Author: Tomeu Vizoso <tomeu.vizoso@gmail.com>

accel/thames: Add support for submitting DMA operations

This commit adds support for validating, translating, and writing C7x DRU DMA
transfer records into an IPC transfer table pool so jobs can perform
asynchronous DMA copies between context BOs and L2 apertures.

> diff --git a/drivers/accel/thames/thames_drv.c b/drivers/accel/thames/thames_drv.c
> index a17e7a90f7c1d..030c60963a0a8 100644
> --- a/drivers/accel/thames/thames_drv.c
> +++ b/drivers/accel/thames/thames_drv.c

[ ... ]

> @@ -269,6 +272,10 @@ static struct thames_device *thames_device_init(struct platform_device *pdev,
>  	if (err)
>  		return ERR_PTR(err);
>  
> +	err = drmm_mutex_init(&tdev->ddev, &tdev->transfer_table_pool_lock);
> +	if (err)
> +		return ERR_PTR(err);
> +
>  	err = drmm_mutex_init(&tdev->ddev, &tdev->asid_lock);
>  	if (err)
>  		return ERR_PTR(err);
> @@ -281,6 +288,7 @@ static struct thames_device *thames_device_init(struct platform_device *pdev,
>  	if (err)
>  		return ERR_PTR(err);
>  
> +	INIT_LIST_HEAD(&tdev->transfer_tables);
>  	INIT_LIST_HEAD(&tdev->contexts);
>  
>  	tdev->asid_next = 1;
> @@ -349,6 +357,15 @@ static int thames_platform_probe(struct platform_device *pdev)
>  		goto err_fini_dev;
>  	}
>  
> +	tdev->transfer_table_pool_map =
> +		devm_bitmap_zalloc(&pdev->dev,
> +				   tdev->caps.transfer_table_pool_size / THAMES_ABI_LINE_SIZE,
> +				   GFP_KERNEL);
> +	if (!tdev->transfer_table_pool_map) {
> +		err = -ENOMEM;
> +		goto err_fini_cores;
> +	}

[Severity: High]
Can allocating tdev->transfer_table_pool_map with devm_bitmap_zalloc() on
&pdev->dev in thames_platform_probe() cause a use-after-free in
thames_transfers_free() if a job is freed after the platform driver unbinds?

In drivers/accel/thames/thames_drv.c, thames_device_init() allocates tdev with
devm_drm_dev_alloc() and initializes tdev->transfer_table_pool_lock with
drmm_mutex_init(&tdev->ddev, ...) so that client buffers and jobs can clean up
after driver unbind.

However, devm_bitmap_zalloc(&pdev->dev, ...) ties the lifetime of
tdev->transfer_table_pool_map to the platform device binding rather than to
tdev->ddev. If the platform device is unbound while a DRM file or deferred job
cleanup outlives thames_platform_remove(), devres releases
tdev->transfer_table_pool_map while tdev->ddev is still alive. When the job is
subsequently freed:

  thames_sched_job_free()
    -> thames_job_free()
      -> thames_transfers_free()

thames_transfers_free() in drivers/accel/thames/thames_transfers.c acquires
tdev->transfer_table_pool_lock and calls bitmap_clear() on the already-freed
tdev->transfer_table_pool_map:

	scoped_guard(mutex, &tdev->transfer_table_pool_lock) {
		bitmap_clear(tdev->transfer_table_pool_map, table->offset / THAMES_ABI_LINE_SIZE,
			     table->size / THAMES_ABI_LINE_SIZE);
		list_del(&table->node);
	}

Should tdev->transfer_table_pool_map be managed by drmm (or freed in
thames_device_release()) so its lifetime matches tdev->ddev and
tdev->transfer_table_pool_lock?

> diff --git a/drivers/accel/thames/thames_transfers.c b/drivers/accel/thames/thames_transfers.c
> new file mode 100644
> index 0000000000000..aad4542d5cb95
> --- /dev/null
> +++ b/drivers/accel/thames/thames_transfers.c

[ ... ]

> +static int thames_transfer_translate(struct thames_file_priv *file_priv,
> +				     const struct drm_thames_transfer *t,
> +				     struct thames_transfer_table *table,
> +				     struct thames_transfer *out)
> +{
> +	const u32 *w = t->words;
> +	u32 icnt[4], dicnt[4];
> +	s32 dim[4] = { 0 }, ddim[4] = { 0 };
> +	u64 src, dst, start, end, bytes, dbytes, addr;
> +	int ret;
> +
> +	if (!thames_transfer_flags_ok(w[TR_FLAGS]) || w[TR_FMTFLAGS])
> +		return -EINVAL;
> +
> +	icnt[0] = w[TR_ICNT01] & 0xffff;
> +	icnt[1] = w[TR_ICNT01] >> 16;
> +	icnt[2] = w[TR_ICNT23] & 0xffff;
> +	icnt[3] = w[TR_ICNT23] >> 16;
> +	dim[1] = w[TR_DIM1];
> +	dim[2] = w[TR_DIM2];
> +	dim[3] = w[TR_DIM3];
> +	src = (u64)w[TR_ADDR_HI] << 32 | w[TR_ADDR_LO];
> +
> +	dicnt[0] = w[TR_DICNT01] & 0xffff;
> +	dicnt[1] = w[TR_DICNT01] >> 16;
> +	dicnt[2] = w[TR_DICNT23] & 0xffff;
> +	dicnt[3] = w[TR_DICNT23] >> 16;
> +	ddim[1] = w[TR_DDIM1];
> +	ddim[2] = w[TR_DDIM2];
> +	ddim[3] = w[TR_DDIM3];
> +	dst = (u64)w[TR_DADDR_HI] << 32 | w[TR_DADDR_LO];
> +
> +	memcpy(out->record.words, w, sizeof(out->record.words));
> +	out->channel = t->channel;
> +
> +	ret = thames_transfer_span(src, icnt, dim, &start, &end, &bytes);
> +	if (ret)
> +		return ret;
> +	ret = thames_transfer_side(file_priv, start, end, false, table, &out->side[0], &addr);
> +	if (ret)
> +		return ret;
> +	addr += src - start;
> +	out->record.words[TR_ADDR_LO] = lower_32_bits(addr);
> +	out->record.words[TR_ADDR_HI] = upper_32_bits(addr);
> +
> +	ret = thames_transfer_span(dst, dicnt, ddim, &start, &end, &dbytes);
> +	if (ret)
> +		return ret;
> +	if (dbytes != bytes)
> +		return -EINVAL;
> +	ret = thames_transfer_side(file_priv, start, end, true, table, &out->side[1], &addr);

[Severity: Critical]
In drivers/accel/thames/thames_transfers.c, does computing the destination
span in thames_transfer_translate() from dicnt[] (words TR_DICNT01 and
TR_DICNT23) instead of icnt[] allow a Type 8 transfer record to bypass
destination BO bounds checking?

THAMES_TRANSFER_TYPES in drivers/accel/thames/thames_transfers.h is BIT(8), so
thames_transfer_flags_ok() only accepts Type 8 (4D block move without
repacking). In the K3 DRU transfer record format (SPRUIL1D 10.4.3.3), words 14
and 15 (DICNT0..3) are only used by repacking transfer types such as Type 9
and Type 15 (which the UAPI comment on struct drm_thames_transfer in
include/uapi/drm/thames_accel.h references via Table 10-63). For Type 8, words
14 and 15 are reserved and the DRU uses ICNT0..3 for both source and
destination loop iterations while stepping the destination address by
DDIM1..3.

Because thames_transfer_translate() passes dicnt[] to thames_transfer_span()
and only checks that dbytes == bytes, a client can submit a Type 8 record with
icnt = { 1, N, 1, 1 } (for N > 1), dicnt = { N, 1, 1, 1 }, and an arbitrary
ddim[1] stride. In that case:

1. thames_transfer_span() calculates (dicnt[1] - 1) * ddim[1] == 0 and
   validates only [dst, dst + N) against the destination mapping.
2. The DRU executes ICNT1 = N iterations using DDIM1 and writes to physical
   addresses dst + i * ddim[1] outside the validated BO.
3. Conversely, a Type 8 record with reserved words 14 and 15 set to zero is
   rejected with -EINVAL because dicnt[0] is zero.

For Type 8 records, should thames_transfer_translate() require w[TR_DICNT01]
and w[TR_DICNT23] to be zero and pass icnt[] to thames_transfer_span() when
computing the destination span?

[ ... ]

> +static int thames_transfers_alloc_table(struct thames_device *tdev,
> +					struct thames_transfer_table *table)
> +{
> +	const unsigned long lines = tdev->caps.transfer_table_pool_size / THAMES_ABI_LINE_SIZE;
> +	unsigned long start, nr = table->size / THAMES_ABI_LINE_SIZE;
> +
> +	guard(mutex)(&tdev->transfer_table_pool_lock);
> +
> +	start = bitmap_find_next_zero_area(tdev->transfer_table_pool_map, lines, 0, nr, 0);
> +	if (start + nr > lines)
> +		return -ENOSPC;
> +
> +	bitmap_set(tdev->transfer_table_pool_map, start, nr);
> +	table->offset = start * THAMES_ABI_LINE_SIZE;
> +	list_add_tail(&table->node, &tdev->transfer_tables);
> +	return 0;
> +}

[ ... ]

> +	table->count = transfers->count;
> +	table->size = THAMES_TRANSFER_TABLE_SIZE(table->count, table->group_count);
> +
> +	ret = thames_transfers_alloc_table(tdev, table);
> +	if (ret) {
> +		table->size = 0;
> +		goto err_free;
> +	}
> +
> +	table->records = out;
> +	table->group_counts = group_counts;
> +
> +	for (i = 0; i < tdev->num_cores; i++)
> +		thames_transfers_write_table(&tdev->cores[i], table);

[Severity: High]
What happens if thames_core_resume() calls thames_transfers_rewrite()
concurrently with thames_transfers_build() right after
thames_transfers_alloc_table() releases tdev->transfer_table_pool_lock?

In drivers/accel/thames/thames_transfers.c, thames_transfers_alloc_table()
adds table->node to tdev->transfer_tables and drops
tdev->transfer_table_pool_lock before thames_transfers_build() sets
table->records = out and table->group_counts = group_counts:

CPU 0                                   CPU 1
thames_transfers_build()
  thames_transfers_alloc_table()
    guard(mutex)(&tdev->transfer_table_pool_lock);
    list_add_tail(&table->node, &tdev->transfer_tables);
    /* unlocks transfer_table_pool_lock */
                                        thames_core_resume()
                                          thames_transfers_rewrite()
                                            guard(mutex)(&tdev->transfer_table_pool_lock);
                                            thames_transfers_write_table(core, table)
                                              /* dereferences NULL table->records
                                               * and table->group_counts */
  table->records = out;
  table->group_counts = group_counts;

Should table->records and table->group_counts be populated before adding the
table to tdev->transfer_tables (and the initial thames_transfers_write_table()
calls performed under tdev->transfer_table_pool_lock)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-thames-v3-0-3ab40aa17157@tomeuvizoso.net?part=8