Re: [PATCH v2 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] Malicious or malformed asynchronous supplicant RPCs will execute sleeping operations in a hard IRQ handler, causing deadlocks.
- [Critical] The `req->shm` reference is under-retained in `mbedtee_supp_recv()`, leading to a Use-After-Free and double free.
- [Critical] Unvalidated `cmd.size` for rejected async RPCs causes the consumer index to exceed ring buffer bounds, leading to massive memory corruption.
- [Critical] Type confusion privilege escalation via TEE-controlled parameter attributes.
- [High] Unhandled MSI migration on RISC-V causes lost TEE interrupts and hanging RPCs.
- [High] Physical address of the RPC command is silently truncated on 32-bit ARM with LPAE.
- [High] The tee-supplicant's return code is lost and never propagated back to the TEE.
- [High] Module removal deadlocks because the T2R interrupt is torn down before aborting supplicant requests.
- [High] Initial ring buffer consumer and producer pointers are read from untrusted shared memory without bounds checking.
- [High] Unaligned memory access in `mbedtee_ree_time` can trigger alignment faults and kernel panics.
- [Medium] The required buffer size is not returned to the tee-supplicant when -EOVERFLOW occurs.
- [Medium] Leaking TEE sessions if a context acts as both a client and a supplicant.
- [Medium] Encountering an invalid or unhandled RPC command in the TEE-to-REE ring causes a state machine stall.
--

commit e35eb394dcd64d2ef130e20e3614fccbb5021a07
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 over architecture-specific transports.

> 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
> @@ -80,5 +80,6 @@ static void mbedtee_release(struct tee_context *ctx)
>  
>  	if (d->is_supp_ctx) {
>  		mbedtee_supp_release(&mbedtee->supp, ctx);
>  	} else {
>  		list_for_each_entry_safe(s, n, &d->sess_list, list_node)
>  			mbedtee_close_session(ctx, s->session_id);
>  	}

[Severity: Medium]
Does this skip session cleanup for mixed-use contexts? If a userspace
process opens standard TEE sessions and then calls TEE_IOC_SUPPL_RECV,
d->is_supp_ctx becomes true. When the file descriptor is closed, the
mutually exclusive else branch is skipped, which appears to leak the
sessions in d->sess_list and leaves them permanently open in the TEE.

> 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
> @@ -245,6 +245,5 @@ static struct rpc_work *rpc_pick_next(struct mbedtee_device *mbedtee,
>  skip:
>  	if (cmd.waiter_id != 0 && !rpc_queue_complete_only(ctx, cmd.waiter_id))
>  		return NULL;
>  
>  	t2r_ring_advance(ctx, sizeof(cmd) + (cmd.waiter_id ? 0 : cmd.size));
>  
>  	return NULL;
>  }

[Severity: Critical]
Does this blind advance allow a malicious TEE to corrupt the ring read
pointer? Because cmd.size is not validated on this error path, an arbitrarily
large value could cause t2r_ring_advance() to jump far outside the actual
ring bounds, leading to integer underflows and out-of-bounds memory accesses
in subsequent operations.

[Severity: Medium]
Does returning NULL here cause the rpc_drain_ring() loop to break entirely?
If an unrecognized command is skipped, returning NULL will unconditionally
stop the processing loop, leaving subsequent valid commands stranded until
another interrupt fires, which could stall the protocol.

