Re: [PATCH RFC 1/3] virt: coco: introduce tsm_default_tm() and tsm_mr_read()/write()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] virt: coco: UAF in tsm_mr_read/write due to premature lock release
- [High] virt: coco: mutating const parameter causes page fault on rodata
--

commit 9580da40a9c2580df1ab20f035b99adff1efdc0d
Author: Yeoreum Yun <yeoreum.yun@arm.com>

virt: coco: introduce tsm_default_tm() and tsm_mr_read()/write()

This patch introduces new APIs to allow reading and extending TSM measurement
registers from IMA. It adds an IDR mechanism to track measurement contexts
so that IMA can retrieve and manipulate them using the new exported symbols.

> diff --git a/drivers/virt/coco/guest/tsm-mr.c b/drivers/virt/coco/guest/tsm-mr.c
> index 657b9c5739d0d..9e721348be8d3 100644
> --- a/drivers/virt/coco/guest/tsm-mr.c
> +++ b/drivers/virt/coco/guest/tsm-mr.c

[ ... ]

> @@ -230,6 +251,15 @@ tsm_mr_create_attribute_group(const struct tsm_measurements *tm)
>  	ctx->agrp.name = "measurements";
>  	ctx->agrp.bin_attrs = no_free_ptr(attrs);
>  	ctx->tm = tm;
> +
> +	guard(mutex)(&idr_lock);
> +	((struct tsm_measurements *)tm)->ctx_id = idr_alloc(&tm_ctx_idr, ctx, 0,
> +							    TM_NUM_CTX, GFP_KERNEL);

[Severity: High]
Does casting away the const qualifier here cause a write protection violation?

If a provider driver passes a struct tsm_measurements template that is
statically allocated in read-only memory (.rodata), this write to ctx_id
will trigger a kernel oops page fault.

> +	if (tm->ctx_id < 0) {
> +		kfree(ctx->agrp.bin_attrs);
> +		return ERR_PTR(tm->ctx_id);
> +	}
> +
>  	return &no_free_ptr(ctx)->agrp;
>  }

[ ... ]

> @@ -243,9 +273,90 @@ EXPORT_SYMBOL_GPL(tsm_mr_create_attribute_group);
>   */
>  void tsm_mr_free_attribute_group(const struct attribute_group *attr_grp)
>  {
> +	struct tm_context *ctx;
> +
>  	if (!IS_ERR_OR_NULL(attr_grp)) {
> +		ctx = container_of(attr_grp, struct tm_context, agrp);
> +		scoped_guard(mutex, &idr_lock)
> +			idr_remove(&tm_ctx_idr, ctx->tm->ctx_id);
>  		kfree(attr_grp->bin_attrs);
> -		kfree(container_of(attr_grp, struct tm_context, agrp));
> +		kfree(ctx);
>  	}
>  }

[ ... ]

> +int tsm_mr_read(const struct tsm_measurements *tm, int idx,
> +		u8 *digest, u32 digest_size)
> +{
> +	struct tm_context *ctx;
> +	const struct tsm_measurement_register *mr;
> +	int rc;
> +
> +	scoped_guard(mutex, &idr_lock)
> +		ctx = idr_find(&tm_ctx_idr, tm->ctx_id);
> +
> +	if (IS_ERR_OR_NULL(ctx))
> +		return -ENODEV;
> +
> +	if (!digest || (idx >= ctx->tm->nr_mrs) ||
> +	    (ctx->tm->mrs[idx].mr_size > digest_size) ||
> +	    !(ctx->tm->mrs[idx].mr_flags & TSM_MR_F_READABLE))
> +		return -EINVAL;
> +
> +	mr = &ctx->tm->mrs[idx];
> +
> +	rc = __tsm_mr_read(ctx, mr, (char *)digest, 0, mr->mr_size);

[Severity: High]
Can this result in a use-after-free if tsm_mr_free_attribute_group() runs
concurrently?

Since scoped_guard only applies to the single statement block (the idr_find
call), the idr_lock is released immediately afterward. If a concurrent device
unbind triggers tsm_mr_free_attribute_group() before __tsm_mr_read()
completes, the context could be removed from the IDR and freed via kfree(ctx)
while this function is still dereferencing it for bounds checks and the read
operation.

> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_mr_read);
> +
> +int tsm_mr_write(const struct tsm_measurements *tm, int idx,
> +		 u8 *digest, u32 digest_size)
> +{
> +	struct tm_context *ctx;
> +	const struct tsm_measurement_register *mr;
> +	int rc;
> +
> +	scoped_guard(mutex, &idr_lock)
> +		ctx = idr_find(&tm_ctx_idr, tm->ctx_id);
> +
> +	if (IS_ERR_OR_NULL(ctx))
> +		return -ENODEV;
> +
> +	if (!digest || (idx >= ctx->tm->nr_mrs) ||
> +	    !(ctx->tm->mrs[idx].mr_flags & TSM_MR_F_WRITABLE))
> +		return -EINVAL;
> +
> +	/* partial writes are not supported */
> +	if (ctx->tm->mrs[idx].mr_size != digest_size)
> +		return -EINVAL;
> +
> +	mr = &ctx->tm->mrs[idx];
> +
> +	rc = __tsm_mr_write(ctx, mr, (char *)digest, mr->mr_size);

[Severity: High]
Does this suffer from the same concurrent use-after-free issue as
tsm_mr_read()?

The scoped_guard releases idr_lock right after idr_find(), leaving ctx
vulnerable to being freed by tsm_mr_free_attribute_group() before the write
completes.

> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-ima_tgx_integration_v2-v1-0-722c35370548@arm.com?part=1