Re: [PATCH v7 1/7] optee: ffa: Add NULL check in optee_ffa_lend_protmem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Integer division truncation when validating the TEE's response size in `get_protmem_config` allows out-of-bounds memory accesses and NULL pointer dereferences.
- [High] A malicious or compromised TEE returning TEEC_SUCCESS with a non-zero attribute size when no buffer was supplied causes a NULL pointer dereference in `optee_ffa_lend_protmem`.
- [High] The `optee_protmem_dyn_pool` structure leaks its `mem_attrs` array when the pool is destroyed.
--

commit e70191e60cbc95a8fabdaddc766ad1d432febd3b
Author: Mostafa Saleh <smostafa@google.com>

optee: ffa: Add NULL check in optee_ffa_lend_protmem

Sashiko (locally) reports a possible null dereference under memory
pressure due to the lack of validation of the allocated pointer. Fix that
by adding the missing check.

> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> index b4372fa268d03..633715b98625c 100644
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -698,6 +698,9 @@ static int optee_ffa_lend_protmem(struct optee *optee, struct tee_shm *protmem,
>  	int rc;
>  
>  	mem_attr = kzalloc_objs(*mem_attr, ma_count);
> +	if (!mem_attr)
> +		return -ENOMEM;
> +
>  	for (n = 0; n < ma_count; n++) {
>  		mem_attr[n].receiver = mem_attrs[n] & U16_MAX;

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer
dereference of the mem_attrs array if the TEE returns malformed data?

If get_protmem_config() in drivers/tee/optee/protmem.c is called with
mem_attrs == NULL to probe the array size, and the TEE returns
TEEC_SUCCESS (0) with params[1].u.memref.size > 0:

drivers/tee/optee/protmem.c:get_protmem_config() {
	...
	*ma_count = params[1].u.memref.size / sizeof(*mem_attrs);
	...
}

This skips the TEEC_ERROR_SHORT_BUFFER check. The caller
optee_protmem_alloc_dyn_pool() sees rc == 0:

drivers/tee/optee/protmem.c:optee_protmem_alloc_dyn_pool() {
	...
	if (rc) {
		if (rc != -ENOSPC)
			goto err;
	...
}

This bypasses the allocation of rp->mem_attrs, leaving it NULL while
rp->mem_attr_count is > 0. When init_dyn_protmem() later calls
optee_ffa_lend_protmem(), doesn't the loop here dereference the NULL
mem_attrs array?

[Severity: High]
This isn't a bug introduced by this patch, but does
optee_protmem_dyn_pool leak its mem_attrs array?

In optee_protmem_alloc_dyn_pool(), rp->mem_attrs is dynamically
allocated:

drivers/tee/optee/protmem.c:optee_protmem_alloc_dyn_pool() {
	...
	rp->mem_attrs = kcalloc(rp->mem_attr_count,
	                        sizeof(*rp->mem_attrs), GFP_KERNEL);
	...
}

However, in the destruction path pool_op_dyn_destroy_pool(), it appears
the array is not freed before destroying the pool:

drivers/tee/optee/protmem.c:pool_op_dyn_destroy_pool() {
	...
	mutex_destroy(&rp->mutex);
	kfree(rp);
}

If dynamic pools are repeatedly created and destroyed, could this leak
memory on every destruction cycle?

[Severity: Critical]
This is a pre-existing issue, but does the size validation in
get_protmem_config() suffer from integer division truncation?

If the TEE returns a size that is not a multiple of the struct size
(e.g., 7 when *ma_count is 1), does this check fail to catch the invalid
size?

drivers/tee/optee/protmem.c:get_protmem_config() {
	...
	if (!msg_arg->ret && mem_attrs &&
	    *ma_count < params[1].u.memref.size / sizeof(*mem_attrs)) {
	...
}

Since 7 / 4 evaluates to 1, the condition 1 < 1 is false. Would this
bypass the error path and lead to an out-of-bounds memcpy?

drivers/tee/optee/protmem.c:get_protmem_config() {
	...
	if (mem_attrs)
		memcpy(mem_attrs, tee_shm_get_va(shm_param, 0),
		       params[1].u.memref.size);
	...
}

Additionally, on the initial probe where mem_attrs == NULL, if the TEE
returns a size of 3, rp->mem_attr_count becomes 0. On the second call,
kcalloc(0, 4) returns ZERO_SIZE_PTR. If the TEE again returns size 3,
the check evaluates to 0 < 0 (false). Would this result in passing NULL
to tee_shm_get_va() and causing a NULL pointer dereference inside that
function?

>  		mem_attr[n].attrs = mem_attrs[n] >> 16;
>  	}
>  	args.attrs = mem_attr;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617145130.3729015-1-sebastianene@google.com?part=1