> @@ -267,7 +267,6 @@ static void rpc_drain_ring(struct mbedtee_device *mbedtee)
>  		c = rpc_pick_next(mbedtee, ctx, &rw);
>  		if (!c)
>  			break;
>  
>  		if (c->waiter_id == 0) {
>  			c->func(mbedtee, c->data, c->size);
>  		} else {

[Severity: Critical]
Can this sequence cause a deadlock in hard IRQ context? The rpc_drain_ring()
function runs in mbedtee_rpc_irq_handler() with the ctx->ring_lock spinlock
held. If a TEE sends a synchronous supplicant RPC like MBEDTEE_RPC_REEFS with
waiter_id = 0, this inline execution will route to mbedtee_supp_handler(),
which performs a GFP_KERNEL allocation and calls wait_for_completion().
Sleeping in a hard IRQ handler while holding a spinlock will crash the system.

> @@ -400,6 +400,5 @@ static void mbedtee_ree_time(struct mbedtee_device *mbedtee,
>  			     void *data, size_t size)
>  {
>  	struct timespec64 *ts = data;
>  
>  	if (size < sizeof(*ts))
>  		return;
>  
>  	ktime_get_real_ts64(ts);
>  }

[Severity: High]
Does this cause unaligned memory accesses? The data pointer points directly
into shared memory at an offset controlled by the TEE. Writing 64-bit fields
into an unaligned timespec64 structure can trigger alignment faults and
kernel panics on architectures with strict alignment requirements.

> @@ -486,4 +486,5 @@ int mbedtee_rpc_init(struct mbedtee_device *mbedtee)
>  		return -EINVAL;
>  	}
>  	ctx->t2r_ring_sz = resource_size(&res) - sizeof(struct rpc_ringbuf);
>  	/* Observe latest producer index only after ring metadata is visible. */
>  	WRITE_ONCE(ctx->t2r_ring_rd, smp_load_acquire(&ctx->t2r_ring->rd));

[Severity: High]
Are we missing a bounds check on the initial read pointer? Loading this
directly from TEE-controlled shared memory without validating that it is
within ctx->t2r_ring_sz could allow a compromised TEE to instantly trigger
out-of-bounds accesses during driver initialization.

> @@ -585,5 +585,6 @@ void mbedtee_rpc_uninit(struct mbedtee_device *mbedtee)
>  	mutex_unlock(&mbedtee_cpuhp_mutex);
>  
>  	if (mbedtee->rpc_ops)
>  		mbedtee->rpc_ops->uninit(mbedtee);
>  
>  	/*
>  	 * Work queued before IRQ teardown may be blocked in the supplicant
>  	 * path. Abort those requests and reject later ones before draining the
>  	 * workqueue. R2T must stay alive until destroy_workqueue() returns
>  	 * because rpc_routine() sends COMPLETE_TEE replies over R2T.
>  	 */
>  	mbedtee_supp_abort_all(&mbedtee->supp);

[Severity: High]
Will this sequence deadlock during module removal? Unregistering the interrupt
via uninit() happens before mbedtee_supp_abort_all(). If aborting requests
triggers shm unregistration, that process makes a synchronous RPC call to the
TEE. Since the interrupt is already torn down, the completion response will
never arrive, causing the removal thread to block indefinitely.

> diff --git a/drivers/tee/mbedtee/rpc_callee_riscv.c b/drivers/tee/mbedtee/rpc_callee_riscv.c
> --- a/drivers/tee/mbedtee/rpc_callee_riscv.c
> +++ b/drivers/tee/mbedtee/rpc_callee_riscv.c
> @@ -34,6 +34,5 @@ static void mbedtee_rpc_write_msi_msg(struct msi_desc *desc_msi,
>  				      struct msi_msg *msg)
>  {
>  	struct mbedtee_device *mbedtee = dev_get_drvdata(desc_msi->dev);
>  
>  	if (mbedtee)
>  		mbedtee->transport.rpc_msi_msg = *msg;
>  }

[Severity: High]
Does this fail to propagate the new IMSIC target to the TEE? When the IRQ
subsystem changes affinity, this callback updates the local message cache,
but it does not write the new callee_imsic_id and callee_hartid back to the
shared ring buffer header. The TEE will continue sending interrupts to the
stale target, leading to lost notifications and hanging RPCs.

> 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
> @@ -182,5 +182,6 @@ static int mbedtee_param_decode(struct tee_param *params,
>  		case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
>  			p->u.memref.size = rtp->memref.size;
>  			break;
>  		default:
>  			return -EINVAL;
>  		}
>  		p->attr = attr;
>  	}
>  	return 0;
>  }

[Severity: Critical]
Could this lead to type confusion and privilege escalation? Blindly overwriting
p->attr with the TEE-supplied attribute without verifying it matches the
original request allows a malicious TEE to change a VALUE parameter to a
MEMREF parameter. The generic subsystem will then interpret the user-controlled
value fields as a kernel pointer during tee_shm_put(), allowing arbitrary
kernel memory dereferences.

