Re: [PATCH 4/5] scsi: ufs: rpmb: Validate frame before parsing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The UFS RPMB driver mismanages the lifecycle of `struct ufs_rpmb_dev`, resulting in Use-After-Free (UAF) vulnerabilities during teardown and unbind operations. This problem wasn't introduced by this patch, but it is a critical flaw in the driver's device management.
--

--- Patch [4]: [PATCH 4/5] scsi: ufs: rpmb: Validate frame before parsing ---
commit 52562de70e8a3a9ec694967f1ae0889763de9535
Author: Li Qiang <liqiang01@kylinos.cn>

scsi: ufs: rpmb: Validate frame before parsing
    
This commit validates the request length before accessing req_resp at the
end of the first request frame. It also uses unaligned accessors for safe
access to the RPMB buffers.

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index ffad049872b9a..53f66b274acac 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c

[ ... ]

> @@ -69,7 +69,11 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
>  
>  	hba = ufs_rpmb->hba;
>  
> -	req_type = be16_to_cpu(frm_out->req_resp);
> +	/* req_resp is at the end of an RPMB frame. */
> +	if (req_len < sizeof(*frm_out))
> +		return -EINVAL;
> +
> +	req_type = get_unaligned_be16(&frm_out->req_resp);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file I
noticed a pre-existing issue with the lifecycle management of ufs_rpmb_dev.

Can this lead to a use-after-free during device unbind? The struct is
allocated using devm_kzalloc():

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_probe() {
    ...
    ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb), GFP_KERNEL);
    ...
}

But it embeds a reference-counted struct device. When the UFS host controller
unbinds, the devres cleanup executes and frees ufs_rpmb_dev. If a user-space
process holds a sysfs reference to the UFS RPMB device, the delayed
release callback will later access this freed memory:

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_device_release() {
    ...
    rpmb_dev_unregister(ufs_rpmb->rdev);
    ...
}

Additionally, because rpmb_dev_unregister() is called asynchronously in the
.release callback rather than synchronously in ufs_rpmb_remove():

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_remove() {
    ...
    device_unregister(&ufs_rpmb->dev);
    ...
}

Could the RPMB core issue requests to a fully torn-down UFS host controller?

Should rpmb_dev_unregister() be moved to ufs_rpmb_remove(), and should
ufs_rpmb_dev be freed with kfree() inside ufs_rpmb_device_release() instead
of relying on devm?

[ ... ]

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