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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] `mbedtee_supp_recv` leaks `tee_shm` references acquired by the generic TEE core when parameter validation fails.
- [High] Skipping a partially arrived, invalid async RPC payload drops available bytes and permanently desynchronizes the TEE-to-REE RPC ring buffer.
- [High] Unclaimed TEE supplicant requests are never aborted when the supplicant process exits, causing TEE threads to hang indefinitely.
- [High] The driver calls `xa_insert_irq()` with `GFP_KERNEL`, which can sleep while holding an irq-disabled spinlock, causing a BUG.
--

--- Patch [3]: [PATCH v4 3/3] tee: add MbedTEE driver ---
commit a9ea3b44496edc967794a668ad0886985a5a2659
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/rpc_callee.c b/drivers/tee/mbedtee/rpc_callee.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tee/mbedtee/rpc_callee.c
[ ... ]
> +	if (cmd.waiter_id == 0 &&
> +	    (cmd.size > ctx->t2r_ring_sz - sizeof(cmd) ||
> +	     t2r_available_size(ctx) < sizeof(cmd) + cmd.size)) {
> +		t2r_ring_advance(ctx, t2r_available_size(ctx));
> +		return NULL;
> +	}

[Severity: High]
Does skipping a partially arrived, invalid async RPC payload like this
permanently break the ring buffer framing? By advancing only the available
bytes instead of waiting for the full payload to arrive, the rest of the
payload might be read later as a new command header.

> diff --git a/drivers/tee/mbedtee/rpc_caller.c b/drivers/tee/mbedtee/rpc_caller.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tee/mbedtee/rpc_caller.c
[ ... ]
> +int mbedtee_rpc_call_alloc(struct mbedtee_device *mbedtee,
> +			   size_t payload_size, struct mbedtee_rpc_call **call)
> +{
[ ... ]
> +	mutex_lock(&mbedtee->rpc_calls_mutex);
> +	if (mbedtee->stopping) {
> +		mutex_unlock(&mbedtee->rpc_calls_mutex);
> +		kfree(rcall);
> +		return -ESHUTDOWN;
> +	}
> +	ret = xa_insert_irq(&mbedtee->rpc_calls, rpc_index, rcall, GFP_KERNEL);

[Severity: High]
Can this trigger a sleeping in atomic context warning? xa_insert_irq()
disables interrupts internally, but GFP_KERNEL allows sleeping. Since the
xarray was initialized with XA_FLAGS_LOCK_IRQ, should this use the standard
xa_insert() instead?

> diff --git a/drivers/tee/mbedtee/supp.c b/drivers/tee/mbedtee/supp.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tee/mbedtee/supp.c
[ ... ]
> +void mbedtee_supp_release(struct mbedtee_supp *supp, struct tee_context *ctx)
> +{
> +	struct mbedtee_supp_req *r, *n;
> +
> +	mutex_lock(&supp->mutex);
> +
> +	/*
> +	 * Cancel only requests owned by @ctx.  Unclaimed requests and
> +	 * requests owned by other supplicant contexts are left alone.
> +	 */
> +	list_for_each_entry_safe(r, n, &supp->reqs, node) {
> +		if (r->ctx != ctx)
> +			continue;
> +		mbedtee_supp_complete_req(r);
> +	}

[Severity: High]
Are unclaimed requests ever aborted if the supplicant process exits early?
When a request is added to supp->reqs, its ctx is NULL until popped by
the supplicant. If the supplicant exits before popping, r->ctx != ctx
will be true and the request is skipped, which might leave the TEE thread
blocked indefinitely.

[ ... ]
> +int mbedtee_supp_recv(struct tee_context *ctx,
> +		      u32 *func, 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_context_data *d = ctx->data;
> +	struct tee_shm *shm;
> +	int ret;
> +
> +	ret = mbedtee_supp_check_recv_params(*num_params, param);
> +	if (ret != 0)
> +		return ret;

[Severity: High]
Does this code leak tee_shm references when parameter validation fails?
The generic TEE core invokes tee_shm_get_from_id() for shared memory
parameters before calling this function, expecting the driver to clean
them up on error. If untrusted userspace passes invalid parameters
along with valid shared memory references, it could intentionally trigger
this resource leak.

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