Re: [PATCH v2 1/2] scsi: ufs: core: unregister RPMB devices before their parent

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ufs: core: devm_kzalloc() used for struct device in ufs_rpmb_probe()
--

commit dec08b39580959e5820891b21f9cd97a5b04657e
Author: shao.mingyin@zte.com.cn <shao.mingyin@zte.com.cn>

scsi: ufs: core: unregister RPMB devices before their parent

This commit adds ufs_rpmb_unregister_device() to unregister ufs_rpmb->rdev
before calling device_unregister(&ufs_rpmb->dev) during ufs_rpmb_remove()
and the ufs_rpmb_probe() error path, and updates ufs_rpmb_device_release()
to check whether ufs_rpmb->rdev is non-NULL before unregistering it.

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index 65f2d71cf073b..1ed57d50c3ff0 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -149,11 +149,19 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
>  	return ret;
>  }
>  
> +static void ufs_rpmb_unregister_device(struct ufs_rpmb_dev *ufs_rpmb)
> +{
> +	rpmb_dev_unregister(ufs_rpmb->rdev);
> +	ufs_rpmb->rdev = NULL;
> +	device_unregister(&ufs_rpmb->dev);
> +}
> +
>  static void ufs_rpmb_device_release(struct device *dev)
>  {
>  	struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);
>  
> -	rpmb_dev_unregister(ufs_rpmb->rdev);
> +	if (ufs_rpmb->rdev)
> +		rpmb_dev_unregister(ufs_rpmb->rdev);
>  }

[Severity: High]
Can ufs_rpmb->rdev ever be non-NULL in ufs_rpmb_device_release(), and can
dereferencing ufs_rpmb here cause a use-after-free?

In drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_probe(), struct ufs_rpmb_dev embeds
struct device dev, yet it is allocated with devm_kzalloc() on hba->dev:

    ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb), GFP_KERNEL);
    ...
    ufs_rpmb->dev.release = ufs_rpmb_device_release;

While ufs_rpmb->rdev is registered, rdev->dev.parent points to
&ufs_rpmb->dev and holds a reference on it until rpmb_dev_unregister() calls
device_del(&rdev->dev). The only caller of rpmb_dev_unregister() is
ufs_rpmb_unregister_device(), which sets ufs_rpmb->rdev = NULL before
calling device_unregister(&ufs_rpmb->dev), and on probe error paths prior to
rpmb_dev_register(), ufs_rpmb->rdev is already NULL. As a result,
ufs_rpmb->rdev is always NULL when ufs_rpmb_device_release() runs.

More importantly, because ufs_rpmb is allocated with devm_kzalloc(hba->dev,
...), its memory is freed when hba->dev detaches rather than when the
reference count on ufs_rpmb->dev drops to zero. In rpmb_dev_unregister(),
device_del(&rdev->dev) drops the parent reference on &ufs_rpmb->dev
immediately even if OP-TEE still holds a reference to rdev (for example via
optee->rpmb_dev or an in-flight RPMB frame request), and sysfs or
CONFIG_DEBUG_KOBJECT_RELEASE can also keep &ufs_rpmb->dev alive past HBA
driver unbind. Once devres_release_all(hba->dev) frees ufs_rpmb, a subsequent
call to ufs_rpmb_route_frames() or the final put_device(&ufs_rpmb->dev) ->
ufs_rpmb_device_release() will dereference freed ufs_rpmb memory. (Also, if
ufs_rpmb_probe() fails, ufshcd_add_lus() ignores the error and continues,
leaving the devm-allocated ufs_rpmb leaked until HBA removal.)

Should ufs_rpmb_probe() allocate ufs_rpmb with kzalloc() and have
ufs_rpmb_device_release() free it with kfree(ufs_rpmb), matching how
mmc_blk_alloc_rpmb_part() and mmc_blk_rpmb_device_release() manage struct
mmc_rpmb_data in drivers/mmc/core/block.c?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/202611040259247915t_cYYK0k5pPbEqfdTAQu@zte.com.cn?part=1