Re: [PATCH v3 3/3] tee: add MbedTEE driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] Unvalidated cmd.size in the rpc_pick_next error path causes kernel stack buffer overflow.
- [High] xa_insert_irq() called with GFP_KERNEL causes sleeping in atomic context.
- [High] Early parameter validation failure in mbedtee_supp_send() hangs kernel worker threads.
- [High] Malicious TEE can deadlock the kernel by sending supplicant requests as async RPCs.
- [High] RPC call object is accessed after being freed when wait_for_completion is interrupted.
- [High] UAF and list corruption in mbedtee_supp_recv due to unprotected access.
- [High] Unvalidated parameter attributes from the TEE allow an attacker to permanently leak shared memory object references.
- [Medium] RISC-V yield calls fail spuriously with -ENOSPC when the REE-to-TEE ring is transiently full.
- [Medium] The required buffer size is not returned to userspace when a supplicant buffer is too small.
- [Medium] Infinite retry loop in workqueue on TEE failure blocks teardown.
- [Medium] Leaked RPC calls on module removal due to xa_destroy without element freeing.
- [Medium] Data race on complete_work_pending between IRQ and workqueue contexts.
- [Medium] Transient memory allocation failure in rpc_pick_next stalls the ring buffer permanently.
--

--- Patch [3]: [PATCH v3 3/3] tee: add MbedTEE driver ---
commit a7be6b832a5cd2c0d320336aea6907717da8717c
Author: Xing Loong <xing.xl.loong@gmail.com>

tee: add MbedTEE driver

This patch adds a Linux TEE driver that communicates with MbedTEE OS via a
fixed shared-memory RPC protocol. It supports ARM TrustZone and RISC-V IMSIC
platforms.

> diff --git a/drivers/tee/mbedtee/core.c b/drivers/tee/mbedtee/core.c
> --- a/drivers/tee/mbedtee/core.c
> +++ b/drivers/tee/mbedtee/core.c

[ ... ]

> @@ -198,16 +198,15 @@ static void mbedtee_remove(struct platform_device *pdev)
>  {
>  	struct mbedtee_device *mbedtee = platform_get_drvdata(pdev);
>  
>  	tee_device_unregister(mbedtee->teedev);
>  	/*
>  	 * Drain T2R before tearing down R2T: queued RPC work may still need
>  	 * to send COMPLETE_TEE over R2T. Destroy the supplicant mutex only
>  	 * after RPC work can no longer enter mbedtee_supp_handler().
>  	 */
>  	mbedtee_rpc_uninit(mbedtee);
>  	mbedtee_r2t_uninit(mbedtee);
>  	mbedtee_supp_uninit(&mbedtee->supp);
>  	tee_shm_pool_free(mbedtee->pool);
>  	xa_destroy(&mbedtee->rpc_calls);

[Severity: Medium]
Are the elements in the xarray leaked on module removal? Calling xa_destroy
frees the internal nodes of the xarray, but it does not iterate over and free
the elements themselves. Any RPC calls left in the MBEDTEE_RPC_CALL_INTERRUPTED
state would be permanently leaked.

>  }

> diff --git a/drivers/tee/mbedtee/rpc_callee.c b/drivers/tee/mbedtee/rpc_callee.c
> --- a/drivers/tee/mbedtee/rpc_callee.c
> +++ b/drivers/tee/mbedtee/rpc_callee.c

[ ... ]

> @@ -148,15 +148,15 @@ static void t2r_ring_read(struct mbedtee_t2r_ctx *ctx,
>  }
>  
>  static bool rpc_queue_complete_only(struct mbedtee_t2r_ctx *ctx,
>  				    u64 waiter_id)
>  {
>  	if (ctx->complete_work_pending)

[Severity: Medium]
Is there a data race on complete_work_pending here? This variable is read as a
plain C variable in this IRQ context, but it is modified using WRITE_ONCE in
rpc_routine from a workqueue context. This could allow the compiler to cache
or reorder the read, potentially leaving the driver perpetually thinking a
completion work is pending.

>  		return false;
>  
>  	ctx->complete_work_pending = true;
>  	ctx->complete_work.waiter_id = waiter_id;
>  	queue_work(ctx->rpc_wq, &ctx->complete_work.work);
>  
>  	return true;
>  }

