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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: physical address truncation in rpmi_tee_block_iter_next()
- [Medium] mm: resource leak of segment slot in rpmi_tee_parcel_create() error path
- [Low] subsystem: mixing goto-based error handling and __free() in rpmi_tee_parcel_create()
--

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

firmware: riscv: rpmi-tee: add memory parcel operations
    
Implement lend, share, and reclaim of RPMI memory parcels for public
memory endpoints. Convert scatterlists into RPMI page blocks and
transparently split large parcels across multiple requests when they
do not fit a single mailbox message, subject to the per-endpoint
multisegment limit reported by firmware.

> diff --git a/drivers/firmware/riscv_rpmi_tee/driver.c b/drivers/firmware/riscv_rpmi_tee/driver.c
> index 7b06c1f96ba83..0004f54c7d899 100644
> --- a/drivers/firmware/riscv_rpmi_tee/driver.c
> +++ b/drivers/firmware/riscv_rpmi_tee/driver.c

[ ... ]

> +static bool rpmi_tee_block_iter_next(struct rpmi_tee_block_iter *iter,
> +				     u32 *high, u32 *low)
> +{
> +	u32 pages;
> +
> +	if (!iter->length) {
> +		if (!iter->sg)
> +			return false;
> +		/* Next SG. */
> +		iter->address = sg_phys(iter->sg);
> +		iter->length = iter->sg->length;
> +		iter->sg = sg_next(iter->sg);
> +	}
> +
> +	/* RPMI memory block represents at most @RPMI_TEE_BLOCK_MAX_PAGES pages. */
> +	pages = min_t(size_t, iter->length >> RPMI_TEE_MEM_PAGE_SHIFT,
> +		      RPMI_TEE_BLOCK_MAX_PAGES);
> +
> +	*high = upper_32_bits(iter->address >> RPMI_TEE_MEM_PAGE_SHIFT);
> +	*low = lower_32_bits(iter->address >> RPMI_TEE_MEM_PAGE_SHIFT) << 12 |
> +		(pages - 1);

[Severity: High]
Does this code inadvertently truncate physical addresses at or above 4GB?

Because lower_32_bits() returns a u32, shifting the result left by 12 will
overflow and lose the upper 12 bits, which correspond to bits 43..32 of the
original physical address. 

Similarly, upper_32_bits() will extract bits 75..44 instead of 63..32.

This might cause the firmware to receive incorrect physical addresses,
leading to the TEE accessing the wrong memory and potentially causing data
corruption or crashes during sharing/lending requests.

> +
> +	iter->address += (phys_addr_t)pages << RPMI_TEE_MEM_PAGE_SHIFT;
> +	iter->length -= (size_t)pages << RPMI_TEE_MEM_PAGE_SHIFT;
> +
> +	return true;
> +}

[ ... ]

> +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);

[Severity: Low]
Is it intentional to mix __free() with goto-based cleanup here?

The subsystem guidelines recommend converting all resources that need a goto
cleanup to scope-based cleanup, or converting none of them. This function
uses __free(kfree) for the req allocation but also maintains a goto
out_release_slot label for releasing the segment slot.

> +	if (!req) {
> +		ret = -ENOMEM;
> +		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]
Can this direct return leak a segment slot on unexpected response lengths?

If rpmi_tee_send() succeeds but the response length is invalid, returning
-EPROTO directly skips the out_release_slot label. 

This fails to decrement priv->mem.multisegment_active, leaking the reserved
slot and eventually exhausting multisegment_max, which would permanently
block future multi-segment operations with -EBUSY.

> +
> +	xfer.parcel_id = get_unaligned_le32(&resp.parcel_id);
> +	/* Send remaining blocks as segments. */
> +	ret = rpmi_tee_parcel_send_segments(priv, &xfer);
> +	if (ret) {
> +		/* On error, retain the slot as firmware may still hold it. */
> +		if (rpmi_tee_memory_reclaim(priv, xfer.parcel_id)) {
> +			dev_warn(priv->dev, "failed to abort parcel %#x\n",
> +				 xfer.parcel_id);
> +
> +			return ret;
> +		}
> +	} else {
> +		args->parcel_id = xfer.parcel_id;
> +	}
> +
> +out_release_slot:
> +	if (segmented)
> +		rpmi_tee_release_segment_slot(priv);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-riscv-rpmi-tee-abi-v1-0-04908b81d885@oss.qualcomm.com?part=9