Re: [PATCH RFC v3 4/9] tee: optee: add RPMI shared-memory and parameter support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tee: optee: OOB access in RPC handlers due to unvalidated memref size
- [High] tee: optee: NULL pointer dereference in RPC handlers from empty memref
- [Medium] tee: optee: IDA nonce leak on memory_share failure allows DoS
--

commit 75de307b7e8cbec8782ab18004851de96c3a4c73
Author: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>

tee: optee: add RPMI shared-memory and parameter support

OP-TEE commands and RPCs need shared memory that secure world can
identify through RPMI parcels.

Register normal-world pages as read-write parcels shared with the
OP-TEE endpoint. Track each parcel ID and nonce with the common
shared-memory map. Store the combined identity in tee_shm.sec_world_id
and allocate nonzero nonces to distinguish registered memory from NULL
references. Reuse nonces after successful parcel reclamation.

Add conversions between TEE parameters and parcel memory references.

On client memory unregistration, ask OP-TEE to release its mapping
before reclaiming the parcel. Supplicant memory has already been
released by OP-TEE through its SHM_FREE RPC and only needs reclaiming.

> diff --git a/drivers/tee/optee/rpmi_abi.c b/drivers/tee/optee/rpmi_abi.c
> new file mode 100644
> index 0000000000000..940676794280d
> --- /dev/null
> +++ b/drivers/tee/optee/rpmi_abi.c

[ ... ]

> +static int optee_rpmi_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> +				   struct page **pages, size_t num_pages,
> +				   unsigned long start)
> +{

[ ... ]

> +	rc = ida_alloc_min(&optee->rpmi.nonce_ida, 1, GFP_KERNEL);
> +	if (rc < 0) {
> +		sg_free_table(&sgt);
> +		return rc;
> +	}
> +
> +	args.nonce = rc;
> +	args.sg = sgt.sgl;
> +	rc = rdev->ops->mem_ops->memory_share(rdev, &args);
> +	sg_free_table(&sgt);
> +	/* A failed exchange may leave a parcel using this nonce. */
> +	if (rc)
> +		return rc;

[Severity: Medium]
Does this error path leak the allocated IDA nonce?

If memory_share() fails (for instance, when firmware rejects the request),
it appears the nonce allocated via ida_alloc_min() is returned without being
freed. Since TEE_IOC_SHM_REGISTER is accessible to unprivileged users, could
this be repeatedly triggered to exhaust the IDA space and cause a denial of
service for subsequent shared memory registrations?

> +
> +	global_id = optee_rpmi_global_id(args.parcel_id, args.nonce);

[ ... ]

> +/* Convert an RPMI parcel reference to a memref; callers own SHM lifetime. */
> +static int from_msg_param_rpmi_mem(struct optee *optee, struct tee_param *p,
> +				   u32 attr, const struct optee_msg_param *mp)
> +{
> +	struct tee_shm *shm;
> +	u64 offset;
> +
> +	p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + attr -
> +		  OPTEE_MSG_ATTR_TYPE_PMEM_INPUT;
> +
> +	if (mp->u.pmem.size > SIZE_MAX)
> +		return -EOVERFLOW;
> +	p->u.memref.size = mp->u.pmem.size;
> +
> +	if (!optee_rpmi_nonce(mp->u.pmem.global_id)) {
> +		/* Return NULL shm. */
> +		if (mp->u.pmem.offs)
> +			return -EINVAL;
> +		p->u.memref.shm = NULL;
> +		p->u.memref.shm_offs = 0;
> +		return 0;

[Severity: High]
Can a malformed RPC request with an empty PMEM argument cause a NULL pointer
dereference in the common RPC handlers?

If OP-TEE sends a request with a NULL reference (global_id == 0), this
function successfully sets shm to NULL and returns 0. When the common RPC
handlers in drivers/tee/optee/rpc.c (such as handle_rpc_func_rpmb_probe_next()
or handle_rpc_func_cmd_i2c_transfer()) process the parameters, they appear to
dereference the shm pointer or pass it to tee_shm_get_va() without checking if
it is valid.

> +	}
> +
> +	shm = optee_shm_handle_map_lookup(&optee->rpmi.shm_map,
> +					  mp->u.pmem.global_id);
> +	if (!shm || mp->u.pmem.offs < shm->offset)
> +		return -EINVAL;
> +
> +	offset = mp->u.pmem.offs - shm->offset;
> +	if (offset > SIZE_MAX)
> +		return -EOVERFLOW;
> +
> +	p->u.memref.shm = shm;
> +	p->u.memref.shm_offs = offset;

[Severity: High]
Are we missing bounds checking for the memory reference size and offset?

The size and offset from the OP-TEE message are assigned to the tee_param
without bounds validation. While this might be necessary to return required
buffer sizes in TEEC_ERROR_SHORT_BUFFER, downstream RPC handlers in
drivers/tee/optee/rpc.c (like handle_rpc_func_rpmb_probe_next() and
handle_rpc_func_rpmb_frames()) appear to trust these parameters blindly.

Could a compromised OP-TEE use this to trigger out-of-bounds reads and writes
in the kernel by causing the handlers to calculate out-of-bounds pointers via
tee_shm_get_va() and perform operations like memcpy()?

> +
> +	return 0;
> +}

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