[ ... ]

> @@ -215,9 +215,9 @@ static struct rpc_work *rpc_pick_next(struct mbedtee_device *mbedtee,
>  
>  		if (off > ctx->t2r_shm_sz - cmd.size)
>  			goto skip;
>  
>  		new_work = kzalloc_obj(*new_work, GFP_ATOMIC);
>  		if (!new_work)
>  			return NULL;

[Severity: Medium]
Does a transient memory allocation failure here permanently stall the ring
buffer? If kzalloc_obj fails with GFP_ATOMIC, the function returns NULL and
rpc_drain_ring breaks out of its loop without advancing the read pointer.
The TEE thread would block forever since the entry remains unprocessed and no
retry is scheduled.

>  
>  		t2r_ring_advance(ctx, sizeof(cmd));

[ ... ]

> @@ -279,11 +279,11 @@ skip:
>  	if (cmd.waiter_id != 0 && !rpc_queue_complete_only(ctx, cmd.waiter_id))
>  		return NULL;
>  
>  	/*
>  	 * Bad or unhandled entry: consume header plus any inline payload
>  	 * (async RPC payloads follow the header in the ring) to keep the
>  	 * ring moving.  For sync RPCs payload is in t2r_shm, not the ring.
>  	 */
>  	t2r_ring_advance(ctx, sizeof(cmd) + (cmd.waiter_id ? 0 : cmd.size));

[Severity: Critical]
Can this advance the read pointer by an unvalidated size and cause a stack
buffer overflow? In rpc_pick_next, if an async RPC jumps to the skip label,
t2r_ring_advance is called with 32 + cmd.size without checking if cmd.size
exceeds the ring buffer capacity. 

If a malicious TEE sets a very large cmd.size, the read pointer wraps. On the
next iteration, t2r_available_size underflows and t2r_ring_copy executes
a memcpy of up to 4GB directly into the cmd struct on the kernel stack.

>  
>  	return NULL;
>  }

[ ... ]

> @@ -296,14 +296,14 @@ static void rpc_drain_ring(struct mbedtee_device *mbedtee)
>  	spin_lock_irqsave(&ctx->ring_lock, flags);
>  	while (READ_ONCE(ctx->t2r_ring_rd) !=
>  	    /* Pair with producer store-release after ring write. */
>  	    smp_load_acquire(&ctx->t2r_ring->wr)) {
>  		rw.data = ctx->rpc_data;
>  		c = rpc_pick_next(mbedtee, ctx, &rw);
>  		if (!c)
>  			break;
>  
>  		if (c->waiter_id == 0) {
>  			c->func(mbedtee, c->data, c->size);

[Severity: High]
Is it possible for a malicious TEE to deadlock the kernel by sending
supplicant requests as async RPCs? The TEE-to-REE interrupt handler runs in
hard IRQ context, and rpc_drain_ring relies entirely on cmd.waiter_id == 0
to process a command inline. 

If the TEE sends MBEDTEE_RPC_REEFS with waiter_id = 0, the driver invokes
mbedtee_supp_handler inline while holding the ring_lock. That function
allocates memory with GFP_KERNEL and calls wait_for_completion, both of
which sleep in atomic context.

>  		} else {
>  			INIT_WORK(&c->work, rpc_routine);
>  			queue_work(ctx->rpc_wq, &c->work);
>  		}
>  	}
>  	spin_unlock_irqrestore(&ctx->ring_lock, flags);
>  }

[ ... ]

> @@ -339,12 +339,12 @@ static void rpc_routine(struct work_struct *work)
>  	 * Keep retrying rather than silently dropping the completion, which
>  	 * would leave the TEE thread blocked in rpc_call_sync forever.
>  	 * On ARM the fastcall is a direct SMC so -ENOSPC never occurs.
>  	 */
>  	do {
>  		ret = mbedtee_rpc_fastcall(c->mbedtee, MBEDTEE_RPC_COMPLETE_TEE,
>  					   (unsigned long)c->waiter_id, 0, 0);
>  		if (ret != -ENOSPC && ret != -ENOMEM)
>  			break;
>  		cond_resched();
>  	} while (1);

