Re: [PATCH RFC v2 09/10] firmware: riscv: rpmi-tee: add memory parcel operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] rpmi-tee: segment slot leak and goto mixing in rpmi_tee_parcel_create()
- [Low] cleanup: mixing goto and __free() in rpmi_tee_parcel_create()
--

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

firmware: riscv: rpmi-tee: add memory parcel operations

Add memory lending, sharing and reclamation through RPMI memory parcels
so service drivers can exchange memory with TEE endpoints. This patch
introduces two issues in rpmi_tee_parcel_create(): a mixture of __free()
and goto-based cleanup, and a resource leak caused by bypassing the cleanup
label.

> diff --git a/drivers/firmware/riscv_rpmi_tee/driver.c b/drivers/firmware/riscv_rpmi_tee/driver.c
> index b43a09f5e8536..690561ed949dc 100644
> --- a/drivers/firmware/riscv_rpmi_tee/driver.c
> +++ b/drivers/firmware/riscv_rpmi_tee/driver.c
[ ... ]
> @@ -326,6 +446,355 @@ static int rpmi_tee_op_call(struct rpmi_tee_device *rdev, const void *req,
[ ... ]
> +/* Create a memory parcel after the caller has validated its operation. */
> +static int rpmi_tee_parcel_create(struct rpmi_tee_transport *priv,
> +				  struct rpmi_tee_mem_args *args)
> +{
> +	struct rpmi_tee_parcel_create_resp resp;
> +	struct rpmi_tee_parcel_xfer xfer;
> +	size_t blk_off, req_len, resp_len;
> +	bool segmented;
> +	int ret;
> +	u32 i;
> +
> +	ret = rpmi_tee_count_blocks_sg(args->sg, &xfer.block_count);
> +	if (ret)
> +		return ret;
> +
> +	/* BLOCK_HIGH[] follows the request header and receiver arrays. */
> +	blk_off = RPMI_TEE_PARCEL_CREATE_SIZE + args->receiver_count *
> +		RPMI_TEE_PARCEL_CREATE_RECEIVER_INFO_SIZE;
> +
> +	/* Limit the initial request to the parcel's actual block count. */
> +	xfer.next_block = min((priv->mbox.max_msg_data_size - blk_off) /
> +			      RPMI_TEE_PARCEL_CREATE_BLOCK_SIZE,
> +			      xfer.block_count);
> +
> +	segmented = xfer.next_block < xfer.block_count;
> +	if (segmented) {
> +		if (!priv->mem.multisegment_max)
> +			return -EOPNOTSUPP;
> +		/* Reserve a slot against the firmware's advertised limit. */
> +		ret = rpmi_tee_reserve_segment_slot(priv);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	req_len = blk_off + RPMI_TEE_PARCEL_CREATE_BLOCK_SIZE * xfer.next_block;
> +
> +	struct rpmi_tee_parcel_create_req *req __free(kfree) =
> +		kzalloc(req_len, GFP_KERNEL);
> +	if (!req) {
> +		ret = -ENOMEM;
> +		goto out_release_slot;
> +	}

[Severity: Low]
Does this mix goto-based error handling with scope-based cleanup?

The cleanup.md subsystem guide explicitly forbids mixing goto-based cleanup
labels and __free() declarations in the same function because it creates
confusing ownership semantics and leak hazards.

Could this be refactored to either use scope-based cleanup for all resources or
stick entirely to goto-based cleanup?

> +
> +	rpmi_tee_block_iter_init(&xfer.iter, args->sg);
> +
> +	/* INIT request. */
> +	req->creator_id = cpu_to_le32(priv->self_id);
> +	req->creator_access = cpu_to_le32(rpmi_tee_access(args->creator_access));
> +	req->receiver_count = cpu_to_le32(args->receiver_count);
> +	req->flags = cpu_to_le32(segmented ? RPMI_TEE_PARCEL_MULTI_SEGMENT : 0);
> +	req->nonce = cpu_to_le32(args->nonce);
> +	req->block_count = cpu_to_le32(xfer.next_block);
> +	memcpy(req->label, args->label, sizeof(req->label));
> +
> +	for (i = 0; i < args->receiver_count; i++) {
> +		u32 tee_access = rpmi_tee_access(args->receivers[i].access);
> +		/* Store RECEIVER_ID[i] and ACCESS[i]. */
> +		rpmi_tee_put_receiver(req->data, args->receiver_count, i,
> +				      args->receivers[i].endpoint_id,
> +				      tee_access);
> +	}
> +
> +	ret = rpmi_tee_fill_blocks(&xfer.iter,
> +				   req->data + 8 * args->receiver_count,
> +				   xfer.next_block);
> +	if (ret)
> +		goto out_release_slot;
> +
> +	resp_len = sizeof(resp);
> +	ret = rpmi_tee_send(priv, RPMI_TEE_SRV_MEMORY_PARCEL_CREATE,
> +			    req, req_len, &resp, &resp_len);
> +	if (ret)
> +		goto out_release_slot;
> +	if (resp_len != sizeof(resp))
> +		return -EPROTO;

[Severity: Medium]
Does this direct return leak the reserved segment slot?

If the firmware returns a malformed response with a valid length but not exactly
equal to sizeof(resp), the direct return of -EPROTO bypasses the
goto out_release_slot; cleanup path.

This causes a permanent leak of the multisegment_active accounting slot, which
can permanently block future multisegment memory operations with -EBUSY once
the limit is reached.

This seems to be a direct consequence of mixing __free() and goto in the same
function. Should this also use goto out_release_slot; to ensure the slot
is properly released?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-riscv-rpmi-tee-abi-v2-0-45939a13d223@oss.qualcomm.com?part=9