Re: [PATCH 2/2] ufs: rpmb: use a fixed-length RPMB dev_id

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Selecting a non-existent Kconfig symbol (`CRYPTO_LIB_BLAKE2S`).

Pre-existing issues:
- [High] Circular reference between the parent `ufs_rpmb->dev` and the child RPMB device `rdev` prevents proper unregistration and causes a memory leak.
- [High] struct ufs_rpmb_dev is allocated with devm_kzalloc but contains a reference-counted struct device, leading to a Use-After-Free.
- [High] TOCTOU race: ufs_rpmb->region_id is initialized after rpmb_dev_register() completes, allowing concurrent requests to use a zeroed region ID.
- [High] ufs_rpmb_bus_type is assigned to the device but never registered, causing device_register() to unconditionally fail.
--

--- Patch [2]: [PATCH 2/2] ufs: rpmb: use a fixed-length RPMB dev_id ---
commit 3594b8409e2ab8cc7cc016e7f74ba42ec3857db5
Author: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
Subject: ufs: rpmb: use a fixed-length RPMB dev_id

This commit changes the UFS RPMB ID generation to use a fixed 16-byte
Blake2s hash instead of a variable-length string. This aligns the UFS RPMB
ID with the eMMC CID layout, ensuring compatibility with OP-TEE.

> diff --git a/drivers/ufs/Kconfig b/drivers/ufs/Kconfig
> index f662e7ce71f1b..0a050565872d2 100644
> --- a/drivers/ufs/Kconfig
> +++ b/drivers/ufs/Kconfig
> @@ -7,6 +7,7 @@ menuconfig SCSI_UFSHCD
>  	tristate "Universal Flash Storage Controller"
>  	depends on SCSI && SCSI_DMA
>  	depends on RPMB || !RPMB
> +	select CRYPTO_LIB_BLAKE2S if RPMB
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Does the CRYPTO_LIB_BLAKE2S config symbol exist?

As documented in lib/crypto/Kconfig, BLAKE2s support is always built-in,
so this select statement appears to reference a non-existent symbol which
could cause unmet direct dependency warnings.

[ ... ]
> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index adbec80cf3811..a7595be0884c6 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
[ ... ]
> @@ -156,6 +158,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
>  {
>  	struct ufs_rpmb_dev *ufs_rpmb, *it, *tmp;

[Severity: High]
This isn't a bug introduced by this patch, but does devm_kzalloc() safely
manage the lifecycle of the reference-counted ufs_rpmb_dev structure?

In ufs_rpmb_probe():
    ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb), GFP_KERNEL);
    ...
    ret = device_register(&ufs_rpmb->dev);

Because devm_kzalloc() ties the memory to the host device's devres, the
devres subsystem will automatically free ufs_rpmb during driver unbind. If
the device reference count is still elevated at that time (e.g. via sysfs),
could the release callback ufs_rpmb_device_release() later dereference this
freed memory when the reference count finally drops to zero?


[Severity: High]
This isn't a bug introduced by this patch, but does the UFS RPMB bus
registration fail unconditionally?

In ufs_rpmb_probe(), the bus is assigned:
    ufs_rpmb->dev.bus = &ufs_rpmb_bus_type;

However, ufs_rpmb_bus_type appears to be statically defined and never
registered via bus_register(). When device_register(&ufs_rpmb->dev) is
called, will it fail with -EINVAL because the driver core cannot find the
bus subsystem?

[ ... ]
> @@ -230,6 +242,8 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
>  
>  		kfree(cid);
>  		cid = NULL;
> +		kfree(dev_id);
> +		dev_id = NULL;
>  
>  		ufs_rpmb->rdev = rdev;

[Severity: High]
This is a pre-existing issue, but does this create a circular reference
that prevents the device from ever being unregistered?

ufs_rpmb_probe() registers a child device:
    rdev = rpmb_dev_register(&ufs_rpmb->dev, &descr);

The child holds a reference to the parent. In ufs_rpmb_remove(), the parent
is unregistered without explicitly unregistering the child:
    device_unregister(&ufs_rpmb->dev);

Since the release callback ufs_rpmb_device_release() contains the call to
rpmb_dev_unregister(ufs_rpmb->rdev), it will never be called because the
child keeps the parent's reference count elevated. Can this lead to a
permanent memory leak of the device?

>  		ufs_rpmb->region_id = region;

[Severity: High]
This is a pre-existing issue, but is there a race condition between
registering the RPMB device and setting the region_id?

In ufs_rpmb_probe():
    rdev = rpmb_dev_register(&ufs_rpmb->dev, &descr);
    ...
    ufs_rpmb->region_id = region;

When rpmb_dev_register() completes, the RPMB class interface's add_dev
callback could schedule a background worker to access the device. If this
worker issues requests before region_id is initialized, could
ufs_rpmb_route_frames() read a zero-initialized region_id and incorrectly
route requests?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715214327.1933560-1-jorge.ramirez@oss.qualcomm.com?part=2