Re: [PATCH RFC v3 7/9] tee: optee: execute yielding RPMI calls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] optee: missing bounds check on rpc_arg->num_params allows OOB access
--

commit 5f5afbcfcd90452c0729b00f504945e1e4f52c26
Author: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>

tee: optee: execute yielding RPMI calls

This patch adds the RPMI yielding-call path used by the common OP-TEE
session operations. It submits the command and RPC argument buffers as
ranges within a shared memory parcel and handles RPC requests while the
call is suspended.

> diff --git a/drivers/tee/optee/rpmi_abi.c b/drivers/tee/optee/rpmi_abi.c
> index 8b90c3653e80d..9a6439e15a33f 100644
> --- a/drivers/tee/optee/rpmi_abi.c
> +++ b/drivers/tee/optee/rpmi_abi.c

[ ... ]

> +static int optee_rpmi_yielding_call(struct tee_context *ctx,
> +				    const struct optee_rpmi_call_req *req,
> +				    struct optee_msg_arg *rpc_arg,
> +				    bool system_thread)
> +{

[ ... ]

> +		result = le32_to_cpu(resp.result);
> +		if (result == OPTEE_RPMI_YIELDING_CALL_RETURN_DONE)
> +			goto done;
> +
> +		cond_resched();
> +		optee_rpmi_handle_rpc(ctx, optee, result, rpc_arg);

[Severity: High]
Does this code allow an out-of-bounds access if the firmware returns a
maliciously large num_params?

In optee_rpmi_do_call_with_arg() below, the RPC buffer is bounded in shared
memory by optee->rpc_param_count.

However, when the firmware populates rpc_arg and optee_rpmi_handle_rpc()
is called here, the num_params field isn't validated. The call chain
optee_rpmi_handle_rpc()->optee_rpmi_handle_rpc_cmd()->optee_rpc_cmd() reaches
handle_rpc_supp_cmd() in drivers/tee/optee/rpc.c, which uses the unvalidated
num_params:

	params = kmalloc_objs(struct tee_param, arg->num_params);
	if (!params) {
		arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
		return;
	}

	if (optee->ops->from_msg_param(optee, params, arg->num_params,
				       arg->params)) {

This processes the RPC arguments using the firmware-supplied arg->num_params,
which can lead to out-of-bounds reads and writes on the arg->params shared
memory array if arg->num_params > optee->rpc_param_count.

[ ... ]

> +static int optee_rpmi_do_call_with_arg(struct tee_context *ctx,
> +				       struct tee_shm *shm, u_int offs,
> +				       bool system_thread)
> +{
> +	struct optee *optee = tee_get_drvdata(ctx->teedev);
> +	struct optee_msg_arg *arg, *rpc_arg;

[ ... ]

> +	arg_size = OPTEE_MSG_GET_ARG_SIZE(arg->num_params);
> +	rpc_size = OPTEE_MSG_GET_ARG_SIZE(optee->rpc_param_count);
> +	rpc_offset = offs + arg_size;
> +	rpc_arg = tee_shm_get_va(shm, rpc_offset);
> +	if (IS_ERR(rpc_arg))
> +		return PTR_ERR(rpc_arg);
> +
> +	req.arg_offset = cpu_to_le64((u64)shm->offset + offs);
> +	req.rpc_offset = cpu_to_le64((u64)shm->offset + rpc_offset);
> +	req.arg_size = cpu_to_le32(arg_size);
> +	req.rpc_size = cpu_to_le32(rpc_size);
> +
> +	return optee_rpmi_yielding_call(ctx, &req, rpc_arg, system_thread);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-rpmi-tee-service-grp-dev-v3-0-975b574667e2@oss.qualcomm.com?part=7