[Severity: Medium]
Could this loop infinitely if the ring buffer remains full? If
mbedtee_rpc_fastcall repeatedly returns -ENOSPC (which occurs on RISC-V when
the R2T ring is full), it retries indefinitely. If the TEE crashes and stops
draining the ring, this loop never terminates, and destroy_workqueue during
module removal would block forever waiting for the hung work item.

>  
>  	if (c != &ctx->complete_work) {

> diff --git a/drivers/tee/mbedtee/rpc_caller.c b/drivers/tee/mbedtee/rpc_caller.c
> --- a/drivers/tee/mbedtee/rpc_caller.c
> +++ b/drivers/tee/mbedtee/rpc_caller.c

[ ... ]

> @@ -35,12 +35,12 @@ int mbedtee_rpc_call_alloc(struct mbedtee_device *mbedtee,
>  	rcall->rpc.ret = -EOPNOTSUPP;
>  	rcall->rpc_phys = mbedtee_virt_to_phys(&rcall->rpc);
>  
>  	do {
>  		rpc_index = atomic_long_inc_return(&mbedtee->rpc_call_seq);
>  	} while (rpc_index == 0);
>  
>  	ret = xa_insert_irq(&mbedtee->rpc_calls, rpc_index, rcall, GFP_KERNEL);

[Severity: High]
Does this code sleep in atomic context? The driver initializes
mbedtee->rpc_calls with XA_FLAGS_ALLOC1. Calling xa_insert_irq with
GFP_KERNEL drops the spinlock but leaves hardware interrupts disabled, then
calls kmem_cache_alloc with GFP_KERNEL which can sleep, violating atomic
context rules.

>  	if (ret != 0) {
>  		kfree(rcall);
>  		return ret;
>  	}

[ ... ]

> @@ -170,14 +170,14 @@ static int mbedtee_param_decode(struct tee_param *params,
>  		case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT:
>  		case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
>  		case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
>  			p->u.memref.size = rtp->memref.size;
>  			break;
>  		default:
>  			return -EINVAL;
>  		}
>  		p->attr = attr;

[Severity: High]
Can a malicious TEE permanently leak shared memory object references by
returning unvalidated parameter attributes? The kernel decodes the parameter
types returned by the TEE and unconditionally overwrites the local p->attr
with the TEE's value. 

If userspace passes a MEMREF, a compromised TEE could return a VALUE type for
that slot, causing the TEE subsystem to skip dropping the reference during
cleanup because tee_param_is_memref will see a VALUE type instead.

>  	}
>  	return 0;
>  }

[ ... ]