> diff --git a/drivers/tee/mbedtee/rpc_caller_arm.c b/drivers/tee/mbedtee/rpc_caller_arm.c
> --- a/drivers/tee/mbedtee/rpc_caller_arm.c
> +++ b/drivers/tee/mbedtee/rpc_caller_arm.c
> @@ -30,5 +30,6 @@ long mbedtee_rpc_yieldcall(struct mbedtee_device *mbedtee,
>  	call->rpc.id = fn;
>  
>  	arm_smccc_smc(fn, call->rpc_phys,
>  		      0, 0, 0, 0, 0, 0, &res);
>  	ret = res.a0;

[Severity: High]
Does this silently truncate the physical address on 32-bit ARM with LPAE?
The call->rpc_phys field is a 64-bit phys_addr_t, but arm_smccc_smc() casts
its arguments to a 32-bit unsigned long. If the memory resides above 4GB,
the upper bits will be lost, giving the TEE an invalid address and causing
memory corruption or crashes.

> 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
> @@ -166,4 +166,5 @@ int mbedtee_r2t_init(struct mbedtee_device *mbedtee)
>  		return -ENOMEM;
>  	}
>  	ctx->ring_sz = resource_size(&res) - sizeof(struct rpc_ringbuf);
>  	/* Read initial writer index with acquire for coherent ring bootstrap. */
>  	WRITE_ONCE(ctx->ring_wr, smp_load_acquire(&ctx->ring->wr));

[Severity: High]
Is it safe to load the initial write pointer from shared memory without bounds
validation? A compromised TEE could set this to an out-of-bounds value prior
to driver initialization, leading to immediate out-of-bounds memory accesses.

> 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
> @@ -153,6 +153,5 @@ void mbedtee_supp_handler(struct mbedtee_device *mbedtee,
>  	/* Wake up the supplicant daemon to handle this request. */
>  	complete(&supp->reqs_c);
>  
>  	wait_for_completion(&req->c);
>  
>  	kfree(req);
>  }

[Severity: High]
Are we missing a step to propagate the supplicant's return code back to the TEE?
After wait_for_completion() returns, req->ret holds the execution status, but
this value is never written into the shared memory cmd->ret before req is
freed, causing the TEE to lose the actual result of the operation.

> @@ -203,6 +203,5 @@ int mbedtee_supp_recv(struct tee_context *ctx,
>  		return ret;
>  
>  	shm = tee_shm_get_from_id(ctx, shm->id);
>  	tee_shm_put(param->u.memref.shm);
>  	if (IS_ERR(shm)) {

[Severity: Critical]
Does this pattern lead to a use-after-free and double free? The code acquires
a generic SHM reference and immediately drops the previous reference. This
results in a net zero reference count change, meaning the driver does not truly
own the reference it assigns to req->shm. When the IOCTL finishes, the generic
layer will put the reference again, leaving req->shm dangling.

> @@ -231,9 +231,8 @@ int mbedtee_supp_recv(struct tee_context *ctx,
>  	req->ctx = ctx;
>  
>  	if (req->size > param->u.memref.size) {
>  		/* 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;
>  	}

[Severity: Medium]
Is the required buffer size missing from the -EOVERFLOW response?
If the user-provided buffer is too small, the driver returns -EOVERFLOW but
fails to update param->u.memref.size with req->size. The userspace supplicant
daemon relies on this updated size to know how much memory to allocate for a
retry.

> @@ -298,5 +298,6 @@ int mbedtee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
>  		req->ret = ret;
>  		memcpy(req->data, req->shm->kaddr, param->u.memref.size);
>  	}
>  	if (req->shm)
>  		tee_shm_put(req->shm);
>  	req->shm = NULL;

[Severity: Critical]
Because the reference was not correctly incremented in mbedtee_supp_recv(),
calling tee_shm_put(req->shm) here drops a reference that belongs to the
userspace process, completing the double free.

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