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

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

Pre-existing issues:
- [High] Use-after-free due to improper devm_kzalloc allocation of a structure containing an embedded struct device.
- [High] Permanent resource leak of parent and child devices due to a circular reference dependency in the teardown path.
--

--- Patch [2]: [PATCH v3 2/2] ufs: rpmb: use a fixed-length RPMB dev_id ---
commit 9b16b5ee089ea3c3bc7689863d98de4a3af82dfa
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. OP-TEE expects a fixed 16-byte eMMC CID. This patch hashes the
variable-length UFS id into a fixed 16-byte dev_id using blake2b.

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index 5f98472369d00..90dd7fd64ff1f 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c

[ ... ]

> @@ -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;

[Severity: High]
This is a pre-existing issue, but does using devm_kzalloc for a structure
that embeds a struct device lead to a use-after-free?

If a process holds an active reference to the embedded device by keeping a
sysfs attribute open during unbind, devres will automatically free the
memory when the remove function returns. When the process later closes the
file, the device's release callback will operate on the already freed memory:

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

Should this structure be allocated with a standard kzalloc and explicitly
freed inside ufs_rpmb_device_release() to tie its lifetime to the device's
reference count?

>  	u8 dev_id[UFS_RPMB_ID_LEN];
>  	struct rpmb_dev *rdev;
>  	char *cid = NULL;

[ ... ]

> @@ -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 is there a permanent resource
leak here due to a circular reference?

In ufs_rpmb_probe(), the child device is registered under the parent:

ufs_rpmb_probe() {
        ...
        rdev = rpmb_dev_register(&ufs_rpmb->dev, &descr);
        ...
}

Registering the child takes a reference on the parent. During teardown, if
only the parent is unregistered, the child still holds a reference, meaning
the parent's refcount never reaches zero. As a result, the parent's release
callback is never invoked:

ufs_rpmb_device_release() {
        ...
        rpmb_dev_unregister(ufs_rpmb->rdev);
}

Since the child is only unregistered in the parent's release callback, both
devices will be leaked permanently. Should the child be unregistered
explicitly during the removal path before unregistering the parent?

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