> @@ -415,12 +415,12 @@ int mbedtee_invoke_func(struct tee_context *ctx,
>  	if (ret) {
>  		mbedtee_rpc_call_free(mbedtee, call);
>  		return ret;
>  	}
>  
>  	ret = mbedtee_rpc_yieldcall(mbedtee, MBEDTEE_RPC_INVOKE_SESSION, call, true);
>  
>  	dev_dbg(mbedtee->dev, "invoke session ret %d gp_ret %d\n", ret,
>  		call->rpc.ret);

[Severity: High]
Does this introduce a use-after-free if wait_for_completion is interrupted? If
mbedtee_rpc_wait_for_completion is interrupted by a signal, it sets the
state to MBEDTEE_RPC_CALL_INTERRUPTED and returns -ERESTARTSYS. The caller
here then reads call->rpc.ret and calls mbedtee_rpc_call_free.

Concurrently, mbedtee_rpc_complete_call can see the INTERRUPTED state, erase
the XArray entry, and free the call structure, causing the original caller to
perform UAF reads.

>  
>  	if (ret != 0) {
>  		arg->ret = TEEC_ERROR_COMMUNICATION;

> diff --git a/drivers/tee/mbedtee/rpc_caller_riscv.c b/drivers/tee/mbedtee/rpc_caller_riscv.c
> --- a/drivers/tee/mbedtee/rpc_caller_riscv.c
> +++ b/drivers/tee/mbedtee/rpc_caller_riscv.c

[ ... ]

> @@ -75,10 +75,10 @@ long mbedtee_rpc_yieldcall(struct mbedtee_device *mbedtee,
>  	call->rpc.id = fn;
>  
>  	spin_lock_irqsave(&ctx->lock, flags);
>  	if (!rpc_ring_enough(ctx, sizeof(u64))) {
>  		dev_err_ratelimited(mbedtee->dev, "rpc ring full\n");
>  		spin_unlock_irqrestore(&ctx->lock, flags);
>  		return -ENOSPC;

[Severity: Medium]
Should this implement a wait queue or backoff instead of failing immediately?
On RISC-V, mbedtee_rpc_yieldcall returns -ENOSPC if the r2t-ring buffer is
full, which mbedtee_invoke_func translates to TEEC_ERROR_COMMUNICATION.
Without a polling backoff or wait mechanism, high concurrency from userspace
will easily fill the ring and cause spurious, unrecoverable failures.

>  	}
>  
>  	phys = call->rpc_phys;

> diff --git a/drivers/tee/mbedtee/supp.c b/drivers/tee/mbedtee/supp.c
> --- a/drivers/tee/mbedtee/supp.c
> +++ b/drivers/tee/mbedtee/supp.c

[ ... ]

> @@ -232,18 +232,18 @@ int mbedtee_supp_recv(struct tee_context *ctx,
>  		if (wait_for_completion_interruptible(&supp->reqs_c)) {
>  			ret = -ERESTARTSYS;
>  			goto err;
>  		}
>  	}
>  
>  	/* Record which supplicant context owns this request. */
>  	req->ctx = ctx;
>  
>  	if (req->size > param->u.memref.size) {

[Severity: High]
Can this cause a use-after-free and linked list corruption? After
supp_pop_req removes a request and drops the mutex, the function accesses
req->ctx and req->size locklessly. 

If mbedtee_supp_abort_all runs concurrently (e.g. on module removal), it
could remove the request from active_reqs, complete it, and free it, causing
UAF reads here. If the size check fails, it then takes the mutex and
manipulates the list links of the freed node, corrupting the supp->reqs list.

[Severity: Medium]
Does this code fail to return the required buffer size to userspace? When
req->size is larger than param->u.memref.size, it correctly returns -EOVERFLOW
but fails to update param->u.memref.size with the required req->size. The
generic TEE framework relies on this updated value to inform userspace how
much memory to allocate, breaking buffer renegotiation.

>  		/* Return the request to the queue so it isn't lost */
>  		mutex_lock(&supp->mutex);
>  		list_del(&req->node);
>  		list_add(&req->node, &supp->reqs);
>  		mutex_unlock(&supp->mutex);
>  		ret = -EOVERFLOW;
>  		goto err;
>  	}

[ ... ]

> @@ -262,11 +262,11 @@ int mbedtee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
>  		      struct tee_param *param)
>  {
>  	struct tee_device *teedev = ctx->teedev;
>  	struct mbedtee_device *mbedtee = tee_get_drvdata(teedev);
>  	struct mbedtee_supp *supp = &mbedtee->supp;
>  	struct mbedtee_supp_req *req;
>  	struct mbedtee_supp_req *_req;
>  
>  	if (num_params != 1)
>  		return -EINVAL;

[Severity: High]
Could this lead to a permanently hung kernel worker thread? If userspace calls
TEE_IOC_SUPPL_SEND with invalid parameters (like num_params != 1),
mbedtee_supp_send returns -EINVAL early. Because it does not pop the active
request or complete it, the worker thread executing mbedtee_supp_handler
would remain permanently blocked in wait_for_completion.

>  
>  	if (!tee_param_is_memref(param))
>  		return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720073558.799755-1-xing.xl.loong@gmail.com?part=3