Re: [PATCH v2 4/6] scsi: ufs: rpmb: Validate request frame length before parsing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing IS_ERR() check for tee_shm_get_va() return values in OP-TEE RPMB RPC handler.
- [High] Circular refcount and incorrect use of devm_kzalloc() for a dynamically registered struct device lead to a memory leak and severe Use-After-Free when UFS is unbound.
--

--- Patch [4]: [PATCH v2 4/6] scsi: ufs: rpmb: Validate request frame length before parsing ---
commit 1f68e5566b9d9ef98707ec46f344522aaa25cb6b
Author: Li Qiang <liqiang01@kylinos.cn>

scsi: ufs: rpmb: Validate request frame length before parsing

The RPMB core only verifies that request and response buffers are nonempty.
This commit adds a check to ensure a complete RPMB request frame is provided
before reading req_resp at the end of the frame.

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index ffad049872b9a..431f85b368760 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -69,6 +69,10 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
>  
>  	hba = ufs_rpmb->hba;

[Severity: High]
This is a pre-existing issue, but does this lead to a use-after-free when the
UFS host is unbound?

Because ufs_rpmb was allocated with devm_kzalloc(), it is automatically freed
on unbind. However, ufs_rpmb_remove() unregisters ufs_rpmb->dev but fails to
unregister the child rdev. Since rdev->dev implicitly holds a parent reference
to ufs_rpmb->dev, ufs_rpmb->dev's refcount never drops to zero, and
ufs_rpmb_device_release() is never invoked to call rpmb_dev_unregister().

If a subsequent RPMB request routes a frame, will it extract and dereference
the freed ufs_rpmb object here?

> +	/* req_resp is at the end of an RPMB frame. */
> +	if (req_len < sizeof(*frm_out))
> +		return -EINVAL;
> +
>  	req_type = be16_to_cpu(frm_out->req_resp);

[Severity: High]
This isn't a bug introduced by this patch, but can we dereference an error
pointer here?

If drivers/tee/optee/rpc.c:handle_rpc_func_rpmb_frames() receives an
out-of-bounds offset from the Secure World, tee_shm_get_va() returns
ERR_PTR(-EINVAL). Because there is no IS_ERR() check, this error pointer is
passed as 'req' to rpmb_route_frames() and eventually reaches here as frm_out.

Will accessing frm_out->req_resp cause a panic if the Secure World sends an
invalid offset?

>  
>  	switch (req_type) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717153914.26321-1-liqiang01@kylinos.cn?part=4