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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Allocating a structure embedding a reference-counted `struct device` with `devm_kzalloc()` causes a Use-After-Free due to lifecycle mismatch.
- [High] Circular reference between `ufs_rpmb->dev` and `rdev` prevents device cleanup, leading to permanent resource leaks of both device structures.
- [High] Initialization race: RPMB device is exposed to the subsystem and notifiers before `region_id` is initialized.
- [High] The `ufs_rpmb_bus_type` is never registered with the driver core, causing `device_register()` to deterministically fail.
--

commit a3eaa4c56fa0d1803a39f46b9f298247acd5bdfd
Author: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>

ufs: rpmb: use a fixed-length RPMB dev_id

The RPMB authentication key is derived from the dev_id handed to the
RPMB subsystem. OP-TEE implements the eMMC RPMB flow, where the dev_id
is the eMMC CID: a fixed 16-byte value the key derivation depends on.

[ ... ]

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index 00ba4f3b842bf..783ecfc7581d7 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -21,6 +22,7 @@
>  #include <linux/unaligned.h>
>  #include "ufshcd-priv.h"
>  
> +#define UFS_RPMB_ID_LEN			16	/* Match eMMC CID Length */
>  #define UFS_RPMB_UA_RETRIES		3	/* Retries for the power-on UNIT ATTENTION */
>  #define UFS_RPMB_SEC_PROTOCOL		0xEC	/* JEDEC UFS application */
>  #define UFS_RPMB_SEC_PROTOCOL_ID	0x01	/* JEDEC UFS RPMB protocol ID, CDB byte3 */

[Severity: High]
This is a pre-existing issue, but the ufs_rpmb_bus_type defined nearby is
completely missing a corresponding bus_register() call anywhere in the codebase:

static const struct bus_type ufs_rpmb_bus_type = {
	.name = "ufs_rpmb",
};

When ufs_rpmb_probe() assigns this unregistered bus type to ufs_rpmb->dev.bus:

	ufs_rpmb->dev.bus = &ufs_rpmb_bus_type;

does this guarantee that device_register() will deterministically fail when it
attempts to retrieve the bus subsystem?

[ ... ]

> @@ -157,6 +159,7 @@ static void ufs_rpmb_device_release(struct device *dev)
>  int ufs_rpmb_probe(struct ufs_hba *hba)
>  {
>  	struct ufs_rpmb_dev *ufs_rpmb, *it, *tmp;
> +	u8 dev_id[UFS_RPMB_ID_LEN];
>  	struct rpmb_dev *rdev;
>  	char *cid = NULL;
>  	int region;

[Severity: High]
This isn't a bug introduced by this patch, but ufs_rpmb_device_release() just
above here accesses memory that could have already been freed:

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

Since ufs_rpmb_probe() allocates ufs_rpmb with devm_kzalloc() and subsequently
calls device_register(&ufs_rpmb->dev), if the host driver unbinds, devres will
free ufs_rpmb unconditionally. If a local user holds an open file descriptor or
sysfs reference to the device, the kobject remains alive.

When the final reference is dropped, ufs_rpmb_device_release() is invoked,
which then dereferences the already-freed ufs_rpmb memory. Can this cause a
use-after-free?

[Severity: High]
This is also a pre-existing issue, but child unregistration is incorrectly
placed inside the parent's release function:

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

During driver removal, ufs_rpmb_remove() calls device_unregister(&ufs_rpmb->dev)
on the parent before unregistering the child. Because the child still pins the
parent's refcount, the parent's refcount never reaches zero, preventing its
release function from executing.

Does this create an unbreakable circular reference that prevents device cleanup
and permanently leaks both device structures?

[ ... ]

> @@ -215,8 +218,10 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
>  			goto err_out;
>  		}
>  
> -		descr.dev_id = cid;
> -		descr.dev_id_len = strlen(cid);
> +		blake2b(NULL, 0, cid, strlen(cid), dev_id, UFS_RPMB_ID_LEN);
> +
> +		descr.dev_id = dev_id;
> +		descr.dev_id_len = UFS_RPMB_ID_LEN;
>  		descr.capacity = cap;
>  
>  		/* Register RPMB device */

[Severity: High]
This isn't a bug introduced by this patch, but there is an initialization
race when registering the RPMB device just below this block:

		/* Register RPMB device */
		rdev = rpmb_dev_register(&ufs_rpmb->dev, &descr);
		...
		ufs_rpmb->rdev = rdev;
		ufs_rpmb->region_id = region;

rpmb_dev_register() exposes the device to the rpmb_class, which synchronously
triggers class notifiers. If a notifier like OP-TEE schedules a workqueue to
probe the RPMB device, the worker could issue SMC calls resulting in an RPC
back to ufs_rpmb_route_frames(), which relies on ufs_rpmb->region_id.

Since ufs_rpmb->region_id is initialized after rpmb_dev_register() returns,
could the asynchronous probe thread read an uninitialized region_id for
non-zero regions?

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