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

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

Pre-existing issues:
- [High] The bus type `ufs_rpmb_bus_type` is never registered, causing device_register() to unconditionally fail.
- [High] Using devm_kzalloc for a structure embedding a struct device violates driver model lifetime rules, leading to a Use-After-Free on unbind and memory leaks on error paths.
- [High] Parent-child reference counting deadlock in device removal paths prevents unregistration and causes a permanent memory and device leak.
--

commit ecc19262c86a06d5ad583f54476cad317a179650
Author: Jorge Ramirez-Ortiz <[email protected]>

ufs: rpmb: use a fixed-length RPMB dev_id

The RPMB authentication key is derived from the dev_id handed to the RPMB
subsystem. A fixed 16-byte dev_id is needed so the derived key stays stable
and unique per region while matching the eMMC CID layout.

> 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

[ ... ]

> @@ -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 looking at how the bus is
assigned slightly earlier in ufs_rpmb_probe():

	ufs_rpmb->dev.bus = &ufs_rpmb_bus_type;

Is ufs_rpmb_bus_type ever registered with bus_register()? If it is never
registered, won't device_register() unconditionally fail with -EINVAL because
it cannot find the subsystem for the unregistered bus, breaking the UFS RPMB
registration on every boot?

[Severity: High]
This is a pre-existing issue, but I noticed that ufs_rpmb_probe() allocates
the ufs_rpmb structure using devm_kzalloc() slightly earlier in the same
function:

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

Since this structure embeds a struct device (ufs_rpmb->dev) which gets
registered with device_register(), could this lead to a use-after-free on
unbind?

If a user unbinds the hba->dev driver while holding a sysfs reference to
ufs_rpmb->dev, devres will immediately free the memory. When the user later
drops the final reference, wouldn't the kernel access the freed memory when
calling ufs_rpmb_device_release()?

[Severity: High]
This is a pre-existing issue, but is there an unresolvable cyclic dependency
during device removal? Looking at how the child device is unregistered inside
the parent's release callback:

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_device_release() {
	struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);

	rpmb_dev_unregister(ufs_rpmb->rdev);
}

When device_unregister(&ufs_rpmb->dev) is called during a probe error path or
removal, the child device (rdev) is not unregistered first. Since the child
still holds a reference to the parent, the parent's refcount never reaches 0.
Wouldn't this prevent ufs_rpmb_device_release() from ever executing, causing
both devices to be permanently leaked?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2